[Rd] Pb with defineVar() example in the "Writing R Extensions" manual

[Peter Dalgaard]

Luke Tierney wrote:
> On Wed, 16 Jan 2008, Peter Dalgaard wrote:
>
>> Herve Pages wrote:
>> Hi Peter,
>>
>> Peter Dalgaard wrote:
>>
>>> Herve Pages wrote:
>>>
>>>> Hi,
>>>>
>>>> I'm wondering if this code from the "Writing R Extensions" manual
>>>> is really safe:
>>>>
>>>>      SEXP mkans(double x)
>>>>      {
>>>>          SEXP ans;
>>>>          PROTECT(ans = allocVector(REALSXP, 1));
>>>>          REAL(ans)[0] = x;
>>>>          UNPROTECT(1);
>>>>          return ans;
>>>>      }
>>>>
>>>>      double feval(double x, SEXP f, SEXP rho)
>>>>      {
>>>>          defineVar(install("x"), mkans(x), rho);
>>>>          return(REAL(eval(f, rho))[0]);
>>>>      }
>>>>
>>>> In C, the order in which function arguments are evaluated before the
>>>> function itself is called is undefined. Hence there is no guarantee
>>>> that install("x") will be evaluated before mkans(x). What happens if
>>>> mkans(x) is evaluated first? Then install("x") will be called and
>>>> eventually trigger garbage collection while the SEXP returned by
>>>> mkans(x) is still unprotected.
>>>>
>>>> I'm asking because I'm getting all sorts of problems with
>>>>
>>>>   defineVar(install(somekey), mkans(x), rho);
>>>>
>>>> In my code this line is inside a big loop (hundred of thousands of
>>>> iterations) so I end up with a lot of symbols in the rho environment.
>>>>
>>>> The problems I've seen are hard to reproduce: sometimes it's a
>>>> segfault,
>>>> sometimes a "cons memory exhausted" error, or sometimes everything
>>>> looks
>>>> fine except that, later, when I retrieve values from the rho
>>>> environment
>>>> with findVar(), some of them are altered!
>>>>
>>>> But if I replace the above line by:
>>>>
>>>>   PROTECT(ans = mkans(x));
>>>>   defineVar(install(somekey), ans, rho);
>>>>   UNPROTECT(1);
>>>>
>>>> then everything works fine :-)
>>>>
>>>>
>>>>
>>> Sounds like you are right. You don't really have the "smoking gun", but
>>> it doesn't seem to be worth trying to catch the actual bug in action
>>> with hardware watchpoints and whatnot.
>>>
>>> The opposite fix should work too (does it?):
>>>
>>> { SEXP sym = install(somekey) ; defineVar(sym, mkans(x), rho);}
>>>
>>
>> So now you are protected against install(somekey) eventually triggering
>> garbage collection but you are still not protected against
>> defineVar() itself
>> triggering garbage collection. Maybe defineVar() does not do that,
>> and will
>> never do it, but isn't it risky to rely on this kind of assumption?
>>
>> Thanks!
>> H.
>>
> That's not the problem you raised (argument evaluation order), but
> there's a CONS inside defineVar, and as far as I can see, it doesn't
> protect its arguments, so you could well be right.

> CONS itself protects its arguments though.
>
> luke
>
Ah, right! (and one of them is "value"). That explains why we haven't
seen crashes left right and center from this....

How about the hashed environment case? R_Newhashpjw and R_HashSet
wouldn't need allocation, and R_HashResize is after the assignment, so I
suppose it is OK.

We should make this sort ot convention more explicit somehow.


-- 
   O__  ---- Peter Dalgaard             Øster Farimagsgade 5, Entr.B
  c/ /'_ --- Dept. of Biostatistics     PO Box 2099, 1014 Cph. K
 (*) \(*) -- University of Copenhagen   Denmark          Ph:  (+45) 35327918
~~~~~~~~~~ - (p.dalgaard at biostat.ku.dk)                  FAX: (+45) 35327907