Re: [patch] kern/exec_script: avoid invalid free() in a case of error

[Ted Unangst]

Michael McConville wrote:
> Michael McConville wrote:
> > Michael McConville wrote:
> > > > On Sun, Dec 13, 2015 at 9:45 PM, Maxim Pugachev  
> > > > wrote:
> > > > > Hi,
> > > > >
> > > > > In exec_script_makecmds function, when EXEC_HASFD flag was set, but
> > > > > copystr/copyinstr returns an error, we need to set *tmpsap to NULL to
> > > > > terminate a loop (under "fail" label) correctly.
> > > 
> > > I spent a while straining to see the forest through the trees, but this
> > > makes sense to me. ok mmcc@
> > > 
> > > Is this allocation chain idiom used elsewhere in sys/kern? Maybe we
> > > could break it out into ~3 functions. The current state of affairs seems
> > > bug-prone.
> > 
> > It seems that all possible paths through this nested condition increment
> > tmpsap. Why not just increment it afterward so no one else ends up with
> > the headache I now have?
> > 
> > As always: I could be misinterpreting this.
> 
> tedu and gsoares pointed out the nop expression in my last diff. I guess
> I was moving too fast...
> 
> Here's a new one:

this looks better to me.

Re: [patch] kern/exec_script: avoid invalid free() in a case of error

[Michael McConville]

Michael McConville wrote:
> Michael McConville wrote:
> > > On Sun, Dec 13, 2015 at 9:45 PM, Maxim Pugachev  
> > > wrote:
> > > > Hi,
> > > >
> > > > In exec_script_makecmds function, when EXEC_HASFD flag was set, but
> > > > copystr/copyinstr returns an error, we need to set *tmpsap to NULL to
> > > > terminate a loop (under "fail" label) correctly.
> > 
> > I spent a while straining to see the forest through the trees, but this
> > makes sense to me. ok mmcc@
> > 
> > Is this allocation chain idiom used elsewhere in sys/kern? Maybe we
> > could break it out into ~3 functions. The current state of affairs seems
> > bug-prone.
> 
> It seems that all possible paths through this nested condition increment
> tmpsap. Why not just increment it afterward so no one else ends up with
> the headache I now have?
> 
> As always: I could be misinterpreting this.

tedu and gsoares pointed out the nop expression in my last diff. I guess
I was moving too fast...

Here's a new one:


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  30 Dec 2015 06:53:38 -
@@ -208,24 +208,25 @@ check_shell:
 #if NSYSTRACE > 0
if (ISSET(p->p_flag, P_SYSTRACE)) {
error = systrace_scriptname(p, *tmpsap);
-   if (error == 0)
-   tmpsap++;
-   else
+   if (error != 0)
/*
 * Since systrace_scriptname() provides a
 * convenience, not a security issue, we are
 * safe to do this.
 */
-   error = copystr(epp->ep_name, *tmpsap++,
+   error = copystr(epp->ep_name, *tmpsap,
MAXPATHLEN, NULL);
} else
 #endif
-   error = copyinstr(epp->ep_name, *tmpsap++, MAXPATHLEN,
+   error = copyinstr(epp->ep_name, *tmpsap, MAXPATHLEN,
NULL);
-   if (error != 0)
+   if (error != 0) {
+   *(tmpsap + 1) = NULL;
goto fail;
+   }
} else
-   snprintf(*tmpsap++, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
+   snprintf(*tmpsap, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
+   tmpsap++;
*tmpsap = NULL;
 
/*

Re: [patch] kern/exec_script: avoid invalid free() in a case of error

[Michael McConville]

> On Sun, Dec 13, 2015 at 9:45 PM, Maxim Pugachev  wrote:
> > Hi,
> >
> > In exec_script_makecmds function, when EXEC_HASFD flag was set, but
> > copystr/copyinstr returns an error, we need to set *tmpsap to NULL to
> > terminate a loop (under "fail" label) correctly.

I spent a while straining to see the forest through the trees, but this
makes sense to me. ok mmcc@

Is this allocation chain idiom used elsewhere in sys/kern? Maybe we
could break it out into ~3 functions. The current state of affairs seems
bug-prone.

> > 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  13 Dec 2015 18:33:53 -
> > @@ -222,8 +222,10 @@ check_shell:
> >  #endif
> > error = copyinstr(epp->ep_name, *tmpsap++, 
> > MAXPATHLEN,
> > NULL);
> > -   if (error != 0)
> > +   if (error != 0) {
> > +   *tmpsap = NULL;
> > goto fail;
> > +   }
> > } else
> > snprintf(*tmpsap++, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
> > *tmpsap = NULL;
> 

Re: [patch] kern/exec_script: avoid invalid free() in a case of error

[Michael McConville]

Michael McConville wrote:
> > On Sun, Dec 13, 2015 at 9:45 PM, Maxim Pugachev  
> > wrote:
> > > Hi,
> > >
> > > In exec_script_makecmds function, when EXEC_HASFD flag was set, but
> > > copystr/copyinstr returns an error, we need to set *tmpsap to NULL to
> > > terminate a loop (under "fail" label) correctly.
> 
> I spent a while straining to see the forest through the trees, but this
> makes sense to me. ok mmcc@
> 
> Is this allocation chain idiom used elsewhere in sys/kern? Maybe we
> could break it out into ~3 functions. The current state of affairs seems
> bug-prone.

It seems that all possible paths through this nested condition increment
tmpsap. Why not just increment it afterward so no one else ends up with
the headache I now have?

As always: I could be misinterpreting this.


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  29 Dec 2015 01:56:11 -
@@ -209,23 +209,26 @@ check_shell:
if (ISSET(p->p_flag, P_SYSTRACE)) {
error = systrace_scriptname(p, *tmpsap);
if (error == 0)
-   tmpsap++;
+   tmpsap;
else
/*
 * Since systrace_scriptname() provides a
 * convenience, not a security issue, we are
 * safe to do this.
 */
-   error = copystr(epp->ep_name, *tmpsap++,
+   error = copystr(epp->ep_name, *tmpsap,
MAXPATHLEN, NULL);
} else
 #endif
-   error = copyinstr(epp->ep_name, *tmpsap++, MAXPATHLEN,
+   error = copyinstr(epp->ep_name, *tmpsap, MAXPATHLEN,
NULL);
-   if (error != 0)
+   if (error != 0) {
+   *(tmpsap + 1) = NULL;
goto fail;
+   }
} else
-   snprintf(*tmpsap++, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
+   snprintf(*tmpsap, MAXPATHLEN, "/dev/fd/%d", epp->ep_fd);
+   tmpsap++;
*tmpsap = NULL;
 
/*