Rcpp: Unbalanced PROTECT/UNPROTECT in the Shelter class

[steve-s]

It seems that Shelter may call UNPROTECT with higher n than the number of PROTECT calls it issued because it increments the nprotected member even for NILSXPs, which are however ignored in Rcpp_protect. This does not cause a crash on GNU-R for some reason, but if there are some preexisting SEXPs on the protection stack, it may pop them out unintentionally.

Suggested patch:

--- a/inst/include/Rcpp/protection/Shelter.h
+++ b/inst/include/Rcpp/protection/Shelter.h
@@ -26,7 +26,7 @@ namespace Rcpp {
         Shelter() : nprotected(0){}

         inline SEXP operator()(SEXP x){
-            nprotected++;
+           if ( x != R_NilValue ) nprotected++;
             return Rcpp_protect(x) ;
         }

the same measure is already taken in Shield:

        Shield( SEXP t_) : t(Rcpp_protect(t_)){}
        ~Shield(){
            if( t != R_NilValue ) Rcpp_unprotect(1) ;
        }

Some code snippets that trigger this issue were reported to FastR, but as noted above, they do not cause any observable issues on GNU-R

https://github.com/oracle/fastr/issues/53
https://github.com/oracle/fastr/issues/50

[eddelbuettel]

Thanks for filing the issue -- that looks plausible. Shelter is somewhat mature code just like Shield but less often used so with (ahem) CRAN _et al_ as our unit tests that may not been triggered.

I can make the change and launch a rev.dep check so see if anything bubbles up.

[eddelbuettel]

I had some issues with the reverse-depends check environment which, to cut a long story short, needed to be rebuilt. I am almost done and nothing has come up so I'll make this change 'real soon now'.

[coatless]

@eddelbuettel this can be closed out as PR #940 was merged in.

[eddelbuettel]

Indeed.