Skip to content

Bug-877: Memory fault when attempting to run /dev/fd/1 as a script #879

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

phidebian
Copy link

ksh /dev/fd/1 do core dump while ksh /dev/stdout don't

Based on this I remove in the itnt() path useless code base on filename match /dev/fd/ as there is no reasons to make special case for this.

Note the ksh /dev/fd/0 or /dev/fd/5 do not core dump and so chasing a proper fix for the specific case of /dev/fd/1 may be another option, but I think the excess code removal could be better.

ksh /dev/fd/1 do core dump while ksh /dev/stdout don't

Based on this I remove in the itnt() path useless code base on
filename match /dev/fd/ as there is no reasons to make special case
for this.

Note the ksh /dev/fd/0 or /dev/fd/5 do not core dump and so chasing a
proper fix for the specific case of /dev/fd/1 may be another option,
but I think the excess code removal could be better.
@phidebian
Copy link
Author

After more reading the exfile()

        if(!iop)
        {
                if(fno > 0)
                {
                        if(fno < 10)
                        {
                                int r = sh_fcntl(fno,F_dupfd_cloexec,10);
                                if(r >= 10)
                                {
                                        if(F_dupfd_cloexec != F_DUPFD)
                                                sh.fdstatus[r] = sh.fdstatus[fno]|IOCLEX;
                                        else
                                                sh.fdstatus[r] = sh.fdstatus[fno];
                                        sh_close(fno);
                                        fno = r;
                                }
                        }
                        if(!(sh.fdstatus[fno]&IOCLEX))
                                sh_fcntl(fno,F_SETFD,FD_CLOEXEC);
                        iop = sh_iostream(fno);
                }
                else
                        iop = sfstdin;

I don't get the idea why if a given argv[] script name is /dev/fd/NN, then this fd should be closed, while when the argv[] script name is "/dev/stdout" it would not close the FD=1.
The is no FD leak.
There is no secu reasons
SO the proposal to remove it.

@JohnoKing
Copy link

JohnoKing commented Jun 18, 2025

I don't get the idea why if a given argv[] script name is /dev/fd/NN, then this fd should be closed, while when the argv[] script name is "/dev/stdout" it would not close the FD=1.

For fds < 3 it's definitely wrong to close the file descriptor. For fds >= 3 I believe ksh closes it because ksh generally wants the range of 3-10 to start off closed when running a script, since those are usually reserved for usage in scripts utilizing redirections IIRC. In ksh93v- sh_iomovefd() was altered to dup fds above ten for that purpose, which I previously tried to backport. It caused a regression test to fail, and since I was focused on something else (FD_CLOEXEC) I opted out of backporting it for the time being. (In hindsight, it probably isn't the 93v- code that's wrong, but the regression test itself. I'll probably revisit that at some point in the future.)

In any case changing the code to avoid closing fds 0-2 fixes the crash for me. Apparently that part of the patch doesn't fix the crash, but rather the change to the if statement. Patch (edit:edit again: updated again for better close-on-exec in light of fdin<=2 getting moved now):

Patch
diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c
index 05aa7b34e..bd3294de3 100644
--- a/src/cmd/ksh93/sh/init.c
+++ b/src/cmd/ksh93/sh/init.c
@@ -1347,10 +1347,7 @@ Shell_t *sh_init(int argc,char *argv[], Shinit_f userinit)
 	else
 		sh_offoption(SH_PRIVILEGED);
 	/* shname for $0 in profiles and . scripts */
-	if(sh_isdevfd(argv[1]))
-		sh.shname = sh_strdup(argv[0]);
-	else
-		sh.shname = sh_strdup(sh.st.dolv[0]);
+	sh.shname = sh_strdup(sh.st.dolv[0]);
 	/*
 	 * return here for shell script execution
 	 * but not for parenthesis subshells
diff --git a/src/cmd/ksh93/sh/main.c b/src/cmd/ksh93/sh/main.c
index ea8f5d92f..222bb924d 100644
--- a/src/cmd/ksh93/sh/main.c
+++ b/src/cmd/ksh93/sh/main.c
@@ -218,11 +218,9 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 				fdin = 0;
 			else
 			{
-				char *sp;
 				/* open stream should have been passed into shell */
-				if(strmatch(name,e_devfdNN))
+				if(strmatch(name,e_devfdNN) && (fdin=(int)strtol(name+8, NULL, 10)) != 1)
 				{
-					fdin = (int)strtol(name+8, NULL, 10);
 					if(fstat(fdin,&statb)<0)
 					{
 						errormsg(SH_DICT,ERROR_system(1),e_open,name);
@@ -234,10 +232,11 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 				}
 				else
 				{
+					char *sp;
 					int isdir = 0;
-					if((fdin=sh_open(name,O_RDONLY,0))>=0 &&(fstat(fdin,&statb)<0 || S_ISDIR(statb.st_mode)))
+					if((fdin=sh_open(name,O_RDONLY|O_cloexec,0))>=0 &&(fstat(fdin,&statb)<0 || S_ISDIR(statb.st_mode)))
 					{
-						close(fdin);
+						sh_close(fdin);
 						isdir = 1;
 						fdin = -1;
 					}
@@ -250,7 +249,7 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 							sp = stkptr(sh.stk,PATH_OFFSET);
 						if(sp)
 						{
-							if((fdin=sh_open(sp,O_RDONLY,0))>=0)
+							if((fdin=sh_open(sp,O_RDONLY|O_cloexec,0))>=0)
 								sh.st.filename = path_fullname(sp);
 						}
 					}
@@ -272,7 +271,13 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 							strcopy(name," \"$@\"");
 						goto shell_c;
 					}
-					if(fdin==0)
+					/*
+					 * Note: fdin could wind up being zero or some such if the
+					 *       shell was opened with stdin/stdout/stderr closed
+					 *       (i.e. 0>&-), so we move the fd in such a case to
+					 *       keep that file descriptor closed.
+					 */
+					if(fdin<=2)
 						fdin = sh_iomovefd(fdin);
 				}
 				sh.readscript = sh.shname;
@@ -356,7 +361,8 @@ static void	exfile(Sfio_t *iop,int fno)
 						sh.fdstatus[r] = sh.fdstatus[fno]|IOCLEX;
 					else
 						sh.fdstatus[r] = sh.fdstatus[fno];
-					sh_close(fno);
+					if(fno>2)
+						sh_close(fno);
 					fno = r;
 				}
 			}

@phidebian
Copy link
Author

Yes at first I thought fd 1,2,3 should not undergo the close, then later I generalised, I think there is no reason to close any given fd in the runstring, the iop for the script should not be related with FD's even if they are common, For instance with my fix I got

$ more /tmp/foo
echo ==================
sleep 1111111111111

$ /d1/ksh-phideb/arch/linux.i386-64/bin/ksh 23</tmp/foo /dev/fd/23

$ ll /proc/17697/fd
total 0
lrwx------ 1 phi phi 64 Jun 18 22:13 0 -> /dev/pts/1
lrwx------ 1 phi phi 64 Jun 18 22:13 1 -> /dev/pts/1
l--------- 1 phi phi 64 Jun 18 22:13 10 -> /d1/ksh-phideb
lr-x------ 1 phi phi 64 Jun 18 22:13 11 -> /tmp/foo
lrwx------ 1 phi phi 64 Jun 18 22:13 2 -> /dev/pts/1
lr-x------ 1 phi phi 64 Jun 18 22:13 23 -> /tmp/foo

I don't think script are run with fd 0,1 2 are closed when a ksh script is started it inherit what the 'caller' have left open, here in this example bash (the caller) did pre-open fd23 and the script run with it.

