Fix segfault when handling trap #603
Conversation
|
I was not able to reproduce this bug on fedora, but it's easily reproducible on RHEL. |
|
Patch has been taken from https://src.fedoraproject.org/rpms/ksh/blob/master/f/ksh-20120801-trapcom.patch |
| nsig += sizeof(char *); | ||
| memcpy(savsig = malloc(nsig), (char *)&shp->st.trapcom[0], nsig); | ||
| nsig = shp->st.trapmax; | ||
| if (nsig > 0 || shp->st.trapcom[0]) { |
There was a problem hiding this comment.
I know you're just copying the original statement but now is the time to think about whether it is correct. Shouldn't it be impossible for st.trapmax to be zero yet a trap be defined? Shouldn't this just be if (nsig)?
There was a problem hiding this comment.
I think that's why shp->st.trapcom[0] is being checked here.
There was a problem hiding this comment.
I admit I haven't looked closely at the code to understand how st.trapmax is used. If it is the maximum offset into the st.trapcom array then it should be set to -1 rather than zero if the array is empty. In which case the check of st.trapcom[0] isn't needed. Regardless, it would be better would be to redefine it as st.trapcount so that zero means there are no traps.
In any case ignore this rant. I'm just pointing out how stupid and confusing the existing code is about such matters. The existing pattern pretty much guarantees bugs will be present in any code that manipulates shell traps. Which was why I was suggesting expanding the scope of the change to fix the core problem.
src/cmd/ksh93/sh/xec.c
Outdated
| @@ -1774,7 +1774,8 @@ int sh_exec(Shell_t *shp, const Shnode_t *t, int flags) { | |||
| struct checkpt *buffp = | |||
| (struct checkpt *)stkalloc(shp->stk, sizeof(struct checkpt)); | |||
| shp->st.otrapcom = 0; | |||
| if ((nsig = shp->st.trapmax * sizeof(char *)) > 0 || shp->st.trapcom[0]) { | |||
| nsig = shp->st.trapmax * sizeof(char *); | |||
There was a problem hiding this comment.
As you did in subshell.c the multiplication by sizeof(char *) should be deferred to the actual malloc call.
| @@ -3339,9 +3348,16 @@ int sh_funscope_20120720(Shell_t *shp, int argn, char *argv[], int (*fun)(void * | |||
| shp->topscope = (Shscope_t *)prevscope; | |||
| nv_getval(sh_scoped(shp, IFSNOD)); | |||
| shp->end_fn = 0; | |||
| if (nsig) memcpy((char *)&shp->st.trapcom[0], savstak, nsig); | |||
| if (nsig) { | |||
There was a problem hiding this comment.
You can omit this since the for loop will execute zero times if nsig is zero.
There was a problem hiding this comment.
Doh! Never mind. Obviously the other two statements need to be predicated on this.
src/cmd/ksh93/sh/xec.c
Outdated
| free(shp->st.trapcom[isig]); | ||
| } | ||
| } | ||
| memcpy((char *)&shp->st.trapcom[0], savsig, nsig * sizeof(char *)); |
There was a problem hiding this comment.
The convoluted addressing and typecasting isn't needed; just memcpy(shp->st.trapcom, ...).
src/cmd/ksh93/sh/xec.c
Outdated
| savsig = malloc(nsig * sizeof(char *)); | ||
| // Contents of shp->st.trapcom may change | ||
| for (isig = 0; isig < nsig; ++isig) { | ||
| savsig[isig] = shp->st.trapcom[isig] == Empty |
There was a problem hiding this comment.
The magic pointer Empty is special-cased here but not in the similar code in subshell.c which seems wrong. I would suggest removing this special-case. Instead change this line in src/cmd/ksh93/bltins/trap.c:
shp->st.trapcom[sig] = Empty;
to
shp->st.trapcom[sig] = strdup("");
Then all of the places that manipulate shp->st.trapcom can just assume every entry was dynamically allocated.
Resolves: rhbz#1117404
|
LGTM |
Resolves: rhbz#1117404