Bug 15025 - GC protection gaps in do_system() (sys-unix.c)
GC protection gaps in do_system() (sys-unix.c)
Status: CLOSED FIXED
Product: R
Classification: Unclassified
Component: Low-level
R 2.15.1
Other Linux
: P5 minor
Assigned To: R-core
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-22 08:56 UTC by Andrew Runnalls
Modified: 2012-08-22 17:48 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Runnalls 2012-08-22 08:56:31 UTC
Towards the end of do_system() at current rev 59107 and earlier, we have:

	rval = allocVector(STRSXP, i);
	for (j = (i - 1); j >= 0; j--) {
	    SET_STRING_ELT(rval, j, CAR(tlist));
	    tlist = CDR(tlist);
	}
	if(res) {
	    setAttrib(rval, install("status"), ScalarInteger(res));
	    if(errno)
		setAttrib(rval, install("errmsg"), mkString(strerror(errno)));
	}
	UNPROTECT(1);
	return rval;

Here rval is vulnerable to allocations within the if(res) block, and needs to be PROTECTed.  Similarly, these allocations within the if(res) block are vulnerable to each other (in a way that depends on the order of argument evaluation).  Here's a possible reworking:

	PROTECT(rval = allocVector(STRSXP, i));
	for (j = (i - 1); j >= 0; j--) {
	    SET_STRING_ELT(rval, j, CAR(tlist));
	    tlist = CDR(tlist);
	}
	if(res) {
	    SEXP sstatus, sres;
	    PROTECT(sstatus = install("status"));
	    PROTECT(sres = ScalarInteger(res));
	    setAttrib(rval, sstatus, sres);
	    if(errno) {
		SEXP serrmsg, serrno;
		PROTECT(serrmsg = install("errmsg"));
		PROTECT(serrno = mkString(strerror(errno)));
		setAttrib(rval, serrmsg, serrno);
		UNPROTECT(2);
	    }
	    UNPROTECT(2);
	}
	UNPROTECT(2);
	return rval;

(Discovered during CXXR development.)

Andrew
Comment 1 Simon Urbanek 2012-08-22 15:28:58 UTC
Andrew, thanks - the rval is the only PROTECT needed there (attributes are protected by the contained object and symbols don't need protection). But, please, in the future use svn diff or be more specific as there are several different do_system in different files so chasing them is a bit annoying.
Comment 2 Andrew Runnalls 2012-08-22 16:45:12 UTC
(In reply to comment #1)
> Andrew, thanks - the rval is the only PROTECT needed there (attributes are
> protected by the contained object and symbols don't need protection). But,
> please, in the future use svn diff or be more specific as there are several
> different do_system in different files so chasing them is a bit annoying.

Simon, you are of course right about symbols.  But on my platform at least, C function arguments are evaluated from right to left, so for example in

setAttrib(rval, install("status"), ScalarInteger(res));

the compiler first evaluates ScalarInteger(res), and then evaluates install("status").  If this install gives rise to a garbage collection, then the ScalarInteger will be blown away *before* it has been attached as an attribute to rval. (Which is precisely what happens in CXXR.)  The same thing applies to the setting of the errmsg attribute.

As to which do_system() I meant, well the subject line did say sys-unix.c :-)

Andrew
Comment 3 Simon Urbanek 2012-08-22 17:48:51 UTC
Please see the fix `svn diff -r 60369:60370` - it may clarify your concerns. If you need further assistance, please contact me directly by e-mail.