The runs string argv[] got /dev/fd/23, this is depicted by FD11 -> /tmp/foo and FD23 is what we inherited.

I didn't run your fix, is there a commit link link I should usdd (sorry my git level is low)

I think with your fix if only 0,1,2 are exempt from close I would get

lrwx------ 1 phi phi 64 Jun 18 22:13 0 -> /dev/pts/1
lrwx------ 1 phi phi 64 Jun 18 22:13 1 -> /dev/pts/1
l--------- 1 phi phi 64 Jun 18 22:13 10 -> /d1/ksh-phideb
lr-x------ 1 phi phi 64 Jun 18 22:13 11 -> /tmp/foo
lrwx------ 1 phi phi 64 Jun 18 22:13 2 -> /dev/pts/1

I.e messing FD23 while ledgitimately IO should be available here per 23</tmp/foo

I will think again...

@phidebian
Copy link
Author

phidebian commented Jun 19, 2025

I looked your fix,

The first part

-	if(sh_isdevfd(argv[1]))
-		sh.shname = sh_strdup(argv[0]);
-	else
-		sh.shname = sh_strdup(sh.st.dolv[0]);

is the same as mine, I precise here that my fix is just a candidate, so instead of removing the line I enclose then if #if 0 ... #endif to ease the code review, the final cut indeed we got the cleanup code.

Ditto for my second part I did

        /* bug-877: Phi: No need for this */
	if( 0 && strmatch(name,e_devfdNN))

The if(0&& is just code elimination for the code review that need cleanup if accepted.

Now regarding cloexec, the virtue of my fix is that I do nothing on FD's, if an FD was open by caller (any FD's <10 and >=10) they remain open for our child fork/exec unless explicitly manipulated by the script. In my previous expample

$ /d1/ksh-phideb/arch/linux.i386-64/bin/ksh 5</tmp/foo /dev/fd/5

FD5 ought to be still open for the childs, as bash do.

Thats' why I removed all processing on FD's regarding the script found in argv[]. Ths way the script name found in argv[] even if it is /dev/fd/NN get its own FD (open) and this one is naturally cloexec.

To empasis this I modified the example adding a child

$ cat /tmp/foo
echo ======== $0 $$ =======
bash /tmp/bar
sleep 111111111111

$ cat /tmp/bar
echo ======== $0 $$ =======
sleep 111111111111

$ function p
{ ls -l /proc/$1/fd
}

/d1/ksh-phideb/arch/linux.i386-64/bin/ksh   5</tmp/foo /dev/fd/5
======== /dev/fd/5 10510 =======
======== /tmp/bar 10511 =======

$ p 10510
total 0
lrwx------ 1 phi phi 64 Jun 19 07:11 0 -> /dev/pts/2
lrwx------ 1 phi phi 64 Jun 19 07:11 1 -> /dev/pts/2
l--------- 1 phi phi 64 Jun 19 07:11 10 -> /home/phi
lr-x------ 1 phi phi 64 Jun 19 07:11 11 -> /tmp/foo
lrwx------ 1 phi phi 64 Jun 19 07:11 2 -> /dev/pts/2
lr-x------ 1 phi phi 64 Jun 19 07:11 5 -> /tmp/foo

$ p 10511
total 0
lrwx------ 1 phi phi 64 Jun 19 07:11 0 -> /dev/pts/2
lrwx------ 1 phi phi 64 Jun 19 07:11 1 -> /dev/pts/2
lrwx------ 1 phi phi 64 Jun 19 07:11 2 -> /dev/pts/2
lr-x------ 1 phi phi 64 Jun 19 07:11 255 -> /tmp/bar
lr-x------ 1 phi phi 64 Jun 19 07:11 5 -> /tmp/foo

As observed here FD5 is not closed neither for our ksh and for its child.

Using bash for outter level we got

$ bash   5</tmp/foo /dev/fd/5
======== /dev/fd/5 10775 =======
======== /tmp/bar 10776 =======

$ p 10775
total 0
lrwx------ 1 phi phi 64 Jun 19 07:17 0 -> /dev/pts/2
lrwx------ 1 phi phi 64 Jun 19 07:17 1 -> /dev/pts/2
lrwx------ 1 phi phi 64 Jun 19 07:17 2 -> /dev/pts/2
lr-x------ 1 phi phi 64 Jun 19 07:17 255 -> /tmp/foo
lr-x------ 1 phi phi 64 Jun 19 07:16 5 -> /tmp/foo
 
$ p 10776
total 0
lrwx------ 1 phi phi 64 Jun 19 07:17 0 -> /dev/pts/2
lrwx------ 1 phi phi 64 Jun 19 07:17 1 -> /dev/pts/2
lrwx------ 1 phi phi 64 Jun 19 07:17 2 -> /dev/pts/2
lr-x------ 1 phi phi 64 Jun 19 07:17 255 -> /tmp/bar
lr-x------ 1 phi phi 64 Jun 19 07:17 5 -> /tmp/foo

We got a similar behavior, ksh use FD10 for the PWD (dunno why) anf FD11 for the script name where bash use FD255 for this. We note the our FD10 FD11 are cloexec and FD5 is preserved.

As long as your fix respect that I'll be happy with it.

@JohnoKing
Copy link

JohnoKing commented Jun 19, 2025

FD5 ought to be still open for the childs, as bash do.

Yeah, ksh shouldn't differ from bash here as regards behavior. The file descriptor for the /dev/fd script should not be closed nor should it be marked with close-on-exec.

New better patch which avoids closing inherited /dev/fd file descriptors

RETRACTED Patch
diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c
index 05aa7b34e..bd3294de3 100644
--- a/src/cmd/ksh93/sh/init.c
+++ b/src/cmd/ksh93/sh/init.c
@@ -1347,10 +1347,7 @@ Shell_t *sh_init(int argc,char *argv[], Shinit_f userinit)
 	else
 		sh_offoption(SH_PRIVILEGED);
 	/* shname for $0 in profiles and . scripts */
-	if(sh_isdevfd(argv[1]))
-		sh.shname = sh_strdup(argv[0]);
-	else
-		sh.shname = sh_strdup(sh.st.dolv[0]);
+	sh.shname = sh_strdup(sh.st.dolv[0]);
 	/*
 	 * return here for shell script execution
 	 * but not for parenthesis subshells
diff --git a/src/cmd/ksh93/sh/main.c b/src/cmd/ksh93/sh/main.c
index ea8f5d92f..1930d4d10 100644
--- a/src/cmd/ksh93/sh/main.c
+++ b/src/cmd/ksh93/sh/main.c
@@ -45,6 +45,8 @@
 #   include	<nc.h>
 #endif	/* _hdr_nc */
 
+#define GOT_DEVFD	2
+
 /* These routines are referenced by this module */
 static void	exfile(Sfio_t*,int);
 static void	chkmail(char*);
