[R] Fix memory safety issues #9823

david-cortes · 2023-11-29T18:56:39Z

This PR fixes some remaining memory safety issues:

Allocations from R interleaved with allocations from C++ that could leave leaked objects in case of R failures.
Lengths using a type of insufficient width that might overflow.

david-cortes · 2023-11-29T20:50:35Z

Added them under the namespace that was introduced in #9816 to make a difference w.r.t. the exported functions. Not sure why they would need to have [[nodiscard]] though.

trivialfis

Thank you for the work on fixing the memory safety issue and demonstrating how to properly work with R in C. Left some comments in the hope that we can make the code easier to understand for new comers.

trivialfis · 2023-11-30T09:07:52Z

R-package/src/xgboost_R.cc

+    } else {
+      for (size_t i = 0; i < olen; ++i) {
+        std::stringstream stream;
+        stream <<  "booster[" << i <<"]\n" << res[i];


Unrelated to this PR. We will probably need to convert the indexing for tree dump into 1-based in the future.

trivialfis · 2023-11-30T09:13:01Z

R-package/src/xgboost_R.cc

+
+struct ErrorWithUnwind : public std::exception {};
+
+void throw_exception_from_R_error(void *unused, Rboolean jump) {


Really appreciate it if you could help add some references and documents for R unwinding with C++. I found your comments in the LGBM issue, which is helpful for providing context. I have some familiarity with continuation-passing, but still trying to wrap my head around how this works.

https://cran.r-project.org/doc/manuals/r-patched/R-exts.html#Condition-handling-and-cleanup-code-1

That's the only documentation there is.

In case it helps: when a failure occurs, the long jump is triggered from the line that calls R_ContinueUnwind. By that point, all the C++ objects will have been destructed, since that line passes through the 'try' block. Upon throwing an R error, all the R allocations that happened during the .Call part are freed.

Simpler example of this usage can be found here: https://github.com/david-cortes/unwindprotect

Thank you for the explanation and the example usage. This has been most helpful.

R-package/src/xgboost_R.cc

trivialfis · 2023-11-30T09:27:37Z

R-package/src/xgboost_R.cc

  auto ctx = DMatrixCtx(R_ExternalPtrAddr(handle));
  if (!strcmp("group", name)) {
    std::vector<unsigned> vec(len);
+    const int *array_ = INTEGER(array);


seems unrelated once #9824 is merged. I will skip this part of review.

Forgot to mention: one is not supposed to call R C functions in parallel from different threads. It might not be a problem with regular objects, but for altrep'd classes a call to INTEGER could potentially trigger an R allocation.

trivialfis · 2023-11-30T09:31:57Z

R-package/src/xgboost_R.cc

+  SEXP name_ = PROTECT(Rf_asChar(name));
+  SEXP val_ = PROTECT(Rf_asChar(val));


would be great if there's a utility function/class that does this. But the longjmp and macros complicate things and I'm not sure if the following code is actually safe.

class SafeRasChar { std::int32_t cnt_{0} public: SEXP AsChar(SEXP str) { auto exp = PROTECT(Rf_asChar(str)); cnt_ ++; } ~SafeRasChar () { UNPROTECT(cnt_); } };

Yes, that's also an option, but be aware that it doesn't play along with rchk: kalibera/rchk#27

Since it seems rchk is not used in the CI jobs, maybe that's a good idea. I know DuckDB for example uses this type of pattern in their R interface.

but be aware that it doesn't play along with rchk

Thanks for the reference. Let's not complicate the cran submit process. ;-)

trivialfis · 2023-11-30T09:35:37Z

R-package/src/xgboost_R.cc

@@ -655,9 +712,19 @@ XGB_DLL SEXP XGBoosterGetAttr_R(SEXP handle, SEXP name) {

 XGB_DLL SEXP XGBoosterSetAttr_R(SEXP handle, SEXP name, SEXP val) {
  R_API_BEGIN();
-  const char *v = isNull(val) ? nullptr : CHAR(asChar(val));
+  const char *v = nullptr;
+  SEXP name_ = PROTECT(Rf_asChar(name));


See the previous comment for a utility class.

david-cortes · 2023-11-30T18:21:39Z

Regarding your other comment:

Is it possible to return errors in a more linear fashion?

It would require modifying the macros R_API_BEGIN, R_API_END, CHECK_CALL; and then modifying functions to be mindful of whether they put objects before/after those. I guess that's more work than the current code here, but an example of how it could work otherwise can be found in lightgbm: https://github.com/microsoft/LightGBM/blob/master/R-package/src/lightgbm_R.cpp

If you want an easier and safer way, there's also Rcpp::exception, Rcpp::unwindProtect, etc. but that would involve having Rcpp as a dependency.

david-cortes · 2023-11-30T18:26:45Z

Then, there's also the possibility of passing R allocator functions to C++ functions, but this is perhaps a lot more involved and harder to remember:
https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Memory-allocation

david-cortes · 2023-12-01T17:08:30Z

@trivialfis It looks like the CUDA job uses R 3.3, which is very old so doesn't have the unwind protection that was introduced in R 3.5, for example. Could you update those to R >= 4.3 for this and the following PRs?

trivialfis · 2023-12-02T05:43:36Z

Could you update those to R >= 4.3 for this and the following PRs?

I have merged the update into this branch and the test build has passed.

fix potential memory issues

fcc8c05

david-cortes mentioned this pull request Nov 29, 2023

[R] Fix potential memory leaks when creating externalptr #9817

Merged

solve merge conflicts, follow function naming casing

37bb055

trivialfis reviewed Nov 30, 2023

View reviewed changes

david-cortes added 3 commits November 30, 2023 19:02

more safety around CHECK_CALL

ce19814

solve merge conflicts

c9f45c6

linter

c68dc86

trivialfis approved these changes Dec 1, 2023

View reviewed changes

Merge branch 'master' into memory_safety

0615edd

trivialfis merged commit 7196c9d into dmlc:master Dec 2, 2023
26 checks passed

trivialfis mentioned this pull request Dec 6, 2023

[R] Bump requirement to 4.3.0. #9847

Merged

trivialfis mentioned this pull request Dec 23, 2023

[R] Destruct dmlc::Error objects before long jumps #9916

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R] Fix memory safety issues #9823

[R] Fix memory safety issues #9823

david-cortes commented Nov 29, 2023

david-cortes commented Nov 29, 2023

trivialfis left a comment

trivialfis Nov 30, 2023 •

edited

Loading

trivialfis Nov 30, 2023

david-cortes Nov 30, 2023

trivialfis Dec 1, 2023

trivialfis Nov 30, 2023

david-cortes Nov 30, 2023

trivialfis Nov 30, 2023

david-cortes Nov 30, 2023

trivialfis Dec 1, 2023

trivialfis Nov 30, 2023

david-cortes commented Nov 30, 2023 •

edited

Loading

david-cortes commented Nov 30, 2023

david-cortes commented Dec 1, 2023

trivialfis commented Dec 2, 2023


		struct ErrorWithUnwind : public std::exception {};

		void throw_exception_from_R_error(void *unused, Rboolean jump) {

		SEXP name_ = PROTECT(Rf_asChar(name));
		SEXP val_ = PROTECT(Rf_asChar(val));

[R] Fix memory safety issues #9823

[R] Fix memory safety issues #9823

Conversation

david-cortes commented Nov 29, 2023

david-cortes commented Nov 29, 2023

trivialfis left a comment

Choose a reason for hiding this comment

trivialfis Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-cortes commented Nov 30, 2023 • edited Loading

david-cortes commented Nov 30, 2023

david-cortes commented Dec 1, 2023

trivialfis commented Dec 2, 2023

trivialfis Nov 30, 2023 •

edited

Loading

david-cortes commented Nov 30, 2023 •

edited

Loading