Valgrind issues on CRAN

[riccardoporreca]

CRAN checks for the released package version 4.20-1 reveals issues with valgrind, which should be investigated and fixed.

Note that a new submission fixing the issue has been requested by the CRAN Team by 2021-01-29 in order to retain the package on CRAN

See comments below for the detailed investigation, which is summarized as follows:

• The issues reported by valgrind are all triggered by tests on errors when parsing an invalid string representation of a TRNG engine, which is pretty much a corner case we don't expect a typical user to hit.
• There is an easy fix that was successfully tested, it is pretty unlikely we can might not be viable to get a new (patched) version of the upstream TRNG C++ library as a new release by Jan 29. As an alternative, we should patch the version we include as part of rTRNG.
  => See Valgrind issues on CRAN #16 (comment) for an explicit and reproducible approach for including a small patch on rTRNG side.
  => See Valgrind issues on CRAN #16 (comment) and following comments regarding a proper fix in TRNG upstream.

Note:

• We should upgrade to the new released TRNG available on GitHub as v4.23 tag, see Upgrade to TRNG 4.23 #18.
  => see below Valgrind issues on CRAN #16 (comment)

See #21 for the consolidation of the upgrade to the patched upstream TRNG v4.23.1 into #20 on top of #18, including the introduction of valgrind checks via GitHub Actions

[riccardoporreca]

The valgrind issues can be reproduced using the Docker-based approach described in https://www.brodieg.com/2018/04/06/adventures-in-r-and-compiled-code/ based on the image wch1/r-debug (see GH repo wch/r-debug).

This is achieved by

docker run --rm -ti -v $(pwd):/mydir wch1/r-debug
# inside the container:
RDvalgrind -e "install.packages(c('Rcpp', 'RcppParallel', 'testthat'))"
RDvalgrind -e "install.packages('https://cran.r-project.org/src/contrib/rTRNG_4.20-1.tar.gz', INSTALL_opts = '--install-tests')"
RDvalgrind -d "valgrind --track-origins=yes" -e "library(testthat); library(rTRNG); testthat::test_package('rTRNG')" &> /mydir/rTRNG.valgrind-4.20-1.txt

=> rTRNG.valgrind-4.20-1.txt, showing very similar output to valgrind output on CRAN