@@ -218,11 +220,9 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 				fdin = 0;
 			else
 			{
-				char *sp;
 				/* open stream should have been passed into shell */
-				if(strmatch(name,e_devfdNN))
+				if(strmatch(name,e_devfdNN) && (fdin=(int)strtol(name+8, NULL, 10)) != 1)
 				{
-					fdin = (int)strtol(name+8, NULL, 10);
 					if(fstat(fdin,&statb)<0)
 					{
 						errormsg(SH_DICT,ERROR_system(1),e_open,name);
@@ -231,13 +231,15 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 					name = av[0];
 					sh_offoption(SH_VERBOSE);
 					sh_offoption(SH_XTRACE);
+					beenhere = GOT_DEVFD;
 				}
 				else
 				{
+					char *sp;
 					int isdir = 0;
-					if((fdin=sh_open(name,O_RDONLY,0))>=0 &&(fstat(fdin,&statb)<0 || S_ISDIR(statb.st_mode)))
+					if((fdin=sh_open(name,O_RDONLY|O_cloexec,0))>=0 &&(fstat(fdin,&statb)<0 || S_ISDIR(statb.st_mode)))
 					{
-						close(fdin);
+						sh_close(fdin);
 						isdir = 1;
 						fdin = -1;
 					}
@@ -250,7 +252,7 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 							sp = stkptr(sh.stk,PATH_OFFSET);
 						if(sp)
 						{
-							if((fdin=sh_open(sp,O_RDONLY,0))>=0)
+							if((fdin=sh_open(sp,O_RDONLY|O_cloexec,0))>=0)
 								sh.st.filename = path_fullname(sp);
 						}
 					}
@@ -272,7 +274,13 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 							strcopy(name," \"$@\"");
 						goto shell_c;
 					}
-					if(fdin==0)
+					/*
+					 * Note: fdin could wind up being zero or some such if the
+					 *       shell was opened with stdin/stdout/stderr closed
+					 *       (i.e. 0>&-), so we move the fd in such a case to
+					 *       keep that file descriptor closed.
+					 */
+					if(fdin<=2)
 						fdin = sh_iomovefd(fdin);
 				}
 				sh.readscript = sh.shname;
