'cd' built-in crashed zsh on a broken file system

'cd' built-in crashed zsh on a broken file system

[Kamil Dudka]

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

Re: 'cd' built-in crashed zsh on a broken file system

[Bart Schaefer]

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().

Re: 'cd' built-in crashed zsh on a broken file system

[Peter Stephenson]

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

Re: 'cd' built-in crashed zsh on a broken file system

[Bart Schaefer]

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.

Re: 'cd' built-in crashed zsh on a broken file system

[Kamil Dudka]

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

Re: 'cd' built-in crashed zsh on a broken file system

[Mikael Magnusson]

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

Re: 'cd' built-in crashed zsh on a broken file system

[Mikael Magnusson]

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

Re: 'cd' built-in crashed zsh on a broken file system

[Bart Schaefer]

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.

Re: 'cd' built-in crashed zsh on a broken file system

[Peter Stephenson]

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