Re: [PATCH] truncate: don't leak a file descriptor with --ref=PIPE

[Jim Meyering]

Pádraig Brady wrote:
> On 08/04/2012 10:06 AM, Jim Meyering wrote:
...
>> Subject: [PATCH] truncate: don't leak a file descriptor with --ref=PIPE
>>
>> * src/truncate.c (main): For a user who makes the mistake of
>> using a non-seekable file as a reference for the desired length,
>> truncate would open that file, attempt to seek to its end, but
>> upon seek failure would neglect to close the file descriptor.
>> Reverse conjuncts, so the close is unconditional.
>> Spotted by coverity.
>
> Cool.
> Not worth a news entry because we exit() right after
> (as file_size will be left at -1 in this case).
>
> Hmm, but I just noticed that close() may clear
> the errno of an lseek() failure, so we'd get
> the classic "Error: success" in this case?

Good catch.
That close call *could* technically clobber errno.
For the record, on linux it does not clear it upon success:

    $ mkfifo F && ./truncate --ref=F k & echo > F
    ./truncate: cannot get the size of 'F': Illegal seek

Hmm...
do we really care if the close fails for a read-only FD
for which our lseek (the important call) succeeded?

I don't think so.
How about this improved patch:

>From c1d56ce2beb1bf0f2e8232db5a3673cf95a87eb4 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 4 Aug 2012 11:02:40 +0200
Subject: [PATCH] truncate: don't leak a file descriptor with --ref=PIPE
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/truncate.c (main): For a user who makes the mistake of
using a non-seekable file as a reference for the desired length,
truncate would open that file, attempt to seek to its end, but
upon seek failure would neglect to close the file descriptor.
Close the file descriptor even when lseek fails.
In addition, ignore failure to close that reference FD, since as
long as the lseek succeeds, a close failure doesn't matter.
Coverity spotted the potential FD leak.

Improved-by: Pádraig Brady.
---
 src/truncate.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/truncate.c b/src/truncate.c
index c1e9666..d638993 100644
--- a/src/truncate.c
+++ b/src/truncate.c
@@ -370,8 +370,15 @@ main (int argc, char **argv)
           if (0 <= ref_fd)
             {
               off_t file_end = lseek (ref_fd, 0, SEEK_END);
-              if (0 <= file_end && close (ref_fd) == 0)
+              int saved_errno = errno;
+              close (ref_fd); /* ignore failure */
+              if (0 <= file_end)
                 file_size = file_end;
+              else
+                {
+                  /* restore, in case close clobbered it. */
+                  errno = saved_errno;
+                }
             }
         }
       if (file_size < 0)
--
1.7.12.rc1.10.g97c7934