Category: C

CRAN reports UB in your R-package: How to fix it?

BobJansen Leave a comment

CRAN automatically checks C/C++ source code packages for Undefined Behaviour (UB). If the automated checks find UB it might result in the removal of the package from CRAN. This is a good thing, the presence of UB is a potential source of bugs and has no positive side effects. These checks have been improving and found scrypt package actually had some UB which needed to be fixed. This scared me a bit since although I’m a maintainer since the package contains some relatively complicated code which I didn’t write or change. A bit of history: Colin Percival conceived and implemented the algorithm in C and Andrew Kipp ported his code to R. I only revived the package after improved automated checks found problems that needed to be solved.

Reproducing the issue was my first challenge. My machine runs Ubuntu LTS and doesn’t have all the fancy tools installed to detect the UB. If I’m not able to reproduce the issue I can’t know whether I fixed it either so reproducing seemed like a good first step. I asked on Twitter and Dirk Eddelbuettel pointed me to the repo of a Docker image purpose made for this kind of analysis by Winston Chang. The instructions are worth reading but this is all I needed:

docker run --rm -ti --security-opt seccomp=unconfined -v $(pwd):/rscrypt wch1/r-debug

This downloads the docker image if you don’t have it yet and starts it with the current directory mounted to /rscrypt inside the container. Within the container you can access versions of R with extra instrumentation. For detecting the UB two versions are available: RDsan (san is shorthand for sanitizer) which is compiled with gcc and RDcsan which is compiled with clang. So now I could do (some output elided):

R> RDcsan CMD INSTALL scrypt_0.1.3.tar.gz  # The last version with the problem
R> scrypt::hashPassword("password")
scrypt-1.1.6/lib/crypto/sha256.c:254:24: runtime error: null pointer passed as argument 2, which is declared to never be null
/usr/include/string.h:44:28: note: nonnull attribute specified here
    #0 0x7f21fe3bde7f in scrypt_SHA256_Update /tmp/.../sha256.c:254:3
    #1 0x7f21fe42b1e6 in scrypt_HMAC_SHA256_Update /tmp/.../sha256.c:335:2
    #2 0x7f21fe42b6a1 in PBKDF2_SHA256 /tmp/../sha256.c:377:2
    #3 0x7f21fe42c9e6 in crypto_scrypt /tmp/.../crypto_scrypt-ref.c:258:2
    #4 0x7f21fe46f822 in getcpuperf(double*) /tmp/.../util.cpp:145:13
    #5 0x7f21fe46420c in (anonymous namespace)::getparams(double, double, int*, unsigned int*, unsigned int*) /tmp/.../scrypt.cpp:49:15
    #6 0x7f21fe462f81 in hashPassword(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, double, double) /tmp/.../scrypt.cpp:129:15
    #7 0x7f21fe439c72 in _scrypt_hashPassword /tmp/.../RcppExports.cpp:17:34
...
etc

The C code of the scrypt package is entered at #7.

The CRAN error report shows that UB was with R compiled by using both gcc and clang but for some reason when I tried the issue was only flagged by RDcsan. The important thing is the error can be reproduced and a possible fix can be validated.

In this particular case the error was easy to find. The code that triggers the UB is the following memcpy() and from the error message we know that the src argument is the culprit:

memcpy(&ctx->buf[r], src, len);

Tracing through the call stack I found that the src in this case is the salt used when figuring out how many iterations scrypt should perform to perform reasonably well on the current machine. More importantly, for this test the hash has the value NULL. This explains the error found: memcpy() can’t copy anything from NULL.

With the root cause identified, the solution is simple: set the initial salt to an actual value, since this code is only called when checking the CPU performance and since the old behaviour is UB anyway it doesn’t really make a difference what value is used. I choose to set it to all zeroes. After that, the UB disappeared, a commit was pushed and a submission to CRAN was accepted.

Lessons learned

  1. CRAN has been checking packages for UB since 2014 and the checks are still improving, great!
  2. Finding UB like this requires help from good tooling which isn’t as easily available as R and CRAN
  3. Luckily, smart people have figured out a way to get this tooling on your computer in a few minutes
  4. Once I knew were to look, the problem was easily solved