[Bioc-devel] PROTECT errors in Bioconductor packages

[Michael Lawrence]

On Thu, Apr 6, 2017 at 2:59 AM, Martin Morgan
<martin.morgan at roswellpark.org> wrote:
> On 04/06/2017 05:33 AM, Aaron Lun wrote:
>>>
>>> The tool is not perfect, so assess each report carefully.
>>
>>
>> I also get a warning on almost every C++ function I've written, because
>> I use the following code to handle exceptions:
>>
>>      SEXP output=PROTECT(allocVector(...));
>>      try {
>>          // do something that might raise an exception
>>      } catch (std::exception& e) {
>>          UNPROTECT(1);
>>          throw; // break out of this part of the function
>>      }
>>      UNPROTECT(1);
>>      return output;
>>
>> Presumably the check doesn't account for transfer of control to the
>> catch block. I find that R itself is pretty good at complaining about
>> stack imbalances during execution of tests, examples, etc.
>>
>>> 'My' packages
>>> (Rsamtools, DirichletMultinomial) had several false positives (all
>>> associated with use of an attribute of a protected SEXP), one subtle
>>> problem (a symbol from a PROTECT'ed package name space; the symbol could
>>> in theory be an active binding and the value obtained not PROTECTed by
>>> the name space), and a genuine bug
>>>
>>>                 tag = NEW_CHARACTER(n);
>>>                 for (int j = 0; j < n; ++j)
>>>                     SET_STRING_ELT(tag, j, NA_STRING);
>>>                 if ('A' == aux[0]) {
>>>                     buf_A = R_alloc(2, sizeof(char));  # <<- bug
>>>                     buf_A[1] = '\0';
>>>                 }
>>>                 ...
>>>                 SET_VECTOR_ELT(tags, i, tag); # PROTECT tag, too late!
>>
>>
>> I assume the bug refers to the un-PROTECT'd NEW_CHARACTER here - the
>> R_alloc call looks okay to me...
>
>
> yes, tag needs protection.
>
> I attributed the bug to R_alloc because I failed to reason that R_alloc
> (obviously) allocates and hence can trigger a garbage collection.
>
> Somehow it reflects my approach to PROTECTion, probably not shared by
> everyone. I like to PROTECT only when necessary, rather than
> indiscriminately. Probably this has no practical consequence in terms of
> performance, making the code a little easier to read at the expense of
> exposing me to bugs like this.
>

I guess it's a tradeoff between syntactic complexity and logical
complexity. You have to think pretty hard to minimize use of the
protect stack.

One recommendation might be to UNPROTECT() as soon as the pointer on
the top is unneeded, rather than trying to figure out the number to
pop just before returning to R.

One thing that got me is that the order in which C evaluates function
call arguments is undefined. I did a lot of R_setAttrib(x,
install("foo"), allocBar()), thinking that the symbol would be
automatically protected, and allocBar() would not need protection,
since it happened last. Unfortunately, it might be evaluated first.

Btw, I think my package RGtk2 got the record: 1952 errors. Luckily
almost all of them happened inside a few macros and autogenerated
code.

> Martin
>
>>
>> Cheers,
>>
>> Aaron
>> _______________________________________________
>> Bioc-devel at r-project.org mailing list
>> https://stat.ethz.ch/mailman/listinfo/bioc-devel
>>
>
>
> This email message may contain legally privileged and/or...{{dropped:2}}
>
> _______________________________________________
> Bioc-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/bioc-devel