Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commit 85a1b79 breaks building Perl5 when using ksh93 as /bin/sh #467

Closed
atheik opened this issue Feb 18, 2022 · 4 comments
Closed

Commit 85a1b79 breaks building Perl5 when using ksh93 as /bin/sh #467

atheik opened this issue Feb 18, 2022 · 4 comments
Labels
blocker This had better be fixed before releasing bug Something is not working

Comments

@atheik
Copy link

atheik commented Feb 18, 2022

Commit 85a1b79 breaks building Perl5 when using ksh93 as /bin/sh

make[1]: Entering directory '/root/perl-5.34.0/cpan/Encode'
/bin/sh: cd: bad directory
make[1]: *** [Makefile:587: subdirs] Error 1

Line 586-587 of cpan/Encode/Makefile looks like this:

subdirs ::
        $(NOECHO) cd Byte && <snip>

Changing directory to cpan/Encode/Byte fails.

If you would like, you can reproduce this in Debian Live under x86_64 QEMU:

curl -LO https://cdimage.debian.org/cdimage/release/11.2.0-live/amd64/iso-hybrid/debian-live-11.2.0-amd64-standard.iso
qemu-system-x86_64 -cpu host -accel kvm -m 1024 -drive file=debian-live-11.2.0-amd64-standard.iso,media=cdrom

Run the following in Debian Live:

sudo -i

cd
curl -L https://codeload.github.com/ksh93/ksh/tar.gz/85a1b79 -o ksh-85a1b79.tar.gz
tar xf ksh-85a1b79.tar.gz
cd ksh-85a1b79
bin/package make
install -vm 755 arch/linux.i386-64/bin/ksh /bin/sh

cd
curl -LO https://www.cpan.org/src/5.0/perl-5.34.0.tar.xz
tar xf perl-5.34.0.tar.xz
cd perl-5.34.0
./Configure -des
make lib/auto/Encode/Encode.so

Sorry, I know this makes for a pretty poor reproducer but I do not know how to reduce it.
I would happily try any suggested patches.

@McDutchie McDutchie added the bug Something is not working label Feb 18, 2022
@McDutchie
Copy link

Yup, I done broke it. :-/

On Linux (but not macOS):

$ (unset PWD; arch/linux.i386-64/bin/ksh)
$ pwd
pwd: cannot determine present working directory
$ echo $PWD
.
$ ls -ld arch
total 4
drwxr-xr-x 8 martijn users 4096 Feb 18 20:53 arch
$ cd arch
arch/linux.i386-64/bin/ksh: cd: bad directory

The bug occurs when PWD is not inherited from the environment.

@McDutchie
Copy link

McDutchie commented Feb 19, 2022

I introduced a classic use after free problem, very obvious. Sorry about that.

Try this patch:

diff --git a/src/cmd/ksh93/sh/path.c b/src/cmd/ksh93/sh/path.c
index f106ffa2..9f7e1c17 100644
--- a/src/cmd/ksh93/sh/path.c
+++ b/src/cmd/ksh93/sh/path.c
@@ -183,6 +183,7 @@ static pid_t command_xargs(const char *path, char *argv[],char *const envp[], in
 char *path_pwd(void)
 {
 	register char *cp;
+	char tofree = 0;
 	Namval_t *pwdnod;
 	/* Don't bother if PWD already set */
 	if(sh.pwd)
@@ -198,7 +199,6 @@ char *path_pwd(void)
 	{
 		/* Check if $HOME is a path to the PWD; this ensures $PWD == $HOME
 		   at login, even if $HOME is a path that contains symlinks */
-		char tofree = 0;
 		cp = nv_getval(sh_scoped(HOME));
 		if(!(cp && *cp=='/' && test_inode(cp,e_dot)))
 		{
@@ -214,8 +214,6 @@ char *path_pwd(void)
 				pwdnod = sh_assignok(pwdnod,1);
 			nv_putval(pwdnod,cp,NV_RDONLY);
 		}
-		if(tofree)
-			free((void*)cp);
 	}
 	nv_onattr(pwdnod,NV_EXPORT);
 	/* Neither obtained the pwd nor can fall back to sane-ish $PWD: fall back to "." */
@@ -224,7 +222,9 @@ char *path_pwd(void)
 	if(!cp || *cp!='/')
 		nv_putval(pwdnod,cp=(char*)e_dot,NV_RDONLY);
 	/* Set shell PWD */
-	sh.pwd = sh_strdup(cp);
+	if(!tofree)
+		cp = sh_strdup(cp);
+	sh.pwd = cp;
 	return((char*)sh.pwd);
 }

@atheik
Copy link
Author

atheik commented Feb 19, 2022

I applied your patch on top of 85a1b79 and Perl5 builds fine now. I also ran bin/shtests but I guess you did that as well. Thank you!

@McDutchie McDutchie added 1.0 blocker This had better be fixed before releasing labels Feb 19, 2022
@McDutchie
Copy link

Thank you for finding and reporting the bug so promptly.

McDutchie added a commit that referenced this issue Feb 19, 2022
Of course, we should not free the 'cp' pointer when we still need
to use it.

Resolves: #467
Thanks to @atheik for the report.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker This had better be fixed before releasing bug Something is not working
Projects
None yet
Development

No branches or pull requests

2 participants