Digging this out, errors all refer to parsing a string representation of a TRNG engine, and actually occur when the string representation does not match what can be parsed (which we capture in stringToRNG(), and test e.g. in test-TRNG.Engine.R).
Therefore, the issue can be reproduced and investigated with minimal example using an interactive R session (launched via RDvalgrind -d "valgrind --track-origins=yes"

yarn2$new("[yarn2 (1 2) (3 4)]") # OK, this is a valid representation
yarn2$new("[yarn2 (1 2) (3 4))") # This is an invalid representation, but no issues are detected
yarn2$new("[yarn2 (1 2) (3 4)") # This is an invalid representation, single issue detected
yarn2$new("!dummy!") # Invalid representation as in the unit tests, many issues detected

The relevant code valgrind refers to is in the delim_c class in trng/utility.hpp, which comes with the upstream TRNG C++ library.

[riccardoporreca]

A possible solution has been considered and investigated, which changes trng/utility.hpp by comparing against the separator conditionally as follows:

char c;
in.get(c);
// avoid comparison if there were parsing issues with other tokens or input is over
if (!in.fail() && !in.eof())
  if (c != d.c)
    in.setstate(std::ios::failbit);

This was done manually in the rTRNG_4.20-1.tar.gz from CRAN => rTRNG_4.20-1-patch-valgrind.tar.gz and tested by replacing the rTRNG installation in the docker container above with

RDvalgrind -e "install.packages('/mydir/rTRNG_4.20-1-patch-valgrind.tar.gz', depedencies = TRUE, INSTALL_opts = '--install-tests')"

Assessed successfully on the miminal examples above, the full run via

RDvalgrind -d "valgrind --track-origins=yes" -e "library(testthat); library(rTRNG); testthat::test_package('rTRNG')" &> /mydir/rTRNG.valgrind-4.20-1-patch-valgrind.txt

shows no other errors => rTRNG_4.20-1-patch-valgrind.txt

[riccardoporreca]

Status after updating to TRNG 4.23 (#18) checked on the corresponding branch as follows:

R CMD build rTRNG
docker run --rm -ti -v $(pwd):/mydir wch1/r-debug
# inside the container:
RDvalgrind -e "install.packages(c('Rcpp', 'RcppParallel', 'testthat'))"
RDvalgrind -e "install.packages('/mydir/rTRNG_4.23-0.9000.tar.gz', INSTALL_opts = '--install-tests')"
RDvalgrind -d "valgrind --track-origins=yes" -e "library(testthat); library(rTRNG); testthat::test_package('rTRNG')" &> /mydir/rTRNG.valgrind-4.23-0.9000.txt

=> rTRNG.valgrind-4.23-0.9000.txt showing the same issue.

[riccardoporreca]

A revised version of the patch mentioned above in #16 (comment) has been applied on top of the 4.23 updates introduced in #18 in a dedicated branch, see commit ed3dbc8 for details:

• To make this reproducible, the patch has been defined as a patch-file inst/tools/patch-delim_c-valgrind.patch
• inst/tools/upgradeTRNG.R has been updated to process the patch file and include a patch component in the TRNG version, as 4.23.1, with an explicit mention in an explicit mention in TRNG.Version() about this being version patched inside rTRNG.

The patched 4.23.1 has been tested from the dedicated branch as follows:

R CMD build rTRNG
docker run --rm -ti -v $(pwd):/mydir wch1/r-debug
# inside the container:
RDvalgrind -e "install.packages(c('Rcpp', 'RcppParallel', 'testthat'))"
RDvalgrind -e "install.packages('/mydir/rTRNG_4.23.1-0.9000.tar.gz', INSTALL_opts = '--install-tests')"
RDvalgrind -d "valgrind --track-origins=yes" -e "library(testthat); library(rTRNG); testthat::test_package('rTRNG')" &> /mydir/rTRNG.valgrind-4.23.1-0.9000.txt

=> rTRNG.valgrind-4.23.1-0.9000.txt shows no issue.

[rabauke]

Should be fixed in TRNG upstream via rabauke/trng4@22cc3b6.

[riccardoporreca]

Thanks a lot @rabauke!

I have included the fix in rabauke/trng4@22cc3b6 as a patch back-ported to the latest TRNG release v4.23 we currently include in rTRNG as follows (for the sake of reproducibility and traceability):

source("inst/tools/upgradeTRNG.R")
patch_file <- file.path(getwd(), "inst", "tools", "fix_uninitialized-memory_read_access-backport-v4.23.patch")
system(paste0("cd ~/GitHubProjects/trng4/ && git diff v4.23..22cc3b6 trng/utility.hpp > ", patch_file))
upgradeTRNG(version = "4.23", patch = patch_file)

This was done with commit 032f559, see in particular fix_uninitialized-memory_read_access-backport-v4.23.patch

Tested as rTRNG 4.23.1-0.9000 via:

R CMD build rTRNG
docker run --rm -ti -v $(pwd):/mydir wch1/r-debug
# inside the container:
RDvalgrind -e "install.packages(c('Rcpp', 'RcppParallel', 'testthat'))"
RDvalgrind -e "install.packages('/mydir/rTRNG_4.23.1-0.9000.tar.gz', INSTALL_opts = '--install-tests')"
RDvalgrind -d "valgrind --track-origins=yes"
# inside the interactive R session
> library(rTRNG); 
> yarn2$new("[yarn2 (1 2) (3 4)]") # OK, this is a valid representation
> yarn2$new("[yarn2 (1 2) (3 4))") # "failed to restore..." expected error, no valgrind issues
> yarn2$new("!dummy!")  # "failed to restore..." expected error, no valgrind issues
> yarn2$new("[yarn2 (1 2) (3 4)")  # "failed to restore..." expected error, no valgrind issues
> q("no")
# full valgrind test as on CRAN
RDvalgrind -d "valgrind --track-origins=yes" -e "library(testthat); library(rTRNG); testthat::test_package('rTRNG')" &> /mydir/rTRNG.valgrind-4.23.1-0.9000_fix_uninitialized-memory_read_access-backport-v4.23.txt

=> rTRNG.valgrind-4.23.1-0.9000_fix_uninitialized-memory_read_access-backport-v4.23.txt

[riccardoporreca]

@rabauke, the test with your fix back-ported to rTRNG confirms the valgrind issues have been resolved, thanks again!

Given that rTRNG is meant to be based on a properly-controlled TRNG release upstream, we have a few options for submitting a new release of rTRNG by Jan 29:

• (A) Use the approach described a few comments above based on the backported rabauke/trng4@22cc3b6, as assessed in the previous comment
• (B) Have a full release of TRNG upstream, where however I fear the timeline might be too tight, especially given the new and possibly ongoing post-v4.23 development in the repo.
• (C) Create a patch release tag for v4.23, e.g. v4.23.1 in TRNG upstream, e.g. as follows:
  • create a branch from tag v4.23
  • back-port the fix by cherry-picking rabauke/trng4@22cc3b6
  • (perhaps update trng.pdf to simply refer to this small patch release)
  • create a tag for the patch release based on the branch
  • => I quickly did this as a PoC in my fork, see branch riccardoporreca/trng4/release/v4.23.1 and release riccardoporreca/trng4@v4.23.1

We would be fine with any of the options above.
Looking forward to hearing your feedback, especially in view of the effort at your end for (B) and, to a minor extent, (C).

[rabauke]

@riccardoporreca See rabauke/trng4@610f783 and https://github.com/rabauke/trng4/tags Do not get confused by the fact that the version number in CMakeLists.txt jumps from 4.22 to 4.23.1. This is just due to the fact that the version number has not been adjusted for 4.23.

[riccardoporreca]

Thank you so much for this @rabauke! I can then fully prepare for the rTRNG submission tomorrow based on the new patch release.

[riccardoporreca]

See #21 for the consolidation of the upgrade to the patched upstream TRNG v4.23.1 into #20 on top of #18