Re: [PATCH] dd: fix check of return value of posix_fadvise()

[Pádraig Brady]

On 09/03/2025 17:35, Frédéric Yhuel via GNU coreutils General Discussion wrote:

Hi,

dd doesn't error out if posix_fadvise() returns an error. Here is a
patch for the fix.

fadvise64() always returns -1 on error, but not posix_fadvise().

Good catch.

The main issue of processing non seekable files was
already diagnosed with other operations related to the "nocache" feature,
but this may diagnose other edge cases.

I've adjusted your patch (attached) to
simplify a bit, always set errno, and add a NEWS entry.

I'll push this later.

thanks!
Pádraig
From 2c7f4ff070a042019ac7654081a486114c8f1c74 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Yhuel?= <frederic.yh...@dalibo.com>
Date: Sun, 9 Mar 2025 19:12:14 +0000
Subject: [PATCH] dd: fix error detection with "nocache" flag

* NEWS: Mention the bug fix.
* src/dd.c (invalidate_cache): Adjust to the unusual
error propagation sematics of posix_fadvise().
---
 NEWS     | 4 ++++
 src/dd.c | 6 ++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index 8d82bc352..902bdaf0e 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   'cksum -a crc' misbehaved on aarch64 with 32-bit uint_fast32_t.
   [bug introduced in coreutils-9.6]
 
+  dd with the 'nocache' flag will now detect all failures to drop the
+  cache for the whole file.  Previously it may have erroneously succeeded.
+  [bug introduced with the "nocache" feature in coreutils-8.11]
+
   'ls -Z dir' would crash.
   [bug introduced in coreutils-9.6]
 
diff --git a/src/dd.c b/src/dd.c
index 4e914336b..d54910516 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -1027,7 +1027,8 @@ cache_round (int fd, off_t len)
 
 /* Discard the cache from the current offset of either
    STDIN_FILENO or STDOUT_FILENO.
-   Return true on success.  */
+   Return true on success.
+   Return false on failure, with errno set.  */
 
 static bool
 invalidate_cache (int fd, off_t len)
@@ -1087,12 +1088,13 @@ invalidate_cache (int fd, off_t len)
      if (clen == 0)
        offset -= offset % page_size;
      adv_ret = posix_fadvise (fd, offset, clen, POSIX_FADV_DONTNEED);
+     errno = adv_ret;
 #else
      errno = ENOTSUP;
 #endif
    }
 
-  return adv_ret != -1 ? true : false;
+  return adv_ret == 0;
 }
 
 /* Read from FD into the buffer BUF of size SIZE, processing any
-- 
2.48.1