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

[Mark Kettenis]

> Date: Fri, 5 Feb 2016 13:06:56 -0500
> From: Michael McConville 
> 
> Michael McConville wrote:
> > Michael McConville wrote:
> > > Maxim Pugachev wrote:
> > > > 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.
> > > 
> > > > 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.
> > 
> > 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.

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

> > > 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.c31 Dec 2015 18:55:33 -  1.37
> > > +++ sys/kern/exec_script.c10 Jan 2016 01:41:55 -
> > > @@ -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:
> > >   /*
> > > 
> > 
> 
> 

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

[Martin Natano]

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

> 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.c31 Dec 2015 18:55:33 -  1.37
> +++ sys/kern/exec_script.c10 Jan 2016 01:41:55 -
> @@ -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:
>   /*
> 

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

[Michael McConville]

Mark Kettenis wrote:
> > Date: Fri, 5 Feb 2016 13:06:56 -0500
> > From: Michael McConville 
> > 
> > Michael McConville wrote:
> > > Michael McConville wrote:
> > > > Maxim Pugachev wrote:
> > > > > 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.
> > > > 
> > > > > 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.
> > > 
> > > 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.
> 
> 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.

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

[Michael McConville]

Michael McConville wrote:
> Maxim Pugachev wrote:
> > 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.
> 
> > 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.

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?

> 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.c31 Dec 2015 18:55:33 -  1.37
> +++ sys/kern/exec_script.c10 Jan 2016 01:41:55 -
> @@ -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:
>   /*
> 

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

[Michael McConville]

Maxim Pugachev wrote:
> 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.

> 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 -  1.37
+++ sys/kern/exec_script.c  10 Jan 2016 01:41:55 -
@@ -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:
/*

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

[Maxim Pugachev]

Ping?

On Thu, Dec 17, 2015 at 8:18 PM, Ted Unangst  wrote:
> Maxim Pugachev wrote:
>> Ping?
>>
>> On Sun, Dec 13, 2015 at 12:28 AM, Maxim Pugachev  
>> wrote:
>> > 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.
>
> Not forgotten. Just not a very pretty file to look at. :)

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

[Ted Unangst]

Maxim Pugachev wrote:
> Ping?
> 
> On Sun, Dec 13, 2015 at 12:28 AM, Maxim Pugachev  
> wrote:
> > 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.

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

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

[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 -  1.36
+++ sys/kern/exec_script.c  12 Dec 2015 21:18:22 -
@@ -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:
/*