[patch] kern/exec_script: return error when the shell name is not specified

Discussion:

Maxim Pugachev

2015-12-12 21:28:00 UTC

Hi,

In a case when the shell name is not specified (i.e. just "#!" without
a path), don't run the heavy logic that checks shell, simply return
ENOENT.

Also, as a tiny improvement: avoid a loop when calculating shell's args length.

Index: sys/kern/exec_script.c
===================================================================
RCS file: /cvs/src/sys/kern/exec_script.c,v
retrieving revision 1.36
diff -u -p -r1.36 exec_script.c
--- sys/kern/exec_script.c 10 Sep 2015 18:10:35 -0000 1.36
+++ sys/kern/exec_script.c 12 Dec 2015 21:18:22 -0000
@@ -69,7 +69,7 @@ exec_script_makecmds(struct proc *p, str
{
int error, hdrlinelen, shellnamelen, shellarglen;
char *hdrstr = epp->ep_hdr;
- char *cp, *shellname, *shellarg, *oldpnbuf;
+ char *cp, *endcp, *shellname, *shellarg, *oldpnbuf;
char **shellargp = NULL, **tmpsap;
struct vnode *scriptvp;
uid_t script_uid = -1;
@@ -103,6 +103,7 @@ exec_script_makecmds(struct proc *p, str
for (cp = hdrstr + EXEC_SCRIPT_MAGICLEN; cp < hdrstr + hdrlinelen;
cp++) {
if (*cp == '\n') {
+ endcp = cp;
*cp = '\0';
break;
}
@@ -119,21 +120,23 @@ exec_script_makecmds(struct proc *p, str
cp++)
;

+ /* return error when the shell name is not specified */
+ if (cp == endcp)
+ return ENOENT;
+
/* collect the shell name; remember its length for later */
shellname = cp;
shellnamelen = 0;
- if (*cp == '\0')
- goto check_shell;
for ( /* cp = cp */ ; *cp != '\0' && *cp != ' ' && *cp != '\t'; cp++)
shellnamelen++;
- if (*cp == '\0')
+ if (cp == endcp)
goto check_shell;
*cp++ = '\0';

/* skip spaces before any argument */
for ( /* cp = cp */ ; *cp == ' ' || *cp == '\t'; cp++)
;
- if (*cp == '\0')
+ if (cp == endcp)
goto check_shell;

/*
@@ -142,9 +145,7 @@ exec_script_makecmds(struct proc *p, str
* behaviour.
*/
shellarg = cp;
- for ( /* cp = cp */ ; *cp != '\0'; cp++)
- shellarglen++;
- *cp++ = '\0';
+ shellarglen = endcp - cp;

check_shell:
/*

Maxim Pugachev

2015-12-16 18:55:35 UTC

Permalink

Ping?

Post by Maxim Pugachev
Hi,
In a case when the shell name is not specified (i.e. just "#!" without
a path), don't run the heavy logic that checks shell, simply return
ENOENT.
Also, as a tiny improvement: avoid a loop when calculating shell's args length.
Index: sys/kern/exec_script.c
===================================================================
RCS file: /cvs/src/sys/kern/exec_script.c,v
retrieving revision 1.36
diff -u -p -r1.36 exec_script.c
--- sys/kern/exec_script.c 10 Sep 2015 18:10:35 -0000 1.36
+++ sys/kern/exec_script.c 12 Dec 2015 21:18:22 -0000
@@ -69,7 +69,7 @@ exec_script_makecmds(struct proc *p, str
{
int error, hdrlinelen, shellnamelen, shellarglen;
char *hdrstr = epp->ep_hdr;
- char *cp, *shellname, *shellarg, *oldpnbuf;
+ char *cp, *endcp, *shellname, *shellarg, *oldpnbuf;
char **shellargp = NULL, **tmpsap;
struct vnode *scriptvp;
uid_t script_uid = -1;
@@ -103,6 +103,7 @@ exec_script_makecmds(struct proc *p, str
for (cp = hdrstr + EXEC_SCRIPT_MAGICLEN; cp < hdrstr + hdrlinelen;
cp++) {
if (*cp == '\n') {
+ endcp = cp;
*cp = '\0';
break;
}
@@ -119,21 +120,23 @@ exec_script_makecmds(struct proc *p, str
cp++)
;
+ /* return error when the shell name is not specified */
+ if (cp == endcp)
+ return ENOENT;
+
/* collect the shell name; remember its length for later */
shellname = cp;
shellnamelen = 0;
- if (*cp == '\0')
- goto check_shell;
for ( /* cp = cp */ ; *cp != '\0' && *cp != ' ' && *cp != '\t'; cp++)
shellnamelen++;
- if (*cp == '\0')
+ if (cp == endcp)
goto check_shell;
*cp++ = '\0';
/* skip spaces before any argument */
for ( /* cp = cp */ ; *cp == ' ' || *cp == '\t'; cp++)
;
- if (*cp == '\0')
+ if (cp == endcp)
goto check_shell;
/*
@@ -142,9 +145,7 @@ exec_script_makecmds(struct proc *p, str
* behaviour.
*/
shellarg = cp;
- for ( /* cp = cp */ ; *cp != '\0'; cp++)
- shellarglen++;
- *cp++ = '\0';
+ shellarglen = endcp - cp;
/*

Ted Unangst

2015-12-17 17:18:12 UTC

Permalink

Ping?

Not forgotten. Just not a very pretty file to look at. :)

Maxim Pugachev

2016-01-05 11:28:12 UTC

Permalink

Ping?

Post by Ted Unangst

Ping?

Not forgotten. Just not a very pretty file to look at. :)

Michael McConville

2016-01-10 01:51:17 UTC

Permalink

Post by Maxim Pugachev
In a case when the shell name is not specified (i.e. just "#!" without
a path), don't run the heavy logic that checks shell, simply return
ENOENT.

I'm not sure whether this is a reasonable thing to do. Someone with more
kernel API experience will have to comment.

Post by Maxim Pugachev
Also, as a tiny improvement: avoid a loop when calculating shell's args length.

It seems like there's a simpler solution to this: strlen. Also, the
assignment that follows the for loop seems dead, as we know *cp will be
NUL.

The below diff makes that change, removes the useless if condition you
noticed, and bumps a few variable initializations into the declaration
block.

Index: sys/kern/exec_script.c
===================================================================
RCS file: /cvs/src/sys/kern/exec_script.c,v
retrieving revision 1.37
diff -u -p -r1.37 exec_script.c
--- sys/kern/exec_script.c 31 Dec 2015 18:55:33 -0000 1.37
+++ sys/kern/exec_script.c 10 Jan 2016 01:41:55 -0000
@@ -67,9 +67,9 @@
int
exec_script_makecmds(struct proc *p, struct exec_package *epp)
{
- int error, hdrlinelen, shellnamelen, shellarglen;
+ int error, hdrlinelen, shellnamelen, shellarglen = 0;
char *hdrstr = epp->ep_hdr;
- char *cp, *shellname, *shellarg, *oldpnbuf;
+ char *cp, *shellname = NULL, *shellarg = NULL, *oldpnbuf;
char **shellargp = NULL, **tmpsap;
struct vnode *scriptvp;
uid_t script_uid = -1;
@@ -110,10 +110,6 @@ exec_script_makecmds(struct proc *p, str
if (cp >= hdrstr + hdrlinelen)
return ENOEXEC;

- shellname = NULL;
- shellarg = NULL;
- shellarglen = 0;
-
/* strip spaces before the shell name */
for (cp = hdrstr + EXEC_SCRIPT_MAGICLEN; *cp == ' ' || *cp == '\t';
cp++)
@@ -122,8 +118,6 @@ exec_script_makecmds(struct proc *p, str
/* collect the shell name; remember its length for later */
shellname = cp;
shellnamelen = 0;
- if (*cp == '\0')
- goto check_shell;
for ( /* cp = cp */ ; *cp != '\0' && *cp != ' ' && *cp != '\t'; cp++)
shellnamelen++;
if (*cp == '\0')
@@ -142,9 +136,8 @@ exec_script_makecmds(struct proc *p, str
* behaviour.
*/
shellarg = cp;
- for ( /* cp = cp */ ; *cp != '\0'; cp++)
- shellarglen++;
- *cp++ = '\0';
+ shellarglen = strlen(shellarg);
+ cp += shellarglen + 1;

check_shell:
/*

Michael McConville

2016-01-11 07:40:02 UTC

Permalink

Post by Michael McConville

Post by Maxim Pugachev
In a case when the shell name is not specified (i.e. just "#!" without
a path), don't run the heavy logic that checks shell, simply return
ENOENT.

I'm not sure whether this is a reasonable thing to do. Someone with more
kernel API experience will have to comment.

Post by Maxim Pugachev
Also, as a tiny improvement: avoid a loop when calculating shell's args length.

I just remembered that this was probably a perfomance concern as well.
In that case, we could assign shellarg and shellarglen earlier in the
function, right?

Post by Michael McConville
Index: sys/kern/exec_script.c
===================================================================
RCS file: /cvs/src/sys/kern/exec_script.c,v
retrieving revision 1.37
diff -u -p -r1.37 exec_script.c
--- sys/kern/exec_script.c 31 Dec 2015 18:55:33 -0000 1.37
+++ sys/kern/exec_script.c 10 Jan 2016 01:41:55 -0000
@@ -67,9 +67,9 @@
int
exec_script_makecmds(struct proc *p, struct exec_package *epp)
{
- int error, hdrlinelen, shellnamelen, shellarglen;
+ int error, hdrlinelen, shellnamelen, shellarglen = 0;
char *hdrstr = epp->ep_hdr;
- char *cp, *shellname, *shellarg, *oldpnbuf;
+ char *cp, *shellname = NULL, *shellarg = NULL, *oldpnbuf;
char **shellargp = NULL, **tmpsap;
struct vnode *scriptvp;
uid_t script_uid = -1;
@@ -110,10 +110,6 @@ exec_script_makecmds(struct proc *p, str
if (cp >= hdrstr + hdrlinelen)
return ENOEXEC;
- shellname = NULL;
- shellarg = NULL;
- shellarglen = 0;
-
/* strip spaces before the shell name */
for (cp = hdrstr + EXEC_SCRIPT_MAGICLEN; *cp == ' ' || *cp == '\t';
cp++)
@@ -122,8 +118,6 @@ exec_script_makecmds(struct proc *p, str
/* collect the shell name; remember its length for later */
shellname = cp;
shellnamelen = 0;
- if (*cp == '\0')
- goto check_shell;
for ( /* cp = cp */ ; *cp != '\0' && *cp != ' ' && *cp != '\t'; cp++)
shellnamelen++;
if (*cp == '\0')
@@ -142,9 +136,8 @@ exec_script_makecmds(struct proc *p, str
* behaviour.
*/
shellarg = cp;
- for ( /* cp = cp */ ; *cp != '\0'; cp++)
- shellarglen++;
- *cp++ = '\0';
+ shellarglen = strlen(shellarg);
+ cp += shellarglen + 1;
/*

Michael McConville

2016-02-05 18:06:56 UTC

Permalink

Post by Michael McConville

Post by Maxim Pugachev
In a case when the shell name is not specified (i.e. just "#!" without
a path), don't run the heavy logic that checks shell, simply return
ENOENT.

I'm not sure whether this is a reasonable thing to do. Someone with more
kernel API experience will have to comment.

Post by Maxim Pugachev
Also, as a tiny improvement: avoid a loop when calculating shell's args length.

I just remembered that this was probably a perfomance concern as well.
In that case, we could assign shellarg and shellarglen earlier in the
function, right?

Is anyone willing to work with me on this? This cleanup should obviously
be done very carefully, but I think it's worth doing because this code
is... unfortunate. I'd be happy to submit and review in smaller chunks.

Post by Michael McConville

Mark Kettenis

2016-02-06 15:48:26 UTC

Permalink

Date: Fri, 5 Feb 2016 13:06:56 -0500

Post by Michael McConville

Post by Maxim Pugachev
In a case when the shell name is not specified (i.e. just "#!" without
a path), don't run the heavy logic that checks shell, simply return
ENOENT.

I'm not sure whether this is a reasonable thing to do. Someone with more
kernel API experience will have to comment.

Post by Maxim Pugachev
Also, as a tiny improvement: avoid a loop when calculating shell's args length.

I just remembered that this was probably a perfomance concern as well.
In that case, we could assign shellarg and shellarglen earlier in the
function, right?

What bug is this fixing? I'm really not interested in diffs that are
basically just churn.

Post by Michael McConville

Michael McConville

2016-02-06 18:49:56 UTC

Permalink

Post by Mark Kettenis

Date: Fri, 5 Feb 2016 13:06:56 -0500

Post by Michael McConville

Post by Maxim Pugachev
In a case when the shell name is not specified (i.e. just "#!" without
a path), don't run the heavy logic that checks shell, simply return
ENOENT.

I'm not sure whether this is a reasonable thing to do. Someone with more
kernel API experience will have to comment.

Post by Maxim Pugachev
Also, as a tiny improvement: avoid a loop when calculating shell's
args length.

I just remembered that this was probably a perfomance concern as well.
In that case, we could assign shellarg and shellarglen earlier in the
function, right?

What bug is this fixing? I'm really not interested in diffs that are
basically just churn.

No bug. It just seems unfortunate to have code like this in sys/kern.
Dead ops, manually inlined strlen() calls, etc. Not a big deal, though.
I'll drop it.

Martin Natano

2016-02-06 15:15:29 UTC

Permalink

I think that 'cp += shellarglen + 1;' is a dead store. The 'cp' variable
is not used anymore after that line.

Either way your diff reads fine to me!

natano

Post by Maxim Pugachev
Index: sys/kern/exec_script.c
===================================================================
RCS file: /cvs/src/sys/kern/exec_script.c,v
retrieving revision 1.37
diff -u -p -r1.37 exec_script.c
--- sys/kern/exec_script.c 31 Dec 2015 18:55:33 -0000 1.37
+++ sys/kern/exec_script.c 10 Jan 2016 01:41:55 -0000
@@ -67,9 +67,9 @@
int
exec_script_makecmds(struct proc *p, struct exec_package *epp)
{
- int error, hdrlinelen, shellnamelen, shellarglen;
+ int error, hdrlinelen, shellnamelen, shellarglen = 0;
char *hdrstr = epp->ep_hdr;
- char *cp, *shellname, *shellarg, *oldpnbuf;
+ char *cp, *shellname = NULL, *shellarg = NULL, *oldpnbuf;
char **shellargp = NULL, **tmpsap;
struct vnode *scriptvp;
uid_t script_uid = -1;
@@ -110,10 +110,6 @@ exec_script_makecmds(struct proc *p, str
if (cp >= hdrstr + hdrlinelen)
return ENOEXEC;
- shellname = NULL;
- shellarg = NULL;
- shellarglen = 0;
-
/* strip spaces before the shell name */
for (cp = hdrstr + EXEC_SCRIPT_MAGICLEN; *cp == ' ' || *cp == '\t';
cp++)
@@ -122,8 +118,6 @@ exec_script_makecmds(struct proc *p, str
/* collect the shell name; remember its length for later */
shellname = cp;
shellnamelen = 0;
- if (*cp == '\0')
- goto check_shell;
for ( /* cp = cp */ ; *cp != '\0' && *cp != ' ' && *cp != '\t'; cp++)
shellnamelen++;
if (*cp == '\0')
@@ -142,9 +136,8 @@ exec_script_makecmds(struct proc *p, str
* behaviour.
*/
shellarg = cp;
- for ( /* cp = cp */ ; *cp != '\0'; cp++)
- shellarglen++;
- *cp++ = '\0';
+ shellarglen = strlen(shellarg);
+ cp += shellarglen + 1;
/*