Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[R-package] Protection stack imbalance and unprotected objects #4390

Closed
fabsig opened this issue Jun 18, 2021 · 4 comments · Fixed by #4391
Closed

[R-package] Protection stack imbalance and unprotected objects #4390

fabsig opened this issue Jun 18, 2021 · 4 comments · Fixed by #4391

Comments

@fabsig
Copy link
Contributor

fabsig commented Jun 18, 2021

The R-package has two protection issues. First, there is a protection stack imbalance when functions are exited by returns from the catch statements in the R_API_END macro. This issue is found when running rchk; see the output below. Second, several objects (results of Rf_asChar(.), e.g. here) which should be protected are currently not protected.

Disclaimer: the main credit for finding the causes of these issues goes to @kalibera.

I will make a pull request / proposal with a suggestion for a solution for this.

Below is output when running rchk on the LightGBM CRAN R-package after this commit
42ddff1. Note that the missing protections for Rf_asChar(.) are not found by rchk.

Function LGBM_DatasetCreateFromMat_R
  [PB] has possible protection stack imbalance /rchk/packages/build/JczWIO8n/lightgbm/src/lightgbm_R.cpp:126

Function LGBM_DatasetGetFeatureNames_R
  [PB] has possible protection stack imbalance /rchk/packages/build/JczWIO8n/lightgbm/src/lightgbm_R.cpp:210

Function LGBM_DatasetGetSubset_R
  [PB] has possible protection stack imbalance /rchk/packages/build/JczWIO8n/lightgbm/src/lightgbm_R.cpp:150
Analyzed 39640 functions, traversed 108278 states.
Library name (usually package name): lightgbm
Initialization function: R_init_lightgbm
Functions: 41
Checked call to R_registerRoutines: 1

Rchk version: 3d653b7c8f92dac912532856b55f44d2986c6553
R version: 79889/R Under development (unstable) (2021-01-27 r79889)
LLVM version: 10.0.0
@jameslamb
Copy link
Collaborator

jameslamb commented Jun 18, 2021

Thanks very much for your investigation and write-up! Linking #4282 as there is some overlap between this issue and that one, but this issue adds a lot of good details not contained there.

Could you describe how you ran rchk on {lightgbm}? Ideally a docker run command or something else that would be easy for me to reproduce. I have tried before by following the examples at https://github.com/kalibera/rchk and was not successful (possibly because of some non-standard things we did in {lightgbm}.

Separate from #4391, I'd like to add a continuous integration job that runs rchk so we can detect and prevent such issues in the future.

@fabsig
Copy link
Contributor Author

fabsig commented Jun 18, 2021

Thanks a lot for your feedback.

Could you describe how you ran rchk on {lightgbm}? Ideally a docker run command or something else that would be easy for me to reproduce. I have tried before by following the examples at https://github.com/kalibera/rchk and was not successfully (possibly because of some non-standard things we did in {lightgbm}.

I used docker and followed the steps described here. After creating the CRAN lightgbm_*.tar.gz file with your shell script, I ran the following docker command docker run -v %cd%/packages:/rchk/packages kalibera/rchk:latest /rchk/packages/lightgbm_3.2.1.99.tar.gz

@jameslamb
Copy link
Collaborator

Excellent, thanks very much! For the sake of others looking to this issue interested in reproducing these problems, this set of commands worked for me.

sh build-cran-package.sh
mkdir packages
cp lightgbm_3.2.1.99.tar.gz packages
docker run -v `pwd`/packages:/rchk/packages kalibera/rchk:latest /rchk/packages/lightgbm_3.2.1.99.tar.gz

And that resulted in the following logs

Library name (usually package name): lightgbm
Initialization function: R_init_lightgbm
Functions: 41
Checked call to R_registerRoutines: 1
ERROR: too many states (abstraction error?) in function strptime_internal

Function LGBM_BoosterCreateFromModelfile_R
  [PB] has possible protection stack imbalance /rchk/packages/build/ziRQDfzj/lightgbm/src/lightgbm_R.cpp:368

Function LGBM_BoosterCreate_R
  [PB] has possible protection stack imbalance /rchk/packages/build/ziRQDfzj/lightgbm/src/lightgbm_R.cpp:355

Function LGBM_BoosterDumpModel_R
  [PB] has possible protection stack imbalance /rchk/packages/build/ziRQDfzj/lightgbm/src/lightgbm_R.cpp:718

Function LGBM_BoosterGetEvalNames_R
  [PB] has possible protection stack imbalance /rchk/packages/build/ziRQDfzj/lightgbm/src/lightgbm_R.cpp:521

Function LGBM_BoosterLoadModelFromString_R
  [PB] has possible protection stack imbalance /rchk/packages/build/ziRQDfzj/lightgbm/src/lightgbm_R.cpp:381

Function LGBM_BoosterSaveModelToString_R
  [PB] has possible protection stack imbalance /rchk/packages/build/ziRQDfzj/lightgbm/src/lightgbm_R.cpp:695

Function LGBM_DatasetCreateFromCSC_R
  [PB] has possible protection stack imbalance /rchk/packages/build/ziRQDfzj/lightgbm/src/lightgbm_R.cpp:102

Function LGBM_DatasetCreateFromFile_R
  [PB] has possible protection stack imbalance /rchk/packages/build/ziRQDfzj/lightgbm/src/lightgbm_R.cpp:70

Function LGBM_DatasetCreateFromMat_R
  [PB] has possible protection stack imbalance /rchk/packages/build/ziRQDfzj/lightgbm/src/lightgbm_R.cpp:126

Function LGBM_DatasetGetFeatureNames_R
  [PB] has possible protection stack imbalance /rchk/packages/build/ziRQDfzj/lightgbm/src/lightgbm_R.cpp:210

Function LGBM_DatasetGetSubset_R
  [PB] has possible protection stack imbalance /rchk/packages/build/ziRQDfzj/lightgbm/src/lightgbm_R.cpp:150
Analyzed 39640 functions, traversed 108278 states.

Rchk version: 3d653b7c8f92dac912532856b55f44d2986c6553
R version: 79889/R Under development (unstable) (2021-01-27 r79889)
LLVM version: 10.0.0

I've also opened #4400 to track the work for possibly adding a CI job.

@jameslamb jameslamb added the bug label Jun 25, 2021
jameslamb added a commit that referenced this issue Jun 27, 2021
…ixes #4390) (#4391)

* [R-package] fix protection stack imbalance and unprotected objects issues

* [R-package] fix minor linting issues

* [ci][R-package] change timeout-minutes in valgrind test

* [R-package] remove extra space

Co-authored-by: James Lamb <jaylamb20@gmail.com>

* [R-package] remove counter for number of protected objects

* Update .github/workflows/r_valgrind.yml

Co-authored-by: James Lamb <jaylamb20@gmail.com>
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants