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.
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 29 Dec 2015 01:56:11 -0000
@@ -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;
/*
