File descriptor leakage?

File descriptor leakage?

[lilydjwg]

Hi,

I discover that my zsh keeps a file descriptor to a deleted
".histfile.new" file:

    3  REG       0x28     326859    1993633 /home/lilydjwg/.histfile.new (deleted)

And it's inherited in some of its child processes. Does zsh forget to
close it?

My zsh version is 5.0.6, and I do not see this with 5.0.5.

-- 
Best regards,
lilydjwg

Re: File descriptor leakage?

[Bart Schaefer]

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.

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.

Re: File descriptor leakage?

[lilydjwg]

On Sun, Sep 28, 2014 at 11:07:10PM -0700, Bart Schaefer wrote:
> [...]
> 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.
> [...]

Hi, I did a git bisect and found this commit is the first bad one:
979f72199f3d3e98b1f6c712fa26f731dac2cb51

But during the test, it's slightly different: the leaking fd is refering
to a file named ".histfile" and is not deleted.

-- 
Best regards,
lilydjwg

Re: File descriptor leakage?

[Peter Stephenson]

On Sun, 28 Sep 2014 23:07:10 -0700
Bart Schaefer <schaefer@brasslantern.com> 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@gmail.com> 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

Re: File descriptor leakage?

[lilydjwg]

On Mon, Sep 29, 2014 at 11:59:01AM +0100, Peter Stephenson wrote:
> [...]
>
> 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
>

Hi, I'm sorry I didn't test master. It turns out that this has already
been fix at commit 4414e54ea7ffe50acca851c11c2ef49dc867c55d. And the
patch above doesn't fix it for me (patched against that bad commit).

-- 
Best regards,
lilydjwg

Re: File descriptor leakage?

[Peter Stephenson]

On Mon, 29 Sep 2014 21:58:51 +0800
lilydjwg <lilydjwg@gmail.com> wrote:
> Hi, I'm sorry I didn't test master. It turns out that this has already
> been fix at commit 4414e54ea7ffe50acca851c11c2ef49dc867c55d

Great.

> And the patch above doesn't fix it for me (patched against that bad commit).

I wouldn't expect it to, since there's no sign anything was actually
failing apart from the leak; it's just tidying up another hole.

pws