Package: release.debian.org
Severity: normal
Tags: bookworm
X-Debbugs-Cc: k...@packages.debian.org, couc...@debian.org, s...@vuorela.dk,
r...@debian.org, a...@debian.org
Control: affects -1 + src:kio
User: release.debian....@packages.debian.org
Usertags: pu
[ Reason ]
With network shares mounted using CIFS, under some circumstances
libreoffice documents may disappear from the file system. The
underlying reason are locking mechanisms by the file system.
A lock prevents moving a temporary file when saving the document.
The latter gets lost in the unfortunate case.
A patched kio seems to fix the problem.
The problem is described in more detail #1070322 and in #1069855 (the
latter, related to the same underlying reason, is not fixed by the
patches available so far).
[ Impact ]
For the person preparing a document in libreoffice, the impact is
rather high, of course depending on the work that has been put into
the document's preparation so far.
[ Tests ]
We've tested the patch on our systems (a school with perhaps right now
about several hundred active PC users per day) and did not notice any
suspicious events.
The case with libreoffice documents was not reproducible with the
patched kio.
The problem with ark (#1069855) remains.
( https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1070322#129 )
[ Risks ]
I cannot estimate the risk apart from reporting our successful tests.
Perhaps people more familiar with the code can comment.
[ Checklist ]
[X] *all* changes are documented in the d/changelog
[X] I reviewed all changes and I approve them
[X] attach debdiff against the package in (old)stable
[X] the issue is verified as fixed in unstable (only the libreoffice one)
[ Changes ]
>From the Changelog:
- Don't unlink + rename on CIFS mounts during copy operations; Don't crash
if KMountPoint gives nothing back while checking.
- Don't leak existing file handles to newly spanwed KIO workers.
[ Other info ]
-
diff -Nru kio-5.103.0/debian/changelog kio-5.103.0/debian/changelog
--- kio-5.103.0/debian/changelog 2023-02-12 21:44:31.000000000 +0100
+++ kio-5.103.0/debian/changelog 2024-05-23 23:13:17.000000000 +0200
@@ -1,3 +1,14 @@
+kio (5.103.0-1+deb12u1) bookworm; urgency=medium
+
+ [ Aurélien COUDERC ]
+ * Backport upstream patches to fix incorrect behaviours with CIFS:
+ - Don't unlink + rename on CIFS mounts during copy operations; Don't crash
+ if KMountPoint gives nothing back while checking. (Closes: #1069855)
+ - Don't leak existing file handles to newly spanwed KIO workers.
+ (Closes: #1070322)
+
+ -- Aurélien COUDERC <couc...@debian.org> Thu, 23 May 2024 23:13:17 +0200
+
kio (5.103.0-1) unstable; urgency=medium
[ Aurélien COUDERC ]
diff -Nru kio-5.103.0/debian/patches/fix_cifs_file_locks.patch
kio-5.103.0/debian/patches/fix_cifs_file_locks.patch
--- kio-5.103.0/debian/patches/fix_cifs_file_locks.patch 1970-01-01
01:00:00.000000000 +0100
+++ kio-5.103.0/debian/patches/fix_cifs_file_locks.patch 2024-05-23
23:13:17.000000000 +0200
@@ -0,0 +1,45 @@
+From d1a2dab1da43d613ae5a8459ddcb62c8d78c46ff Mon Sep 17 00:00:00 2001
+From: Kevin Ottens <kevin.ott...@enioka.com>
+Date: Fri, 5 Jan 2024 11:51:49 +0100
+Subject: [PATCH] Don't leak file descriptors when spawning new workers
+
+By default we inherit file descriptors from the parent in
+the worker process. This is a leak of resources since the
+worker won't be able to do anything with them. Also, in
+the case of CIFS this causes locks which might lead to bad
+surprises in the parent process.
+---
+
+Index: kio-5.103.0/src/kioslave/kioslave.cpp
+===================================================================
+--- kio-5.103.0.orig/src/kioslave/kioslave.cpp
++++ kio-5.103.0/src/kioslave/kioslave.cpp
+@@ -18,6 +18,10 @@
+ #include <QPluginLoader>
+ #include <QString>
+
++#ifdef Q_OS_UNIX
++#include <sys/resource.h>
++#endif
++
+ #ifdef Q_OS_WIN
+ #include <QProcess>
+ #include <QStandardPaths>
+@@ -40,6 +44,17 @@ extern "C" KIO::AuthInfo *_kioslave_init
+
+ int main(int argc, char **argv)
+ {
++#ifdef Q_OS_UNIX
++ int max_fd = INT_MAX;
++ struct rlimit limit;
++ if (getrlimit(RLIMIT_NOFILE, &limit) == 0) {
++ max_fd = limit.rlim_cur;
++ }
++ for (int fd = STDERR_FILENO + 1; fd < max_fd; fd++) {
++ ::close(fd);
++ }
++#endif
++
+ if (argc < 5) {
+ fprintf(stderr, "Usage: kioslave5 <slave-lib> <protocol>
<klauncher-socket> <app-socket>\n\nThis program is part of KDE.\n");
+ return 1;
diff -Nru kio-5.103.0/debian/patches/series kio-5.103.0/debian/patches/series
--- kio-5.103.0/debian/patches/series 2022-05-12 22:53:40.000000000 +0200
+++ kio-5.103.0/debian/patches/series 2024-05-23 23:13:17.000000000 +0200
@@ -2,3 +2,6 @@
#fix_kfreebsd_build
hurd_disable_unimplemented.diff
Use-CXX_FLAGS-for-moc_predefs.h.patch
+upstream_3e6800b3_fix_cifs_copy.patch
+upstream_48322f44_fix_crash_when_kmountpoint_gives_nothing_on_cifs.patch
+fix_cifs_file_locks.patch
diff -Nru kio-5.103.0/debian/patches/upstream_3e6800b3_fix_cifs_copy.patch
kio-5.103.0/debian/patches/upstream_3e6800b3_fix_cifs_copy.patch
--- kio-5.103.0/debian/patches/upstream_3e6800b3_fix_cifs_copy.patch
1970-01-01 01:00:00.000000000 +0100
+++ kio-5.103.0/debian/patches/upstream_3e6800b3_fix_cifs_copy.patch
2024-05-23 23:13:17.000000000 +0200
@@ -0,0 +1,53 @@
+From 3e6800b378cd143cb9c4ca4cfa500916a5e5c17c Mon Sep 17 00:00:00 2001
+From: Kevin Ottens <kevin.ott...@enioka.com>
+Date: Thu, 24 Aug 2023 18:42:19 +0200
+Subject: [PATCH] Don't unlink + rename on CIFS mounts during copy operations
+
+It turns out that the behavior of unlink() on CIFS mounts can be a bit
+"interesting". If the file one tries to unlink is opened then the
+operation is claimed to have succeeded but the filename is still visible
+in the file hierarchy until the last handle is closed.
+
+Since we rightfully attempt to copy under a temporary name + unlink +
+rename during copy() operations this would end badly (as the unlink()
+would "succeed" but the rename() would fail!). For instance Okular would
+constantly hit this case but it's likely not the only one to be affected.
+
+So instead we detect if the destination is a CIFS mount, in such cases
+we now overwrite the file directly since this actually succeed.
+
+BUG: 454693
+(cherry picked from commit d248949eea3e3dcbb9283f30eebcb9ae86412cd1)
+---
+ src/ioslaves/file/file_unix.cpp | 8 +++++++-
+ 1 file changed, 7 insertions(+), 1 deletion(-)
+
+diff --git a/src/ioslaves/file/file_unix.cpp b/src/ioslaves/file/file_unix.cpp
+index 8d005daf81..87c47e7e74 100644
+--- a/src/ioslaves/file/file_unix.cpp
++++ b/src/ioslaves/file/file_unix.cpp
+@@ -325,6 +325,12 @@ inline static time_t stat_mtime(const QT_STATBUF &buf)
+ }
+ #endif
+
++static bool isOnCifsMount(const QString &filePath)
++{
++ const auto mount = KMountPoint::currentMountPoints().findByPath(filePath);
++ return mount->mountType() == QStringLiteral("cifs") || mount->mountType()
== QStringLiteral("smb3");
++}
++
+ static bool createUDSEntry(const QString &filename, const QByteArray &path,
UDSEntry &entry, KIO::StatDetails details, const QString &fullPath)
+ {
+ assert(entry.count() == 0); // by contract :-)
+@@ -734,7 +740,7 @@ void FileProtocol::copy(const QUrl &srcUrl, const QUrl
&destUrl, int _mode, JobF
+ return;
+ }
+ }
+- } else if (S_ISREG(buffDest.st_mode)) {
++ } else if (S_ISREG(buffDest.st_mode) && !isOnCifsMount(dest)) {
+ _destBackup = _dest;
+ dest.append(QStringLiteral(".part"));
+ _dest = QFile::encodeName(dest);
+--
+GitLab
+
diff -Nru
kio-5.103.0/debian/patches/upstream_48322f44_fix_crash_when_kmountpoint_gives_nothing_on_cifs.patch
kio-5.103.0/debian/patches/upstream_48322f44_fix_crash_when_kmountpoint_gives_nothing_on_cifs.patch
---
kio-5.103.0/debian/patches/upstream_48322f44_fix_crash_when_kmountpoint_gives_nothing_on_cifs.patch
1970-01-01 01:00:00.000000000 +0100
+++
kio-5.103.0/debian/patches/upstream_48322f44_fix_crash_when_kmountpoint_gives_nothing_on_cifs.patch
2024-05-23 23:13:17.000000000 +0200
@@ -0,0 +1,28 @@
+From 48322f44323a1fc09305d66d9093fe6c3780709e Mon Sep 17 00:00:00 2001
+From: Kevin Ottens <kevin.ott...@enioka.com>
+Date: Fri, 15 Sep 2023 09:45:58 +0200
+Subject: [PATCH] Don't crash if KMountPoint gives nothing back while checking
+ for CIFS
+
+BUG: 474451
+---
+ src/ioslaves/file/file_unix.cpp | 3 +++
+ 1 file changed, 3 insertions(+)
+
+diff --git a/src/ioslaves/file/file_unix.cpp b/src/ioslaves/file/file_unix.cpp
+index 87c47e7e74..c0bc64354d 100644
+--- a/src/ioslaves/file/file_unix.cpp
++++ b/src/ioslaves/file/file_unix.cpp
+@@ -328,6 +328,9 @@ inline static time_t stat_mtime(const QT_STATBUF &buf)
+ static bool isOnCifsMount(const QString &filePath)
+ {
+ const auto mount = KMountPoint::currentMountPoints().findByPath(filePath);
++ if (!mount) {
++ return false;
++ }
+ return mount->mountType() == QStringLiteral("cifs") || mount->mountType()
== QStringLiteral("smb3");
+ }
+
+--
+GitLab
+
