* 'cd' built-in crashed zsh on a broken file system @ 2015-01-20 12:35 Kamil Dudka 2015-01-20 18:28 ` Bart Schaefer 0 siblings, 1 reply; 9+ messages in thread From: Kamil Dudka @ 2015-01-20 12:35 UTC (permalink / raw) To: zsh-workers The following bug report describes a crash of the 'cd' built-in of zsh: https://bugzilla.redhat.com/1183238 The root cause was probably an inconsistent state of the sshfs file system after resume from suspend-to-ram. However, the segmentation fault suggests there might be some insufficient error handling in zsh code. unmeta() was called with NULL as pwd at the following line: http://sourceforge.net/p/zsh/code/ci/638bfa93/tree/Src/builtin.c#l807 It is not clear to me why pwd was NULL after the return from cd_new_pwd() and I was not able to trigger the bug myself to debug it further. The full backtrace is available here: https://bugzilla.redhat.com/attachment.cgi?id=981010 Kamil ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 'cd' built-in crashed zsh on a broken file system 2015-01-20 12:35 'cd' built-in crashed zsh on a broken file system Kamil Dudka @ 2015-01-20 18:28 ` Bart Schaefer 2015-01-20 20:34 ` Peter Stephenson 0 siblings, 1 reply; 9+ messages in thread From: Bart Schaefer @ 2015-01-20 18:28 UTC (permalink / raw) To: Kamil Dudka, zsh-workers On Jan 20, 1:35pm, Kamil Dudka wrote: } Subject: 'cd' built-in crashed zsh on a broken file system } } It is not clear to me why pwd was NULL after the return from cd_new_pwd() xsymlink() in utils.c: if (*s != '/') return NULL; called from findpwd() also utils.c, which in turn is only called if you have CHASE_LINKS set. I'm not sure if the right thing to do is test the return of xsymlink() down in findpwd(), or to check the return of findpwd() in cd_new_pwd(). ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 'cd' built-in crashed zsh on a broken file system 2015-01-20 18:28 ` Bart Schaefer @ 2015-01-20 20:34 ` Peter Stephenson 2015-01-20 21:57 ` Bart Schaefer ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Peter Stephenson @ 2015-01-20 20:34 UTC (permalink / raw) To: zsh-workers On Tue, 20 Jan 2015 10:28:10 -0800 Bart Schaefer <schaefer@brasslantern.com> wrote: > On Jan 20, 1:35pm, Kamil Dudka wrote: > } Subject: 'cd' built-in crashed zsh on a broken file system > } > } It is not clear to me why pwd was NULL after the return from cd_new_pwd() > > xsymlink() in utils.c: > > if (*s != '/') > return NULL; > > called from findpwd() also utils.c, which in turn is only called if you > have CHASE_LINKS set. > > I'm not sure if the right thing to do is test the return of xsymlink() > down in findpwd(), or to check the return of findpwd() in cd_new_pwd(). I'd say the latter: dealing with an I'm-sorry-dave-I-can't-do-that is a bit neater higher up otherwise the intermediate return values are in some sort of limbo. I don't think there's any point in printing an error message: we don't know what happened, so just pretend it didn't. How about this? diff --git a/Src/builtin.c b/Src/builtin.c index 1818941..08be1ac 100644 --- a/Src/builtin.c +++ b/Src/builtin.c @@ -1156,9 +1156,11 @@ cd_new_pwd(int func, LinkNode dir, int quiet) zsfree(getlinknode(dirstack)); if (chasinglinks) { - s = new_pwd; - new_pwd = findpwd(s); - zsfree(s); + s = findpwd(new_pwd); + if (s) { + zsfree(new_pwd); + new_pwd = s; + } } if (isset(PUSHDIGNOREDUPS)) { LinkNode n; diff --git a/Src/utils.c b/Src/utils.c index 4561b5e..cf18f12 100644 --- a/Src/utils.c +++ b/Src/utils.c @@ -1108,10 +1108,13 @@ getnameddir(char *name) if ((pw = getpwnam(name))) { char *dir = isset(CHASELINKS) ? xsymlink(pw->pw_dir) : ztrdup(pw->pw_dir); - adduserdir(name, dir, ND_USERNAME, 1); - str = dupstring(dir); - zsfree(dir); - return str; + if (dir) { + adduserdir(name, dir, ND_USERNAME, 1); + str = dupstring(dir); + zsfree(dir); + return str; + } else + return ztrdup(pw->pw_dir); } } #endif /* HAVE_GETPWNAM */ pws ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 'cd' built-in crashed zsh on a broken file system 2015-01-20 20:34 ` Peter Stephenson @ 2015-01-20 21:57 ` Bart Schaefer 2015-01-23 13:31 ` Kamil Dudka 2015-01-24 19:09 ` Mikael Magnusson 2 siblings, 0 replies; 9+ messages in thread From: Bart Schaefer @ 2015-01-20 21:57 UTC (permalink / raw) To: zsh-workers On Jan 20, 8:34pm, Peter Stephenson wrote: } } How about this? Looks fine, though if the home directory from the password entry does not beghin with a '/' (second hunk of the patch) there's probably a lot more broken than this. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 'cd' built-in crashed zsh on a broken file system 2015-01-20 20:34 ` Peter Stephenson 2015-01-20 21:57 ` Bart Schaefer @ 2015-01-23 13:31 ` Kamil Dudka 2015-01-24 19:09 ` Mikael Magnusson 2 siblings, 0 replies; 9+ messages in thread From: Kamil Dudka @ 2015-01-23 13:31 UTC (permalink / raw) To: Peter Stephenson; +Cc: zsh-workers On Tuesday 20 January 2015 20:34:36 Peter Stephenson wrote: > On Tue, 20 Jan 2015 10:28:10 -0800 > > Bart Schaefer <schaefer@brasslantern.com> wrote: > > On Jan 20, 1:35pm, Kamil Dudka wrote: > > } Subject: 'cd' built-in crashed zsh on a broken file system > > } > > } It is not clear to me why pwd was NULL after the return from > > cd_new_pwd() > > > > xsymlink() in utils.c: > > if (*s != '/') > > > > return NULL; > > > > called from findpwd() also utils.c, which in turn is only called if you > > have CHASE_LINKS set. > > > > I'm not sure if the right thing to do is test the return of xsymlink() > > down in findpwd(), or to check the return of findpwd() in cd_new_pwd(). > > I'd say the latter: dealing with an I'm-sorry-dave-I-can't-do-that is a > bit neater higher up otherwise the intermediate return values are in > some sort of limbo. > > I don't think there's any point in printing an error message: we don't > know what happened, so just pretend it didn't. > > How about this? Thanks for the patch! I have applied it on Fedora packages, will let you know if the problem occurs from now on. Kamil ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 'cd' built-in crashed zsh on a broken file system 2015-01-20 20:34 ` Peter Stephenson 2015-01-20 21:57 ` Bart Schaefer 2015-01-23 13:31 ` Kamil Dudka @ 2015-01-24 19:09 ` Mikael Magnusson 2015-01-24 21:56 ` Mikael Magnusson ` (2 more replies) 2 siblings, 3 replies; 9+ messages in thread From: Mikael Magnusson @ 2015-01-24 19:09 UTC (permalink / raw) To: Peter Stephenson; +Cc: zsh workers On Tue, Jan 20, 2015 at 9:34 PM, Peter Stephenson <p.w.stephenson@ntlworld.com> wrote: > > How about this? > > diff --git a/Src/utils.c b/Src/utils.c > index 4561b5e..cf18f12 100644 > --- a/Src/utils.c > +++ b/Src/utils.c > @@ -1108,10 +1108,13 @@ getnameddir(char *name) > if ((pw = getpwnam(name))) { > char *dir = isset(CHASELINKS) ? xsymlink(pw->pw_dir) > : ztrdup(pw->pw_dir); > - adduserdir(name, dir, ND_USERNAME, 1); > - str = dupstring(dir); > - zsfree(dir); > - return str; > + if (dir) { > + adduserdir(name, dir, ND_USERNAME, 1); > + str = dupstring(dir); > + zsfree(dir); > + return str; > + } else > + return ztrdup(pw->pw_dir); This ztrdup triggered a couple of errors in valgrind. Since everything else returned from getnameddir seems to be heap allocated, should this be dupstring instead? -- Mikael Magnusson ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 'cd' built-in crashed zsh on a broken file system 2015-01-24 19:09 ` Mikael Magnusson @ 2015-01-24 21:56 ` Mikael Magnusson 2015-01-25 3:54 ` Bart Schaefer 2015-01-25 17:54 ` Peter Stephenson 2 siblings, 0 replies; 9+ messages in thread From: Mikael Magnusson @ 2015-01-24 21:56 UTC (permalink / raw) To: Peter Stephenson; +Cc: zsh workers On Sat, Jan 24, 2015 at 8:09 PM, Mikael Magnusson <mikachu@gmail.com> wrote: > On Tue, Jan 20, 2015 at 9:34 PM, Peter Stephenson > <p.w.stephenson@ntlworld.com> wrote: >> >> How about this? >> >> diff --git a/Src/utils.c b/Src/utils.c >> index 4561b5e..cf18f12 100644 >> --- a/Src/utils.c >> +++ b/Src/utils.c >> @@ -1108,10 +1108,13 @@ getnameddir(char *name) >> if ((pw = getpwnam(name))) { >> char *dir = isset(CHASELINKS) ? xsymlink(pw->pw_dir) >> : ztrdup(pw->pw_dir); >> - adduserdir(name, dir, ND_USERNAME, 1); >> - str = dupstring(dir); >> - zsfree(dir); >> - return str; >> + if (dir) { >> + adduserdir(name, dir, ND_USERNAME, 1); >> + str = dupstring(dir); >> + zsfree(dir); >> + return str; >> + } else >> + return ztrdup(pw->pw_dir); > > This ztrdup triggered a couple of errors in valgrind. Since everything > else returned from getnameddir seems to be heap allocated, should this > be dupstring instead? At this point, just assume I mean coverity if I ever mention valgrind. I don't know why this happens... -- Mikael Magnusson ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 'cd' built-in crashed zsh on a broken file system 2015-01-24 19:09 ` Mikael Magnusson 2015-01-24 21:56 ` Mikael Magnusson @ 2015-01-25 3:54 ` Bart Schaefer 2015-01-25 17:54 ` Peter Stephenson 2 siblings, 0 replies; 9+ messages in thread From: Bart Schaefer @ 2015-01-25 3:54 UTC (permalink / raw) To: zsh workers On Jan 24, 8:09pm, Mikael Magnusson wrote: } } This ztrdup triggered a couple of errors in valgrind. Since everything } else returned from getnameddir seems to be heap allocated, should this } be dupstring instead? It should almost certainly match the immediately preceding return in the other branch of the "if (dir)", yes. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 'cd' built-in crashed zsh on a broken file system 2015-01-24 19:09 ` Mikael Magnusson 2015-01-24 21:56 ` Mikael Magnusson 2015-01-25 3:54 ` Bart Schaefer @ 2015-01-25 17:54 ` Peter Stephenson 2 siblings, 0 replies; 9+ messages in thread From: Peter Stephenson @ 2015-01-25 17:54 UTC (permalink / raw) To: zsh workers On Sat, 24 Jan 2015 20:09:14 +0100 Mikael Magnusson <mikachu@gmail.com> wrote: > This ztrdup triggered a couple of errors in valgrind. Since everything > else returned from getnameddir seems to be heap allocated, should this > be dupstring instead? Yes, it was mimicking the ztrdup() further up but that gets explicitly freed in the other branch. diff --git a/Src/utils.c b/Src/utils.c index cf18f12..0490df5 100644 --- a/Src/utils.c +++ b/Src/utils.c @@ -1114,7 +1114,7 @@ getnameddir(char *name) zsfree(dir); return str; } else - return ztrdup(pw->pw_dir); + return dupstring(pw->pw_dir); } } #endif /* HAVE_GETPWNAM */ pws ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-01-25 18:00 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-20 12:35 'cd' built-in crashed zsh on a broken file system Kamil Dudka 2015-01-20 18:28 ` Bart Schaefer 2015-01-20 20:34 ` Peter Stephenson 2015-01-20 21:57 ` Bart Schaefer 2015-01-23 13:31 ` Kamil Dudka 2015-01-24 19:09 ` Mikael Magnusson 2015-01-24 21:56 ` Mikael Magnusson 2015-01-25 3:54 ` Bart Schaefer 2015-01-25 17:54 ` 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).