Re: File descriptor leakage?

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Sun, 28 Sep 2014 23:07:10 -0700
Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
> On Sep 29, 11:53am, lilydjwg wrote:
> }
> } I discover that my zsh keeps a file descriptor to a deleted
> } ".histfile.new" file:
> 
> This is the file created when the HIST_SAVE_BY_COPY option is set, which
> is then renamed to the name given by $HISTFILE after being written.  I
> don't know of any reason the behavior for this would have changed from
> 5.0.5 to 5.0.6, but it's possible that workers/33116 is related.

The option is on by default, so a lot of people ought to be seeing
this.  It looks like either it's system-specific or maybe to do with
some hairy history option combination --- there are plenty of those,
though it's not obvious why they'd have an effect here.

> There is an assumption that
> 
>   fd = open(file, O_WRONLY, 0600);
>   out = fdopen(fd, "w");
>   fclose(out);
> 
> will close fd, but that assumption has been there for as long as the
> HIST_SAVE_BY_COPY option has existed.

Fairly easy to add a "close(fileno(out));" and see if it goes away.
That would be after the fclose() currently at line 2682 of Src/hist.c.

I note we don't handle the case where the open() succeeded but the
fdopen() failed in this case (around line 2595) unlike other cases
(around 2822 and 2876).  This would probably be good practice anyway.

lilydjwg <lilydjwg@xxxxxxxxx> wrote:
> Hi, I did a git bisect and found this commit is the first bad one:
> 979f72199f3d3e98b1f6c712fa26f731dac2cb51

(...finds out about "git show" again...)

Hmm, that's to do with file locking.  That might suggest some path
through the code where this isn't handled that neither Bart nor I have
spotted yet.

diff --git a/Src/hist.c b/Src/hist.c
index d29a65a..4660fd0 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -2593,7 +2593,12 @@ savehistfile(char *fn, int err, int writeflags)
 		out = NULL;
 	    } else {
 		int fd = open(tmpfile, O_CREAT | O_WRONLY | O_EXCL, 0600);
-		out = fd >= 0 ? fdopen(fd, "w") : NULL;
+		if (fd >=0) {
+		    out = fdopen(fd, "w");
+		    if (!out)
+			close(fd);
+		} else
+		    out = NULL;
 	    }
 
 #ifdef HAVE_FCHMOD

pws