Bug 1460940

I can reproduce with ksh distributed with RHEL-7 and Fedora 27 but not with the latest upstream version of ksh.  We will need to check whether it was fixed on purpose, or as a side effect of other code or configuration changes.

Created attachment 1408847 [details]
Fix a leak in subshell when trap is used

This has been picked up from latest upstream.

Comment on attachment 1408847 [details]
Fix a leak in subshell when trap is used

I did not try to build/run it but the backport looks good.  I think you can remove the 'isig' variable, which becomes unused after applying this patch.  The assignment nsig += sizeof(char *); looks redundant but safe.  It is also not clear why shp->st.otrapcom is assigned.  It does not seem to be read anywhere in the code or am I missing something?

The only place I see it being used is here[1].

[1] https://github.com/att/ast/blob/2012-08-01-master/src/cmd/ksh93/bltins/trap.c#L140

Indeed.  I overlooked this one.  Then we should probably nullify shp->st.otrapcom after calling free(savsig) to avoid a use after free, but not necessarily in this RHEL-7 fix.

Created attachment 1409826 [details]
Fix a leak in subshell when trap is used

Updated patch to remove isig variable.

Created attachment 1436790 [details]
Fix a leak in subshell when trap is used

Comment on attachment 1436790 [details]
Fix a leak in subshell when trap is used

As noted on IRC, I would modify the code to assign NULL immediately after free() to minimize the window where the array references already freed objects.

*** Bug 1577907 has been marked as a duplicate of this bug. ***

Patch posted in comment 19 introduces a regression:

test basic failed at 2019-08-14+13:49:32 with exit code 1 [ 102 tests 1 error ]
test basic(C.UTF-8) begins at 2019-08-14+13:49:32
        basic.sh[257]: traps ignored by parent not ignored
test basic(C.UTF-8) failed at 2019-08-14+13:50:01 with exit code 1 [ 102 tests 1 error ]
test basic(shcomp) begins at 2019-08-14+13:50:01
        shcomp-basic.ksh[257]: traps ignored by parent not ignored

Development Management has reviewed and declined this request. You may appeal this decision by reopening this request.

(In reply to Siteshwar Vashisht from comment #23)
> Patch posted in comment 19 introduces a regression:
> 
> test basic failed at 2019-08-14+13:49:32 with exit code 1 [ 102 tests 1
> error ]
> test basic(C.UTF-8) begins at 2019-08-14+13:49:32
>         basic.sh[257]: traps ignored by parent not ignored
> test basic(C.UTF-8) failed at 2019-08-14+13:50:01 with exit code 1 [ 102
> tests 1 error ]
> test basic(shcomp) begins at 2019-08-14+13:50:01
>         shcomp-basic.ksh[257]: traps ignored by parent not ignored

  The issue should be corrected by reverting ksh-20120801-trapcom.patch
and adding this pseudo patch to sh/subshell.c:

 			nsig += sizeof(char*);
			memcpy(savsig=malloc(nsig),(char*)&shp->st.trapcom[0],nsig);
+			memset((char*)&shp->st.trapcom[0],0,nsig);
 			/* this nonsense needed for $(trap) */
 			shp->st.otrapcom = (char**)savsig;

and equivalent change to sh/xec.c.

  ksh-20120801-trapcom.patch was a rather complex/incorrect correction to #1117404

  The leak happens in sh_sigreset(0) that does set the shp->st.trapcom[X] to
NULL, and only calls free if the mode argument is non zero.

  The upstream code was buggy in that it would save a vector of string pointers,
release the pointers and later restore the vector of string pointers.
  Since sh_sigreset is always called with a zero argument, it means it will always
set the pointers to NULL in sh.st.trapcom[X], calling memset will prevent it from
releasing the pointers in the later call to sh_sigreset(1), *or* to the right thing
at that moment, contrary to ksh-20120801-trapcom.patch.