Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unbalanced PROTECT/UNPROTECT in the Shelter class #935

Closed
steve-s opened this issue Jan 31, 2019 · 4 comments
Closed

Unbalanced PROTECT/UNPROTECT in the Shelter class #935

steve-s opened this issue Jan 31, 2019 · 4 comments

Comments

@steve-s
Copy link

steve-s commented Jan 31, 2019

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

oracle/fastr#53
oracle/fastr#50

@eddelbuettel
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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

@eddelbuettel
Copy link
Member

Indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants