[R-package] fix protection stack imbalance and unprotected objects (fixes #4390)

[fabsig]

Fixes #4390.

[jameslamb]

Thank you very much for your efforts and clear description of the problem in #4390!

I'll be able to give this a more thorough review later today or maybe tomorrow. For now, could you please address the minor linting issues described in https://github.com/microsoft/LightGBM/pull/4391/checks?check_run_id=2858695056?

You can see the following to understand how to replicate those locally:

LightGBM/.ci/test.sh

Line 66 in 42ddff1

pip install --user cpplint isort mypy

LightGBM/.ci/test.sh

Line 75 in 42ddff1

cpplint --filter=-build/c++11,-build/include_subdir,-build/header_guard,-whitespace/line_length --recursive ./src ./include ./R-package ./swig ./tests || exit -1

[jameslamb]

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/950064363

Status: failure ❌.

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/950064363

Status: failure ❌.

[jameslamb]

/gha run r-solaris

Workflow Solaris CRAN check has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/950065364

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-f462ced23b0e4dec8d603c104ce95c5f
solaris-x86-patched-ods: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-da0ec8c6c2804256bb1f04c0d8105bd9
Reports also have been sent to LightGBM public e-mail: http://www.yopmail.com/lightgbm_rhub_checks
Status: success ✔️.

[fabsig]

Linting issues should be fixed.

[jameslamb]

Thanks! Sorry for the delays in CI runs...GitHub has a new mechanism (introduced to prevent abuse) which requires maintainers to manually approve CI runs for new contributors.

[fabsig]

No hurry. A few comments on my changes that might be helpful for easier understanding:

• I dropped all return statements in the catch statements in the R_API_END and CHECK_CALL macros to prevent potential protection stack imbalances. I don't see any use of these return statements.
• I wrapped the R_API_BEGIN and R_API_END only around the calls to the C functions. It seems to me that the other operations don't need any error handling. In this way, the exception handling is encapsulated away from the protection
• Added protection where needed

[jameslamb]

@fabsig we use a continuous integration job triggered by pull request comments which replicates CRAN's use of valgrind to check incoming packages.

I've tried to run it twice on this PR but the job is timing out.

https://github.com/microsoft/LightGBM/actions/runs/950064363

The job running on runner GitHub Actions 23 has exceeded the maximum execution time of 120 minutes.

It's possible that we were already very close to the 120-minute limit and that the changes here added enough time to hit that limit. Could you please update

LightGBM/.github/workflows/r_valgrind.yml

Line 10 in c7134fa

timeout-minutes: 120

to 180 in this PR? Just so we can see how long it takes.

Really sorry for the inconvenience.

[fabsig]

Could you please update

LightGBM/.github/workflows/r_valgrind.yml

Line 10 in c7134fa

timeout-minutes: 120

Of course. Let me know if further changes are needed.

[jameslamb]

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/959047226

Status: failure ❌.

[jameslamb]

Thanks very much for this!

I ran the following to run rchk on this branch, and can confirm these changes eliminate the stack imbalance and protection errors found on master (#4390 (comment)) 🎉

git clone --recursive git@github.com:fabsig/LightGBM.git fabsig-LightGBM
cd fabsig-LightGBM
git fetch origin R_package_protection
git checkout R_package_protection

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

end of rchk logs on this branch

Installed libraries of package  lightgbm 
[1] "/rchk/packages/libsonly/lightgbm/libs/lightgbm.so"

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
Analyzed 39640 functions, traversed 107321 states.

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

So overall, this looks great! Please just see a few minor comments I left about formatting.

---

I think that our friends over at {xgboost} (@trivialfis @hcho3) would also really appreciate a similar contribution, if you have time. That project has similar uses of inline CHAR(asChar()) without protection. For example:

https://github.com/dmlc/xgboost/blob/da1ad798cabb6868be368bd262814cebcbf3d094/R-package/src/xgboost_R.cc#L416-L421

Would you consider opening an issue similar to #4390 in https://github.com/dmlc/xgboost/issues? Not strictly required here, but we have borrowed a lot from that project and try to give back to it when possible.

[fabsig]

I think that our friends over at {xgboost} (@trivialfis @hcho3) would also really appreciate a similar contribution, if you have time. That project has similar uses of inline CHAR(asChar()) without protection. For example:

https://github.com/dmlc/xgboost/blob/da1ad798cabb6868be368bd262814cebcbf3d094/R-package/src/xgboost_R.cc#L416-L421

Would you consider opening an issue similar to #4390 in https://github.com/dmlc/xgboost/issues? Not strictly required here, but we have borrowed a lot from that project and try to give back to it when possible.

Thanks for the suggestion. I will do that.

[trivialfis]

Thanks for letting us know!

[jameslamb]

These changes look great! Thanks for a narrowly-scoped and very clearly-explained pull request that was easy to review.

I am leaving a "request changes" review only to block merging for now. I want to wait to merge this until we can run valgrind checks on this branch, which won't be possible until #4404 is merged.

[StrikerRUS]

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/974132065

Status: success ✔️.

[jameslamb]

valgrind seems happy with these changes (#4391 (comment)).

I also re-ran rchk on this branch and it did not detect any remaining issues.

Installed libraries of package  lightgbm
[1] "/rchk/packages/libsonly/lightgbm/libs/lightgbm.so"
/rchk/scripts/check_package.sh: line 100:   561 Killed                  $RCHK/src/$T $RBC $F > $FOUT 2>&1

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

Thanks very much for this contribution 🙌

[github-actions]

This pull request 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.