GNU bug report logs - #33287
[PATCH] sync: add missing brackets in sync_arg()

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 33287 in the body.
You can then email your comments to 33287 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] sync: add missing brackets in sync_arg()
Date: Tue,  6 Nov 2018 13:02:24 +0100

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

Message #10 received at 33287-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Kamil Dudka <kdudka <at> redhat.com>, 33287-done <at> debbugs.gnu.org
Subject: Re: bug#33287: [PATCH] sync: add missing brackets in sync_arg()
Date: Tue, 6 Nov 2018 10:35:58 -0800

Thanks, I installed that and am closing the bug report.

Message #13 received at 33287 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: 33287 <at> debbugs.gnu.org, eggert <at> cs.ucla.edu, kdudka <at> redhat.com
Subject: Re: bug#33287: [PATCH] sync: add missing brackets in sync_arg()
Date: Wed, 7 Nov 2018 00:33:45 +0100

[Message part 1 (text/plain, inline)]

On 11/6/18 7:35 PM, Paul Eggert wrote:
> 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

[0001-sync-add-NEWS-and-test-for-the-fix-in-the-previous-c.patch (text/x-patch, attachment)]

Message #16 received at 33287 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>, 33287 <at> debbugs.gnu.org,
 kdudka <at> redhat.com
Subject: Re: bug#33287: [PATCH] sync: add missing brackets in sync_arg()
Date: Tue, 6 Nov 2018 17:02:47 -0800

On 11/6/18 3:33 PM, Bernhard Voelker wrote:
> Should we add a NEWS entry and a test - see attached?

Thanks, good point, and I installed that.

Message #19 received at 33287 <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: eggert <at> cs.ucla.edu, 33287 <at> debbugs.gnu.org
Subject: Re: bug#33287: [PATCH] sync: add missing brackets in sync_arg()
Date: Wed, 07 Nov 2018 09:24:38 +0100

On Wednesday, November 7, 2018 12:33:45 AM CET Bernhard Voelker wrote:
> On 11/6/18 7:35 PM, Paul Eggert wrote:
> > 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?

Looks good.  Thanks for the follow-up patch!

Kamil

> Thanks & have a nice day,
> Berny

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.