[R-pkg-devel] Notes from rchk on hdf5r package

[Kevin Ushey]

Using this line of code as an example:

    SEXP hdf5r_ns = PROTECT(eval(lang2(install("getNamespace"),
mkString("hdf5r")), R_GlobalEnv));

The issue is fairly nuanced. There are a few main things to note
before we can understand the issue:

    1. The order of evaluation of arguments in C is unspecified, and
so arguments might be evaluated in any order depending on what the
compiler decides to do,
    2. `mkString()`, like other R APIs, creates an unprotected vector,
    3. `install()` can evaluate, if the symbol "getNamespace" has not
yet been interned into R's symbol table.

That implies that the following could potentially happen:

    1. First, an (unprotected!) R object is created by 'mkString("hdf5r")',
    2. Next, the invocation of `install("getNamespace")` causes an
allocation in R,
    3. That allocation triggers the garbage collector,
    4. The string created in 1. is erroneously collected.

While normally the SEXP created by `mkString()` would implicitly be
protected because it lives within the language object created by
`lang2()`, this doesn't happen in time since `install()` is evaluated
before `lang2()`.

So your intuition is correct: you should protect the result of
`mkString()` before putting it into the list.

For the second case:

    eval(lang3(install("set_ref.H5R"), result, _Robj), hdf5r_ns);

`eval()` does not protect its arguments, and `lang3()` creates an
unprotected SEXP. This implies it could be garbage collected during
execution. This would imply that calls like `sys.calls()`, which
attempt to inspect the set of calls, could break (as the SEXP used for
evaluation of that call has since been cleaned up).

In other words, you should protect language objects before evaluating them.

Best,
Kevin

On Sun, Sep 30, 2018 at 9:10 AM Holger Hoefling <hhoeflin using gmail.com> wrote:
>
> Hi all,
>
> I have been submitting a bug-fix version of a package and was alerted
> to some notes that rchk brought up for it.
>
> A link to the notes
>
> https://raw.githubusercontent.com/kalibera/cran-checks/master/rchk/results/hdf5r.out
>
> and to the version of the code the package refers to
>
> https://github.com/cran/hdf5r/blob/master/src/convert.c
>
>
> To be more specific:
>
> The errors are more specific about usage of Rf_lang2 and Rf_lang1. I
> am not very familiar with them and would appreciate some guidance on
> how to fix.
>
> Example
>
> Suspicious call (two or more unprotected arguments) to Rf_lang2 at
> H5ToR_Post_REFERENCE hdf5r/src/convert.c:1140
>
> Code: SEXP hdf5r_ns = PROTECT(eval(lang2(install("getNamespace"), mkString("
> hdf5r")), R_GlobalEnv));
>
> I assume this can be fixed by wrapping the "install" and "mkString" in
> an additional PROTECT? Is this what it is complaining about?
>
>
> The other:
>
> calling allocating function Rf_eval with argument allocated using
> Rf_lang3(S:set_ref.H5R,?,?) hdf5r/src/convert.c:1141
>
> Code:
>
> eval(lang3(install("set_ref.H5R"), result, _Robj), hdf5r_ns);
>
> I remember cobbling these parts together off of the internet and don't
> understand enough about the internals of R functions to know what is
> going wrong and how to fix.
>
>
> Any help anyone can offer would be appreciated.
>
>
> Thank you!
>
>
> Holger
>
>         [[alternative HTML version deleted]]
>
> ______________________________________________
> R-package-devel using r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-package-devel