@@ -347,21 +355,27 @@ static void	exfile(Sfio_t *iop,int fno)
 	{
 		if(fno > 0)
 		{
-			if(fno < 10)
+			if(beenhere == GOT_DEVFD)
+				beenhere--;  /* leave the inherited /dev/fd as is */
+			else
 			{
-				int r = sh_fcntl(fno,F_dupfd_cloexec,10);
-				if(r >= 10)
+				if(fno < 10)
 				{
-					if(F_dupfd_cloexec != F_DUPFD)
-						sh.fdstatus[r] = sh.fdstatus[fno]|IOCLEX;
-					else
-						sh.fdstatus[r] = sh.fdstatus[fno];
-					sh_close(fno);
-					fno = r;
+					int r = sh_fcntl(fno,F_dupfd_cloexec,10);
+					if(r >= 10)
+					{
+						if(F_dupfd_cloexec != F_DUPFD)
+							sh.fdstatus[r] = sh.fdstatus[fno]|IOCLEX;
+						else
+							sh.fdstatus[r] = sh.fdstatus[fno];
+						if(fno > 2)
+							sh_close(fno);
+						fno = r;
+					}
 				}
+				if(!(sh.fdstatus[fno]&IOCLEX))
+					sh_fcntl(fno,F_SETFD,FD_CLOEXEC);
 			}
-			if(!(sh.fdstatus[fno]&IOCLEX))
-				sh_fcntl(fno,F_SETFD,FD_CLOEXEC);
 			iop = sh_iostream(fno);
 		}
 		else

@JohnoKing
Copy link

We got a similar behavior, ksh use FD10 for the PWD (dunno why)

The sh_diropenat function used by the cd builtin intentionally dups the fd above 10 with the intent of avoiding the 3-9 range of file descriptors. The code itself is essentially a refactored ksh93v- backport (I assume the original devs wanted 3-9 to be reserved for regular usage with file redirections, so e.g. to avoid 4>&5 causing unforseen consequences if the sh.pwdfd wound up being 5).

if(fd < 10)
{
/* Duplicate the fd and register it with the shell */
int shfd = sh_fcntl(fd, F_dupfd_cloexec, 10);
close(fd);
if(shfd < 0)
return shfd;
if(F_dupfd_cloexec == F_DUPFD)
needs_cloexec = 1;
fd = shfd;
}

@JohnoKing
Copy link

JohnoKing commented Jun 19, 2025

My last patch introduced a regression:

$ arch/*/bin/ksh /dev/fd/2
Memory fault(coredump)
Another try, but dup when it's /dev/fd/2 like is already done with /dev/stderr (edit: updated with minor fix for beenhere):
diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c
index 05aa7b34e..bd3294de3 100644
--- a/src/cmd/ksh93/sh/init.c
+++ b/src/cmd/ksh93/sh/init.c
@@ -1347,10 +1347,7 @@ Shell_t *sh_init(int argc,char *argv[], Shinit_f userinit)
 	else
 		sh_offoption(SH_PRIVILEGED);
 	/* shname for $0 in profiles and . scripts */
-	if(sh_isdevfd(argv[1]))
-		sh.shname = sh_strdup(argv[0]);
-	else
-		sh.shname = sh_strdup(sh.st.dolv[0]);
+	sh.shname = sh_strdup(sh.st.dolv[0]);
 	/*
 	 * return here for shell script execution
 	 * but not for parenthesis subshells
diff --git a/src/cmd/ksh93/sh/main.c b/src/cmd/ksh93/sh/main.c
index ea8f5d92f..7d5c02382 100644
--- a/src/cmd/ksh93/sh/main.c
+++ b/src/cmd/ksh93/sh/main.c
@@ -45,6 +45,8 @@
 #   include	<nc.h>
 #endif	/* _hdr_nc */
 
+#define GOT_DEVFD	2
+
 /* These routines are referenced by this module */
 static void	exfile(Sfio_t*,int);
 static void	chkmail(char*);
@@ -125,7 +127,7 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 #endif
 	if(!beenhere)
 	{
-		beenhere++;
+		beenhere = 1;
 		sh_onstate(SH_PROFILE);
 		sh.sigflag[SIGTSTP] |= SH_SIGIGNORE;
 		/* decide whether shell is interactive */
@@ -218,11 +220,9 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 				fdin = 0;
 			else
 			{
-				char *sp;
 				/* open stream should have been passed into shell */
-				if(strmatch(name,e_devfdNN))
+				if(strmatch(name,e_devfdNN) && (fdin=(int)strtol(name+8, NULL, 10)) != 1)
 				{
-					fdin = (int)strtol(name+8, NULL, 10);
 					if(fstat(fdin,&statb)<0)
 					{
 						errormsg(SH_DICT,ERROR_system(1),e_open,name);
@@ -231,13 +231,15 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 					name = av[0];
 					sh_offoption(SH_VERBOSE);
 					sh_offoption(SH_XTRACE);
+					beenhere = GOT_DEVFD;
 				}
 				else
 				{
+					char *sp;
 					int isdir = 0;
-					if((fdin=sh_open(name,O_RDONLY,0))>=0 &&(fstat(fdin,&statb)<0 || S_ISDIR(statb.st_mode)))
+					if((fdin=sh_open(name,O_RDONLY|O_cloexec,0))>=0 &&(fstat(fdin,&statb)<0 || S_ISDIR(statb.st_mode)))
 					{
-						close(fdin);
+						sh_close(fdin);
 						isdir = 1;
 						fdin = -1;
 					}
@@ -250,7 +252,7 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 							sp = stkptr(sh.stk,PATH_OFFSET);
 						if(sp)
 						{
-							if((fdin=sh_open(sp,O_RDONLY,0))>=0)
+							if((fdin=sh_open(sp,O_RDONLY|O_cloexec,0))>=0)
 								sh.st.filename = path_fullname(sp);
 						}
 					}
@@ -272,7 +274,13 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 							strcopy(name," \"$@\"");
 						goto shell_c;
 					}
-					if(fdin==0)
+					/*
+					 * Note: fdin could wind up being zero or some such if the
+					 *       shell was opened with stdin/stdout/stderr closed
+					 *       (i.e. 0>&-), so we move the fd in such a case to
+					 *       keep that file descriptor closed.
+					 */
+					if(fdin<=2)
 						fdin = sh_iomovefd(fdin);
 				}
 				sh.readscript = sh.shname;
@@ -347,21 +355,28 @@ static void	exfile(Sfio_t *iop,int fno)
 	{
 		if(fno > 0)
 		{
-			if(fno < 10)
+			if(fno > 2 && beenhere == GOT_DEVFD)
+				;  /* leave the inherited /dev/fd as is */
+			else
 			{
-				int r = sh_fcntl(fno,F_dupfd_cloexec,10);
-				if(r >= 10)
+				if(fno < 10)
 				{
-					if(F_dupfd_cloexec != F_DUPFD)
-						sh.fdstatus[r] = sh.fdstatus[fno]|IOCLEX;
-					else
-						sh.fdstatus[r] = sh.fdstatus[fno];
-					sh_close(fno);
-					fno = r;
+					int r = sh_fcntl(fno,F_dupfd_cloexec,10);
+					if(r >= 10)
+					{
+						if(F_dupfd_cloexec != F_DUPFD)
+							sh.fdstatus[r] = sh.fdstatus[fno]|IOCLEX;
+						else
+							sh.fdstatus[r] = sh.fdstatus[fno];
+						/* leave std* fds open when they're not explicitly closed */
+						if(fno > 2)
+							sh_close(fno);
+						fno = r;
+					}
 				}
+				if(!(sh.fdstatus[fno]&IOCLEX))
+					sh_fcntl(fno,F_SETFD,FD_CLOEXEC);
 			}
-			if(!(sh.fdstatus[fno]&IOCLEX))
-				sh_fcntl(fno,F_SETFD,FD_CLOEXEC);
 			iop = sh_iostream(fno);
 		}
 		else
@@ -369,6 +384,7 @@ static void	exfile(Sfio_t *iop,int fno)
 	}
 	else
 		fno = -1;
+	beenhere = 1;
 	sh.infd = fno;
 	if(sh_isstate(SH_INTERACTIVE))
 	{

@phidebian
Copy link
Author

Ha thanx for the FD10 explanations :-)

Just curious, did you tried my patch, on my 2 tests machines (Linux ubuntu) it was OK, but I know ksh is used in a lot of more OS/ARCH, so I'd like to know what it is worth. I think 'may be' the are corner cases on OS that don't provide /dev/fd/NN in which case I may have broken something here.

My patch, beside working on first intention on my ubuntu, seems to be armless, only 2 point patching, reverting would be easy if there are some overlooked cases.

I don't have any macos anymore (due to moving) and I don't know if I broke something here.

In substance, my patch disconnect the recognition of any /dev/fd/NN as a special case, because doing

$ ksh /dev/stdout

works and its call path will never ever enter any special case consideration (and circonvolution) and for this reason I thought that we should not apply any /dev/fd distinction in the code. And magically doing this don't present any noticable problems.

If we got to keep special cases for /dev/fd (regarding the script name in argv[] I mean), then we should implement a special case for /dev/stdin /dev/stdout /dev/stderr as well (still doable) plus all the imaginable mknod/link with whatever name the user (admin) would select, and that's precisely the non return point, we simply can not do that, so back to square one, /dev/fd/NN are no special when given in the argv[] as script to run.

Indeed I may miss something somewhere, that why I rely on code review by others, also not that ksh dev is not my primary focus, so indeed I don't have the history of the dev in my mind, just purely intuitive approach.

@JohnoKing
Copy link

JohnoKing commented Jun 20, 2025

Just curious, did you tried my patch, on my 2 tests machines (Linux ubuntu) it was OK, but I know ksh is used in a lot of more OS/ARCH, so I'd like to know what it is worth.

I haven't done testing on such operating systems yet, but those should be unaffected by your patch. The code you disabled is only reached when ksh is passed a /dev/fd file as a script, which don't exist on such operating systems.
The patch could be improved with SHOPT_DEVFD if directives to avoid that codepath when it's never reached.

New patch with SHOPT_DEVFD if directives
diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c
index 4e5f7fd28..d7e9ac1e3 100644
--- a/src/cmd/ksh93/sh/init.c
+++ b/src/cmd/ksh93/sh/init.c
@@ -1341,10 +1341,7 @@ Shell_t *sh_init(int argc,char *argv[], Shinit_f userinit)
 	else
 		sh_offoption(SH_PRIVILEGED);
 	/* shname for $0 in profiles and . scripts */
-	if(sh_isdevfd(argv[1]))
-		sh.shname = sh_strdup(argv[0]);
-	else
-		sh.shname = sh_strdup(sh.st.dolv[0]);
+	sh.shname = sh_strdup(sh.st.dolv[0]);
 	/*
 	 * return here for shell script execution
 	 * but not for parenthesis subshells
diff --git a/src/cmd/ksh93/sh/main.c b/src/cmd/ksh93/sh/main.c
index b52b149e6..63aa39a75 100644
--- a/src/cmd/ksh93/sh/main.c
+++ b/src/cmd/ksh93/sh/main.c
@@ -64,6 +64,10 @@ static struct stat lastmail;
 static time_t	mailtime;
 static char	beenhere = 0;
 
+#if SHOPT_DEVFD
+#define GOT_DEVFD	2
+#endif
+
 /*
  * search for file and exfile() it if it exists
  * 1 returned if file found, 0 otherwise
@@ -124,7 +128,7 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 #endif
 	if(!beenhere)
 	{
-		beenhere++;
+		beenhere = 1;
 		sh_onstate(SH_PROFILE);
 		sh.sigflag[SIGTSTP] |= SH_SIGIGNORE;
 		/* decide whether shell is interactive */
@@ -217,11 +221,10 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 				fdin = 0;
 			else
 			{
-				char *sp;
+#if SHOPT_DEVFD
 				/* open stream should have been passed into shell */
-				if(strmatch(name,e_devfdNN))
+				if(strmatch(name,e_devfdNN) && (fdin=(int)strtol(name+8, NULL, 10)) != 1)
 				{
-					fdin = (int)strtol(name+8, NULL, 10);
 					if(fstat(fdin,&statb)<0)
 					{
 						errormsg(SH_DICT,ERROR_system(1),e_open,name);
@@ -230,13 +233,16 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 					name = av[0];
 					sh_offoption(SH_VERBOSE);
 					sh_offoption(SH_XTRACE);
+					beenhere = GOT_DEVFD;
 				}
 				else
+#endif /* SHOPT_DEVFD */
 				{
+					char *sp;
 					int isdir = 0;
-					if((fdin=sh_open(name,O_RDONLY,0))>=0 &&(fstat(fdin,&statb)<0 || S_ISDIR(statb.st_mode)))
+					if((fdin=sh_open(name,O_RDONLY|O_cloexec,0))>=0 &&(fstat(fdin,&statb)<0 || S_ISDIR(statb.st_mode)))
 					{
-						close(fdin);
+						sh_close(fdin);
 						isdir = 1;
 						fdin = -1;
 					}
@@ -249,7 +255,7 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 							sp = stkptr(sh.stk,PATH_OFFSET);
 						if(sp)
 						{
-							if((fdin=sh_open(sp,O_RDONLY,0))>=0)
+							if((fdin=sh_open(sp,O_RDONLY|O_cloexec,0))>=0)
 								sh.st.filename = path_fullname(sp);
 						}
 					}
@@ -271,7 +277,13 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 							strcopy(name," \"$@\"");
 						goto shell_c;
 					}
-					if(fdin==0)
+					/*
+					 * Note: fdin could wind up being zero or some such if the
+					 *       shell was opened with stdin/stdout/stderr closed
+					 *       (i.e. 0>&-), so we move the fd in such a case to
+					 *       keep that file descriptor closed.
+					 */
+					if(fdin<=2)
 						fdin = sh_iomovefd(fdin);
 				}
 				sh.readscript = sh.shname;
@@ -346,21 +358,30 @@ static void	exfile(Sfio_t *iop,int fno)
 	{
 		if(fno > 0)
 		{
-			if(fno < 10)
+#if SHOPT_DEVFD
+			if(fno > 2 && beenhere == GOT_DEVFD)
+				;  /* leave the inherited /dev/fd as is */
+			else
+#endif
 			{
-				int r = sh_fcntl(fno,F_dupfd_cloexec,10);
-				if(r >= 10)
+				if(fno < 10)
 				{
-					if(F_dupfd_cloexec != F_DUPFD)
-						sh.fdstatus[r] = sh.fdstatus[fno]|IOCLEX;
-					else
-						sh.fdstatus[r] = sh.fdstatus[fno];
-					sh_close(fno);
-					fno = r;
+					int r = sh_fcntl(fno,F_dupfd_cloexec,10);
+					if(r >= 10)
+					{
+						if(F_dupfd_cloexec != F_DUPFD)
+							sh.fdstatus[r] = sh.fdstatus[fno]|IOCLEX;
+						else
+							sh.fdstatus[r] = sh.fdstatus[fno];
+						/* leave std* fds open when they're not explicitly closed */
+						if(fno > 2)
+							sh_close(fno);
+						fno = r;
+					}
 				}
+				if(!(sh.fdstatus[fno]&IOCLEX))
+					sh_fcntl(fno,F_SETFD,FD_CLOEXEC);
 			}
-			if(!(sh.fdstatus[fno]&IOCLEX))
-				sh_fcntl(fno,F_SETFD,FD_CLOEXEC);
 			iop = sh_iostream(fno);
 		}
 		else
@@ -368,6 +389,7 @@ static void	exfile(Sfio_t *iop,int fno)
 	}
 	else
 		fno = -1;
+	beenhere = 1;
 	sh.infd = fno;
 	if(sh_isstate(SH_INTERACTIVE))
 	{

@phidebian
Copy link
Author

Ok got it OS without /dev/fd/NN are imune.

Is there any rationale I should know of for taking care of special cases for script named /dev/fd/NN vs doing nothing special as for OS's without /dev/fd/NN ?

@JohnoKing
Copy link

The special casing code might have been originally implemented as an optimization (there's no need to open the script with an open call if there's already an open file descriptor for it via /dev/fd).
In the current code the main functional concern would be to avoid closing an inherited file descriptor (so avoiding close(2) and FD_CLOEXEC if ksh didn't open the fd).

@phidebian
Copy link
Author

There is no need to open...
Yes there is a need, not doing so leads to bugs. Or may be you mean perf wise, I doubt we can notice that.

The FD saving is not a good idea, I guess, why would we optimize /dev/fd/NN and not /dev/std{in,out,err} or /proc/$$/fd/NN or any other name from mknod/link sounds not orthogonal to me

By doing special case for /dev/fd/NN but not /dev/std{in,out,err} there is a risk of behavior difference between them while there should not, and then /dev/fd/NN must be treated as /dev/std{in,out,err} or /proc/$$/fd/NN

I don't consider dup for iop for the script a leak because that's what we got for regular script.

@JohnoKing
Copy link

JohnoKing commented Jun 20, 2025

there is a risk of behavior difference between them while there should not

Attempting to optimize /dev/std{in,out,err} in the same manner is likely doomed for failure (segfaults), so I'll concede a better route is to avoid that optimization for the equivalent /dev/fd/{0,1,2} to guarantee the same behavior and no segfaulting. The optimization should stay for higher fds though. In those cases if execveat or something similar passes down a file descriptor to ksh for the script, ksh should use that same fd to remain consistent with $0 (cf. #866 (comment) and #874). Closing it after dup'ing is a bad idea since that would make $0 inaccurate, and duping it without closing the fd is a waste of syscalls (the fd is > 2 in that scenario, so ksh should be able to use the fd itself without issues).

enforce-same-behavior.patch
diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c
index 4e5f7fd28..d7e9ac1e3 100644
--- a/src/cmd/ksh93/sh/init.c
+++ b/src/cmd/ksh93/sh/init.c
@@ -1341,10 +1341,7 @@ Shell_t *sh_init(int argc,char *argv[], Shinit_f userinit)
 	else
 		sh_offoption(SH_PRIVILEGED);
 	/* shname for $0 in profiles and . scripts */
-	if(sh_isdevfd(argv[1]))
-		sh.shname = sh_strdup(argv[0]);
-	else
-		sh.shname = sh_strdup(sh.st.dolv[0]);
+	sh.shname = sh_strdup(sh.st.dolv[0]);
 	/*
 	 * return here for shell script execution
 	 * but not for parenthesis subshells
diff --git a/src/cmd/ksh93/sh/main.c b/src/cmd/ksh93/sh/main.c
index b52b149e6..1a125fca0 100644
--- a/src/cmd/ksh93/sh/main.c
+++ b/src/cmd/ksh93/sh/main.c
@@ -64,6 +64,10 @@ static struct stat lastmail;
 static time_t	mailtime;
 static char	beenhere = 0;
 
+#if SHOPT_DEVFD
+#define GOT_DEVFD	2
+#endif
+
 /*
  * search for file and exfile() it if it exists
  * 1 returned if file found, 0 otherwise
@@ -124,7 +128,7 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 #endif
 	if(!beenhere)
 	{
-		beenhere++;
+		beenhere = 1;
 		sh_onstate(SH_PROFILE);
 		sh.sigflag[SIGTSTP] |= SH_SIGIGNORE;
 		/* decide whether shell is interactive */
@@ -217,11 +221,10 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 				fdin = 0;
 			else
 			{
-				char *sp;
+#if SHOPT_DEVFD
 				/* open stream should have been passed into shell */
-				if(strmatch(name,e_devfdNN))
+				if(strmatch(name,e_devfdNN) && (fdin=(int)strtol(name+8, NULL, 10)) > 2)
 				{
-					fdin = (int)strtol(name+8, NULL, 10);
 					if(fstat(fdin,&statb)<0)
 					{
 						errormsg(SH_DICT,ERROR_system(1),e_open,name);
@@ -230,13 +233,16 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 					name = av[0];
 					sh_offoption(SH_VERBOSE);
 					sh_offoption(SH_XTRACE);
+					beenhere = GOT_DEVFD;
 				}
 				else
+#endif /* SHOPT_DEVFD */
 				{
+					char *sp;
 					int isdir = 0;
-					if((fdin=sh_open(name,O_RDONLY,0))>=0 &&(fstat(fdin,&statb)<0 || S_ISDIR(statb.st_mode)))
+					if((fdin=sh_open(name,O_RDONLY|O_cloexec,0))>=0 &&(fstat(fdin,&statb)<0 || S_ISDIR(statb.st_mode)))
 					{
-						close(fdin);
+						sh_close(fdin);
 						isdir = 1;
 						fdin = -1;
 					}
@@ -249,7 +255,7 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 							sp = stkptr(sh.stk,PATH_OFFSET);
 						if(sp)
 						{
-							if((fdin=sh_open(sp,O_RDONLY,0))>=0)
+							if((fdin=sh_open(sp,O_RDONLY|O_cloexec,0))>=0)
 								sh.st.filename = path_fullname(sp);
 						}
 					}
@@ -271,7 +277,13 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 							strcopy(name," \"$@\"");
 						goto shell_c;
 					}
-					if(fdin==0)
+					/*
+					 * Note: fdin could wind up being zero or some such if the
+					 *       shell was opened with stdin/stdout/stderr closed
+					 *       (i.e. 0>&-), so we move the fd in such a case to
+					 *       keep that file descriptor closed.
+					 */
+					if(fdin<=2)
 						fdin = sh_iomovefd(fdin);
 				}
 				sh.readscript = sh.shname;
@@ -346,21 +358,30 @@ static void	exfile(Sfio_t *iop,int fno)
 	{
 		if(fno > 0)
 		{
-			if(fno < 10)
+#if SHOPT_DEVFD
+			if(beenhere == GOT_DEVFD)
+				;  /* leave the inherited /dev/fd as is */
+			else
+#endif
 			{
-				int r = sh_fcntl(fno,F_dupfd_cloexec,10);
-				if(r >= 10)
+				if(fno < 10)
 				{
-					if(F_dupfd_cloexec != F_DUPFD)
-						sh.fdstatus[r] = sh.fdstatus[fno]|IOCLEX;
-					else
-						sh.fdstatus[r] = sh.fdstatus[fno];
-					sh_close(fno);
-					fno = r;
+					int r = sh_fcntl(fno,F_dupfd_cloexec,10);
+					if(r >= 10)
+					{
+						if(F_dupfd_cloexec != F_DUPFD)
+							sh.fdstatus[r] = sh.fdstatus[fno]|IOCLEX;
+						else
+							sh.fdstatus[r] = sh.fdstatus[fno];
+						/* leave std* fds open when they're not explicitly closed */
+						if(fno > 2)
+							sh_close(fno);
+						fno = r;
+					}
 				}
+				if(!(sh.fdstatus[fno]&IOCLEX))
+					sh_fcntl(fno,F_SETFD,FD_CLOEXEC);
 			}
-			if(!(sh.fdstatus[fno]&IOCLEX))
-				sh_fcntl(fno,F_SETFD,FD_CLOEXEC);
 			iop = sh_iostream(fno);
 		}
 		else
@@ -368,6 +389,7 @@ static void	exfile(Sfio_t *iop,int fno)
 	}
 	else
 		fno = -1;
+	beenhere = 1;
 	sh.infd = fno;
 	if(sh_isstate(SH_INTERACTIVE))
 	{

@JohnoKing
Copy link

JohnoKing commented Jun 20, 2025

Another improvement to the patch: if the fd moved with sh_iomovefd() will need to be moved to 10 later on in exfile(), dup it to 10 in advance if an opportunity arises.

Edit: Updated 07-11-2025 with added regression tests and a patch for use with git am.

0001-move-to-ten-in-advance.patch
From b936962dc49319aa5e82014afd92ca5c4e671c2d Mon Sep 17 00:00:00 2001
From: Johnothan King <[email protected]>
Date: Fri, 11 Jul 2025 14:16:13 -0700
Subject: Fix /dev/fd/1 memory fault and $0 for /dev/fd scripts

Changes:
- sh_init(): Don't set $0 to the path to ksh. The correct behavior is to
  always use the script name, even when it's a /dev/fd file. The behavior
  of ksh93 should match that of bash and other shells. Please read:
  https://github.com/ksh93/ksh/pull/866#issuecomment-2968762167
  https://github.com/ksh93/ksh/issues/874#issuecomment-2977615731
- sh_main(): Do not attempt to optimize away sh_open() for the
  stdio fds (0, 1, 2) because that can cause buggy behavior.
  Ksh's handling of /dev/fd/1 and /dev/stdin should not differ. See:
  https://github.com/ksh93/ksh/pull/879#issuecomment-2989729921
- exfile(): Don't dup or close fds for /dev/fd scripts with a file
  descriptor > 2. Those should be left open pursuant to the $0 name
  of the script. (This cannot be done safely for fds 1 and 2, so the
  stdio fds are all dup'ed, but not closed.)
- sh_main(): If the fd obtained for the script by sh_open() is <= 2,
  that means the shell was opened with one of the stdio fds closed,
  so for correct behavior use sh_iomovefd() to keep the stdio fd closed.
- sh_iomovefd(): As an optimization, move the fd to a number that's
  either >= 3 or >= 10, depending on what is most appropriate. This
  avoids a possible inefficient scenario of dup'ing an fd from
  0 -> 3 -> 10 (less syscalls is better for performance).
- sh_main(): Use O_CLOEXEC in advance for the fd (which is guaranteed to
  get close-on-exec via a later fcntl call; if we don't end up calling
  fcntl for whatever reason O_CLOEXEC will help avoid an extra syscall).
- basic.sh: Add regression tests for $0 and its associated fd. For the
  time being I've omitted a /dev/fd/1 regression test because I can't
  get one to work correctly with the pty tool.

Fixes https://github.com/ksh93/ksh/issues/874
Fixes https://github.com/ksh93/ksh/issues/877
---
 src/cmd/ksh93/include/io.h   |  2 +-
 src/cmd/ksh93/sh/init.c      |  5 +--
 src/cmd/ksh93/sh/io.c        | 20 ++++++------
 src/cmd/ksh93/sh/main.c      | 60 ++++++++++++++++++++++++------------
 src/cmd/ksh93/sh/path.c      |  4 +--
 src/cmd/ksh93/tests/basic.sh | 25 ++++++++++++++-
 6 files changed, 79 insertions(+), 37 deletions(-)

diff --git a/src/cmd/ksh93/include/io.h b/src/cmd/ksh93/include/io.h
index 67a64fcaf..d01512ba7 100644
--- a/src/cmd/ksh93/include/io.h
+++ b/src/cmd/ksh93/include/io.h
@@ -76,7 +76,7 @@
 
 extern int	sh_iocheckfd(int);
 extern void 	sh_ioinit(void);
-extern int 	sh_iomovefd(int);
+extern int 	sh_iomovefd(int,int);
 extern int	sh_iorenumber(int,int);
 extern void 	sh_pclose(int[]);
 extern int	sh_rpipe(int[],int);
diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c
index 4e5f7fd28..d7e9ac1e3 100644
--- a/src/cmd/ksh93/sh/init.c
+++ b/src/cmd/ksh93/sh/init.c
@@ -1341,10 +1341,7 @@ Shell_t *sh_init(int argc,char *argv[], Shinit_f userinit)
 	else
 		sh_offoption(SH_PRIVILEGED);
 	/* shname for $0 in profiles and . scripts */
-	if(sh_isdevfd(argv[1]))
-		sh.shname = sh_strdup(argv[0]);
-	else
-		sh.shname = sh_strdup(sh.st.dolv[0]);
+	sh.shname = sh_strdup(sh.st.dolv[0]);
 	/*
 	 * return here for shell script execution
 	 * but not for parenthesis subshells
diff --git a/src/cmd/ksh93/sh/io.c b/src/cmd/ksh93/sh/io.c
index 0fa7cf2ff..90b7c78b4 100644
--- a/src/cmd/ksh93/sh/io.c
+++ b/src/cmd/ksh93/sh/io.c
@@ -896,20 +896,20 @@ int sh_chkopen(const char *name)
 }
 
 /*
- * move open file descriptor to a number > 2
+ * move open file descriptor to a number >= minfd
  */
-int sh_iomovefd(int fdold)
+int sh_iomovefd(int fdold, int minfd)
 {
 	int fdnew, dupflags;
 	if(fdold >= sh.lim.open_max)
 		sh_iovalidfd(fdold);
-	if(fdold<0 || fdold>2)
+	if(fdold<0 || fdold>=minfd)
 		return fdold;
 	if(sh.fdstatus[fdold]&IOCLEX)
 		dupflags = F_dupfd_cloexec;
 	else
 		dupflags = F_DUPFD;
-	fdnew = sh_iomovefd(fcntl(fdold,dupflags,3));
+	fdnew = sh_iomovefd(fcntl(fdold,dupflags,minfd),minfd);
 	if((sh.fdstatus[fdold]&IOCLEX) && F_dupfd_cloexec == F_DUPFD)
 		fcntl(fdnew,F_SETFD,FD_CLOEXEC);
 	sh.fdstatus[fdnew] = sh.fdstatus[fdold];
@@ -945,9 +945,9 @@ int	sh_pipe(int pv[], int cloexec)
 	sh.fdstatus[pv[0]] = IONOSEEK|IOREAD|cloexec;
 	sh.fdstatus[pv[1]] = IONOSEEK|IOWRITE|cloexec;
 	if(pv[0]<=2)
-		pv[0] = sh_iomovefd(pv[0]);
+		pv[0] = sh_iomovefd(pv[0],3);
 	if(pv[1]<=2)
-		pv[1] = sh_iomovefd(pv[1]);
+		pv[1] = sh_iomovefd(pv[1],3);
 	sh_subsavefd(pv[0]);
 	sh_subsavefd(pv[1]);
 	return 0;
@@ -977,9 +977,9 @@ int	sh_rpipe(int pv[], int cloexec)
 	sh.fdstatus[pv[0]] = IONOSEEK|IOREAD|cloexec;
 	sh.fdstatus[pv[1]] = IONOSEEK|IOWRITE|cloexec;
 	if(pv[0]<=2)
-		pv[0] = sh_iomovefd(pv[0]);
+		pv[0] = sh_iomovefd(pv[0],3);
 	if(pv[1]<=2)
-		pv[1] = sh_iomovefd(pv[1]);
+		pv[1] = sh_iomovefd(pv[1],3);
 	sh_subsavefd(pv[0]);
 	sh_subsavefd(pv[1]);
 	return 0;
@@ -1534,7 +1534,7 @@ int	sh_redirect(struct ionod *iop, int flag)
 				sh_close(fn);
 			}
 			if(flag==3)
-				return sh_iomovefd(fd);  /* ensure FD > 2 to make $(<file) work with std{in,out,err} closed */
+				return sh_iomovefd(fd,3);  /* ensure FD > 2 to make $(<file) work with std{in,out,err} closed */
 			if(fd>=0)
 			{
 				if(np)
@@ -1561,7 +1561,7 @@ int	sh_redirect(struct ionod *iop, int flag)
 				}
 				else
 				{
-					fd = sh_iorenumber(sh_iomovefd(fd),fn);
+					fd = sh_iorenumber(sh_iomovefd(fd,3),fn);
 					if(fn>2 && fn<10)
 						sh.inuse_bits |= (1<<fn);
 				}
diff --git a/src/cmd/ksh93/sh/main.c b/src/cmd/ksh93/sh/main.c
index b52b149e6..e523eb2e5 100644
--- a/src/cmd/ksh93/sh/main.c
+++ b/src/cmd/ksh93/sh/main.c
@@ -64,6 +64,10 @@ static struct stat lastmail;
 static time_t	mailtime;
 static char	beenhere = 0;
 
+#if SHOPT_DEVFD
+#define GOT_DEVFD	2
+#endif
+
 /*
  * search for file and exfile() it if it exists
  * 1 returned if file found, 0 otherwise
@@ -124,7 +128,7 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 #endif
 	if(!beenhere)
 	{
-		beenhere++;
+		beenhere = 1;
 		sh_onstate(SH_PROFILE);
 		sh.sigflag[SIGTSTP] |= SH_SIGIGNORE;
 		/* decide whether shell is interactive */
@@ -217,11 +221,10 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 				fdin = 0;
 			else
 			{
-				char *sp;
+#if SHOPT_DEVFD
 				/* open stream should have been passed into shell */
-				if(strmatch(name,e_devfdNN))
+				if(strmatch(name,e_devfdNN) && (fdin=(int)strtol(name+8, NULL, 10)) > 2)
 				{
-					fdin = (int)strtol(name+8, NULL, 10);
 					if(fstat(fdin,&statb)<0)
 					{
 						errormsg(SH_DICT,ERROR_system(1),e_open,name);
@@ -230,11 +233,14 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 					name = av[0];
 					sh_offoption(SH_VERBOSE);
 					sh_offoption(SH_XTRACE);
+					beenhere = GOT_DEVFD;
 				}
 				else
+#endif /* SHOPT_DEVFD */
 				{
+					char *sp;
 					int isdir = 0;
-					if((fdin=sh_open(name,O_RDONLY,0))>=0 &&(fstat(fdin,&statb)<0 || S_ISDIR(statb.st_mode)))
+					if((fdin=sh_open(name,O_RDONLY|O_cloexec,0))>=0 &&(fstat(fdin,&statb)<0 || S_ISDIR(statb.st_mode)))
 					{
 						close(fdin);
 						isdir = 1;
@@ -249,7 +255,7 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 							sp = stkptr(sh.stk,PATH_OFFSET);
 						if(sp)
 						{
-							if((fdin=sh_open(sp,O_RDONLY,0))>=0)
+							if((fdin=sh_open(sp,O_RDONLY|O_cloexec,0))>=0)
 								sh.st.filename = path_fullname(sp);
 						}
 					}
@@ -271,8 +277,14 @@ noreturn void sh_main(int ac, char *av[], Shinit_f userinit)
 							strcopy(name," \"$@\"");
 						goto shell_c;
 					}
-					if(fdin==0)
-						fdin = sh_iomovefd(fdin);
+					/*
+					 * Note: fdin could wind up being zero or some such if the
+					 *       shell was opened with stdin/stdout/stderr closed
+					 *       (i.e. 0>&-), so we move the fd in such a case to
+					 *       keep that file descriptor closed.
+					 */
+					if(fdin<=2)
+						fdin = sh_iomovefd(fdin,10);
 				}
 				sh.readscript = sh.shname;
 			}
@@ -346,21 +358,30 @@ static void	exfile(Sfio_t *iop,int fno)
 	{
 		if(fno > 0)
 		{
-			if(fno < 10)
+#if SHOPT_DEVFD
+			if(beenhere == GOT_DEVFD)
+				;  /* leave the inherited /dev/fd as is */
+			else
+#endif
 			{
-				int r = sh_fcntl(fno,F_dupfd_cloexec,10);
-				if(r >= 10)
+				if(fno < 10)
 				{
-					if(F_dupfd_cloexec != F_DUPFD)
-						sh.fdstatus[r] = sh.fdstatus[fno]|IOCLEX;
-					else
-						sh.fdstatus[r] = sh.fdstatus[fno];
-					sh_close(fno);
-					fno = r;
+					int r = sh_fcntl(fno,F_dupfd_cloexec,10);
+					if(r >= 10)
+					{
+						if(F_dupfd_cloexec != F_DUPFD)
+							sh.fdstatus[r] = sh.fdstatus[fno]|IOCLEX;
+						else
+							sh.fdstatus[r] = sh.fdstatus[fno];
+						/* leave std* fds open when they're not explicitly closed */
+						if(fno > 2)
+							sh_close(fno);
+						fno = r;
+					}
 				}
+				if(!(sh.fdstatus[fno]&IOCLEX))
+					sh_fcntl(fno,F_SETFD,FD_CLOEXEC);
 			}
-			if(!(sh.fdstatus[fno]&IOCLEX))
-				sh_fcntl(fno,F_SETFD,FD_CLOEXEC);
 			iop = sh_iostream(fno);
 		}
 		else
@@ -368,6 +389,7 @@ static void	exfile(Sfio_t *iop,int fno)
 	}
 	else
 		fno = -1;
+	beenhere = 1;
 	sh.infd = fno;
 	if(sh_isstate(SH_INTERACTIVE))
 	{
diff --git a/src/cmd/ksh93/sh/path.c b/src/cmd/ksh93/sh/path.c
index 298bbd2e2..02d93835f 100644
--- a/src/cmd/ksh93/sh/path.c
+++ b/src/cmd/ksh93/sh/path.c
@@ -486,7 +486,7 @@ static int	opentype(const char *name, Pathcomp_t *pp, int fun)
 		}
 	}
 	while(fd<0 && nextpp);
-	if(fd>=0 && (fd = sh_iomovefd(fd)) > 0 && !(sh.fdstatus[fd]&IOCLEX))
+	if(fd>=0 && (fd = sh_iomovefd(fd,10)) > 0 && !(sh.fdstatus[fd]&IOCLEX))
 		sh_fcntl(fd,F_SETFD,FD_CLOEXEC);
 	return fd;
 }
@@ -1320,7 +1320,7 @@ static noreturn void exscript(char *path,char *argv[])
 		errormsg(SH_DICT,ERROR_system(ERROR_NOEXEC),e_exec,path);
 		UNREACHABLE();
 	}
-	sh.infd = sh_iomovefd(sh.infd);
+	sh.infd = sh_iomovefd(sh.infd,10);
 #if SHOPT_ACCT
 	sh_accbegin(path) ;  /* reset accounting */
 #endif	/* SHOPT_ACCT */
diff --git a/src/cmd/ksh93/tests/basic.sh b/src/cmd/ksh93/tests/basic.sh
index 9e503a906..5fa3319da 100755
--- a/src/cmd/ksh93/tests/basic.sh
+++ b/src/cmd/ksh93/tests/basic.sh
@@ -1110,7 +1110,30 @@ cp "$bindir/hijack_shell" "$bindir/hijack_sh"
 rm -r "$bindir"
 unset bindir
 [[ $exp == $got ]] || err_exit 'ksh93 shebang-less scripts are vulnerable to being hijacked for arbitrary code execution' \
-	"(exp $(printf %q "$exp"), got $(printf %q "$got"))"
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
+# ======
+# $0 should be the /dev/fd path for scripts executed from a /dev/fd file
+# https://github.com/ksh93/ksh/issues/874
+# https://github.com/ksh93/ksh/pull/879
+if ((SHOPT_DEVFD))
+then	# $0 should be the /dev/fd script name when the script is a process substitution
+	got=$("$SHELL" <(echo 'echo $0'))
+	if [[ ${got:0:7} != '/dev/fd' ]]
+	then err_exit '$0 is wrong for process substitution scripts' \
+		"(expected a /dev/fd file name, got $(printf %q "$got"))"
+	else	# The file descriptor for the /dev/fd script must remain open
+		if ! "$SHELL" <(echo '[[ -e $0 ]]')
+		then	err_exit 'the file descriptor corresponding to $0 is not open in /dev/fd scripts'
+		fi
+		# The file descriptor for the /dev/fd script should not be close-on-exec
+		typeset -x verify_fd_script=$tmp/verify-procsub-$RANDOM.ksh
+		echo '[[ -e $fd ]]' > "$verify_fd_script"
+		if ! "$SHELL" <(echo 'typeset -x fd=$0; "$SHELL" "$verify_fd_script"')
+		then	err_exit 'file descriptor for /dev/fd script is not passed on to child processes'
+		fi
+	fi
+fi
 
 # ======
 exit $((Errors<125?Errors:125))
-- 
2.50.1

JohnoKing added a commit to JohnoKing/ksh that referenced this pull request Jul 11, 2025
Changes:
- sh_init(): Don't set $0 to the path to ksh. The correct behavior is to
  always use the script name, even when it's a /dev/fd file. The behavior
  of ksh93 should match that of bash and other shells. Please read:
  ksh93#866 (comment)
  ksh93#874 (comment)
- sh_main(): Do not attempt to optimize away sh_open() for the
  stdio fds (0, 1, 2) because that can cause buggy behavior.
  Ksh's handling of /dev/fd/1 and /dev/stdin should not differ. See:
  ksh93#879 (comment)
- exfile(): Don't dup or close fds for /dev/fd scripts with a file
  descriptor > 2. Those should be left open pursuant to the $0 name
  of the script. (This cannot be done safely for fds 1 and 2, so the
  stdio fds are all dup'ed, but not closed.)
- sh_main(): If the fd obtained for the script by sh_open() is <= 2,
  that means the shell was opened with one of the stdio fds closed,
  so for correct behavior use sh_iomovefd() to keep the stdio fd closed.
- sh_iomovefd(): As an optimization, move the fd to a number that's
  either >= 3 or >= 10, depending on what is most appropriate. This
  avoids a possible inefficient scenario of dup'ing an fd from
  0 -> 3 -> 10 (less syscalls is better for performance).
- sh_main(): Use O_CLOEXEC in advance for the fd (which is guaranteed to
  get close-on-exec via a later fcntl call; if we don't end up calling
  fcntl for whatever reason O_CLOEXEC will help avoid an extra syscall).
- basic.sh: Add regression tests for $0 and its associated fd. For the
  time being I've omitted a /dev/fd/1 regression test because I can't
  get one to work correctly with the pty tool.

Patch originally posted here:
ksh93#879 (comment)

Fixes ksh93#874
Fixes ksh93#877
JohnoKing added a commit to JohnoKing/ksh that referenced this pull request Jul 12, 2025
Changes:
- sh_init(): Don't set $0 to the path to ksh. The correct behavior is to
  always use the script name, even when it's a /dev/fd file. The behavior
  of ksh93 should match that of bash and other shells. Please read:
  ksh93#866 (comment)
  ksh93#874 (comment)
- sh_main(): Do not attempt to optimize away sh_open() for the
  stdio fds (0, 1, 2) because that can cause buggy behavior.
  Ksh's handling of /dev/fd/1 and /dev/stdin should not differ. See:
  ksh93#879 (comment)
- exfile(): Don't dup or close fds for /dev/fd scripts with a file
  descriptor > 2. Those should be left open pursuant to the $0 name
  of the script. (This cannot be done safely for fds 1 and 2, so the
  stdio fds are all dup'ed, but not closed.)
- sh_main(): If the fd obtained for the script by sh_open() is <= 2,
  that means the shell was opened with one of the stdio fds closed,
  so for correct behavior use sh_iomovefd() to keep the stdio fd closed.
- sh_iomovefd(): As an optimization, move the fd to a number that's
  either >= 3 or >= 10, depending on what is most appropriate. This
  avoids a possible inefficient scenario of dup'ing an fd from
  0 -> 3 -> 10 (less syscalls is better for performance).
- sh_main(): Use O_CLOEXEC in advance for the fd (which is guaranteed to
  get close-on-exec via a later fcntl call; if we don't end up calling
  fcntl for whatever reason O_CLOEXEC will help avoid an extra syscall).
- basic.sh: Add regression tests for $0 and its associated fd. For the
  time being I've omitted a /dev/fd/1 regression test because I can't
  get one to work correctly with the pty tool.

Patch originally posted here:
ksh93#879 (comment)

Fixes ksh93#874
Fixes ksh93#877
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants