Discussion:
bug#33287: [PATCH] sync: add missing brackets in sync_arg()
(too old to reply)
Kamil Dudka
2018-11-06 12:02:24 UTC
Permalink
Detected by Coverity Analysis:

Error: RESOURCE_LEAK (CWE-772):
coreutils-8.30/src/sync.c:112: open_fn: Returning handle opened by "open".
coreutils-8.30/src/sync.c:112: var_assign: Assigning: "fd" = handle returned from "open(file, 2049)".
coreutils-8.30/src/sync.c:115: leaked_handle: Handle variable "fd" going out of scope leaks the handle.
113| if (fd < 0)
114| error (0, rd_errno, _("error opening %s"), quoteaf (file));
115|-> return false;
116| }
117|
---
src/sync.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/sync.c b/src/sync.c
index bd3671a19..607fa8f7f 100644
--- a/src/sync.c
+++ b/src/sync.c
@@ -111,8 +111,10 @@ sync_arg (enum sync_mode mode, char const *file)
if (open_flags != (O_WRONLY | O_NONBLOCK))
fd = open (file, O_WRONLY | O_NONBLOCK);
if (fd < 0)
- error (0, rd_errno, _("error opening %s"), quoteaf (file));
- return false;
+ {
+ error (0, rd_errno, _("error opening %s"), quoteaf (file));
+ return false;
+ }
}

/* We used O_NONBLOCK above to not hang with fifos,
--
2.17.2
Paul Eggert
2018-11-06 18:35:58 UTC
Permalink
Thanks, I installed that and am closing the bug report.
Bernhard Voelker
2018-11-06 23:33:45 UTC
Permalink
Post by Paul Eggert
Thanks, I installed that and am closing the bug report.
That was a real bug, i.e., not only a resource leak, wasn't it?

If the calling user has -r+w permissions on the file, then sync previously
exited with 1 without actually syncing:

$ install -m 0200 /dev/null /tmp/file

$ ls -log /tmp/file
--w------- 1 0 Nov 7 00:06 /tmp/file

$ strace -v /usr/bin/sync /tmp/file 2>&1 | tail -n6
openat(AT_FDCWD, "/tmp/file", O_RDONLY|O_NONBLOCK) = -1 EACCES (Permission denied)
openat(AT_FDCWD, "/tmp/file", O_WRONLY|O_NONBLOCK) = 3
close(1) = 0
close(2) = 0
exit_group(1) = ?
+++ exited with 1 +++

With the patch, fsync is called, and sync terminated with success:

$ strace -v src/sync /tmp/file 2>&1 | tail
openat(AT_FDCWD, "/tmp/file", O_RDONLY|O_NONBLOCK) = -1 EACCES (Permission denied)
openat(AT_FDCWD, "/tmp/file", O_WRONLY|O_NONBLOCK) = 3
fcntl(3, F_GETFL) = 0x8801 (flags O_WRONLY|O_NONBLOCK|O_LARGEFILE)
fcntl(3, F_SETFL, O_WRONLY|O_LARGEFILE) = 0
fsync(3) = 0
close(3) = 0
close(1) = 0
close(2) = 0
exit_group(0) = ?
+++ exited with 0 +++

Should we add a NEWS entry and a test - see attached?

Thanks & have a nice day,
Berny
Paul Eggert
2018-11-07 01:02:47 UTC
Permalink
Post by Bernhard Voelker
Should we add a NEWS entry and a test - see attached?
Thanks, good point, and I installed that.
Kamil Dudka
2018-11-07 08:24:38 UTC
Permalink
Post by Bernhard Voelker
Post by Paul Eggert
Thanks, I installed that and am closing the bug report.
That was a real bug, i.e., not only a resource leak, wasn't it?
If the calling user has -r+w permissions on the file, then sync previously
$ install -m 0200 /dev/null /tmp/file
$ ls -log /tmp/file
--w------- 1 0 Nov 7 00:06 /tmp/file
$ strace -v /usr/bin/sync /tmp/file 2>&1 | tail -n6
openat(AT_FDCWD, "/tmp/file", O_RDONLY|O_NONBLOCK) = -1 EACCES (Permission
denied) openat(AT_FDCWD, "/tmp/file", O_WRONLY|O_NONBLOCK) = 3
close(1) = 0
close(2) = 0
exit_group(1) = ?
+++ exited with 1 +++
$ strace -v src/sync /tmp/file 2>&1 | tail
openat(AT_FDCWD, "/tmp/file", O_RDONLY|O_NONBLOCK) = -1 EACCES (Permission
denied) openat(AT_FDCWD, "/tmp/file", O_WRONLY|O_NONBLOCK) = 3
fcntl(3, F_GETFL) = 0x8801 (flags
O_WRONLY|O_NONBLOCK|O_LARGEFILE) fcntl(3, F_SETFL, O_WRONLY|O_LARGEFILE) =
0
fsync(3) = 0
close(3) = 0
close(1) = 0
close(2) = 0
exit_group(0) = ?
+++ exited with 0 +++
Should we add a NEWS entry and a test - see attached?
Looks good. Thanks for the follow-up patch!

Kamil
Post by Bernhard Voelker
Thanks & have a nice day,
Berny
Continue reading on narkive:
Loading...