* File descriptor leakage? @ 2014-09-29 3:53 lilydjwg 2014-09-29 6:07 ` Bart Schaefer 0 siblings, 1 reply; 6+ messages in thread From: lilydjwg @ 2014-09-29 3:53 UTC (permalink / raw) To: zsh-users 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: File descriptor leakage? 2014-09-29 3:53 File descriptor leakage? lilydjwg @ 2014-09-29 6:07 ` Bart Schaefer 2014-09-29 10:47 ` lilydjwg 2014-09-29 10:59 ` Peter Stephenson 0 siblings, 2 replies; 6+ messages in thread From: Bart Schaefer @ 2014-09-29 6:07 UTC (permalink / raw) To: zsh-users; +Cc: lilydjwg 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: File descriptor leakage? 2014-09-29 6:07 ` Bart Schaefer @ 2014-09-29 10:47 ` lilydjwg 2014-09-29 10:59 ` Peter Stephenson 1 sibling, 0 replies; 6+ messages in thread From: lilydjwg @ 2014-09-29 10:47 UTC (permalink / raw) To: zsh-users; +Cc: Bart Schaefer 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: File descriptor leakage? 2014-09-29 6:07 ` Bart Schaefer 2014-09-29 10:47 ` lilydjwg @ 2014-09-29 10:59 ` Peter Stephenson 2014-09-29 13:58 ` lilydjwg 1 sibling, 1 reply; 6+ messages in thread From: Peter Stephenson @ 2014-09-29 10:59 UTC (permalink / raw) To: zsh-users 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: File descriptor leakage? 2014-09-29 10:59 ` Peter Stephenson @ 2014-09-29 13:58 ` lilydjwg 2014-09-29 14:03 ` Peter Stephenson 0 siblings, 1 reply; 6+ messages in thread From: lilydjwg @ 2014-09-29 13:58 UTC (permalink / raw) To: zsh-users; +Cc: Peter Stephenson 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: File descriptor leakage? 2014-09-29 13:58 ` lilydjwg @ 2014-09-29 14:03 ` Peter Stephenson 0 siblings, 0 replies; 6+ messages in thread From: Peter Stephenson @ 2014-09-29 14:03 UTC (permalink / raw) To: zsh-users 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-29 14:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-29 3:53 File descriptor leakage? lilydjwg 2014-09-29 6:07 ` Bart Schaefer 2014-09-29 10:47 ` lilydjwg 2014-09-29 10:59 ` Peter Stephenson 2014-09-29 13:58 ` lilydjwg 2014-09-29 14:03 ` Peter Stephenson
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).