Subject: Re: bug#33287: [PATCH] sync: add missing brackets in sync_arg()

From: Bernhard Voelker <[email-masked]>
Date: Wed, 7 Nov 2018 00:33:45 +0100

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
From 90679f499b71f2432a7faa439bc7a9155e7bee9c Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <[email-masked]>
Date: Wed, 7 Nov 2018 00:26:01 +0100
Subject: [PATCH] sync: add NEWS and test for the fix in the previous commit

* NEWS (Bug fixes): Mention the fix in commit 94d364f157f0.
While at it, remove duplicate "Changes in behavior" heading.
* tests/misc/sync.sh: Add a test with a write-only file for the fix.
---
 NEWS               | 5 +++--
 tests/misc/sync.sh | 7 ++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/NEWS b/NEWS
index 088810fe8..8c1283549 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   df no longer corrupts displayed multibyte characters on macOS.
   [bug introduced with coreutils-8.18]
 
+  sync no longer fails for write-only file arguments.
+  [bug introduced with argument support to sync in coreutils-8.24]
+
 ** Changes in behavior
 
   echo now always processes backslash escapes when the POSIXLY_CORRECT
@@ -22,8 +25,6 @@ GNU coreutils NEWS                                    -*- outline -*-
   approach is still used in situations where hard links to directories
   are allowed (e.g., NetBSD when superuser).
 
-** Changes in behavior
-
   'test -a FILE' is not supported anymore.  Long ago, there were concerns about
   the high probability of humans confusing the -a primary with the -a binary
   operator, so POSIX changed this to 'test -e FILE'.  Scripts using it were
diff --git a/tests/misc/sync.sh b/tests/misc/sync.sh
index f60d28c5a..3bb6e179e 100755
--- a/tests/misc/sync.sh
+++ b/tests/misc/sync.sh
@@ -19,7 +19,7 @@
 . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
 print_ver_ sync
 
-touch file
+touch file || framework_failure_
 
 # fdatasync+syncfs is nonsensical
 returns_ 1 sync --data --file-system || fail=1
@@ -30,6 +30,11 @@ returns_ 1 sync -d || fail=1
 # Test syncing of file (fsync) (little side effects)
 sync file || fail=1
 
+# Test syncing of write-only file - which failed since adding argument
+# support to sync in coreutils-8.24.
+chmod 0200 file || framework_failure_
+sync file || fail=1
+
 # Ensure multiple args are processed and diagnosed
 returns_ 1 sync file nofile || fail=1
 
-- 
2.19.1