Michael McConville wrote:
> Michael McConville wrote:
> > > On Sun, Dec 13, 2015 at 9:45 PM, Maxim Pugachev <[email protected]>
> > > 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 -0000 1.36
+++ sys/kern/exec_script.c 30 Dec 2015 06:53:38 -0000
@@ -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;
/*
