[R-package] prevent symbol lookup conflicts (fixes #4045) #4155

jameslamb · 2021-04-02T04:10:36Z

This PR fixes a critical issue affecting Windows users of the R package. In #4045, several users reported a problem like "if I load certain R libraries before {lightgbm}, {lightgbm} can cause the R session to crash when constructing a dataset".

After some investigation (detailed in #4045 (comment)), I believe I found the root cause for this. The routine R_registerRoutines() at

LightGBM/R-package/src/lightgbm_R.cpp

Line 724 in d517ba1

R_registerRoutines(dll, NULL, CallEntries, NULL, NULL);

isn't actually being called by R!

The key piece of documentation is at https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Converting-a-package-to-use-registration.

Construct the registration table ... if included in a C++ file the ‘R_init’ function would need to be declared extern "C"):

This PR adds that declaration to lightgbm_R.cpp and adds a unit test to prevent this type of mistake from being re-introduced in the future.

Fixes #4045.
Addresses the Windows half of #4034.

I need to test if this would fix #4007. Will update this description once I try those.

Background

R provides an interface for "native routine registration", a mechanism to tell R exactly which symbols it should expect to find in a shared library (.dll / .so). This makes calls to compiled code with .Call() safer because R looks in a specific place.

If that type of registration isn't done, then it can create the possibility for conflicts with other loaded DLLs. From https://cran.r-project.org/doc/manuals/r-release/R-exts.html#dyn_002eload-and-dyn_002eunload:

If a shared object/DLL is loaded more than once the most recent version is used. More generally, if the same symbol name appears in several shared objects, the most recently loaded occurrence is used. The PACKAGE argument and registration (see the next section) provide good ways to avoid any ambiguity in which occurrence is meant.

This note from https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Registering-native-routines is relevant as well.

In calls to .C, .Call, .Fortran and .External, R must locate the specified native routine by looking in the appropriate shared object/DLL. By default, R uses the operating-system-specific dynamic loader to lookup the symbol in all132 loaded DLLs and the R executable or libraries it is linked to. Alternatively, the author of the DLL can explicitly register routines with R and use a single, platform-independent mechanism for finding the routines in the DLL. One can use this registration mechanism to provide additional information about a routine, including the number and type of the arguments, and also make it available to R programmers under a different name.

How to test this

You can run the following code to check if registration was successful.

library(lightgbm)
dll_info <- getLoadedDLLs()[["lightgbm"]]

# check that dynamic lookup has been disabled
!isTRUE(dll_info[["dynamicLookup"]])

# check that all the expected symbols show up
getDLLRegisteredRoutines(dll_info[["path"]])

                               .Call .Call.numParameters
1                LGBM_GetLastError_R                   3
2       LGBM_DatasetCreateFromFile_R                   5
3        LGBM_DatasetCreateFromCSC_R                  10
4        LGBM_DatasetCreateFromMat_R                   7
5            LGBM_DatasetGetSubset_R                   6
6      LGBM_DatasetSetFeatureNames_R                   3
7      LGBM_DatasetGetFeatureNames_R                   5
8           LGBM_DatasetSaveBinary_R                   3
9                 LGBM_DatasetFree_R                   2
10            LGBM_DatasetSetField_R                   5
11        LGBM_DatasetGetFieldSize_R                   4
12            LGBM_DatasetGetField_R                   4
13 LGBM_DatasetUpdateParamChecking_R                   3
14          LGBM_DatasetGetNumData_R                   3
15       LGBM_DatasetGetNumFeature_R                   3
16              LGBM_BoosterCreate_R                   4
17                LGBM_BoosterFree_R                   2
18 LGBM_BoosterCreateFromModelfile_R                   3
19 LGBM_BoosterLoadModelFromString_R                   3
20               LGBM_BoosterMerge_R                   3
21        LGBM_BoosterAddValidData_R                   3
22   LGBM_BoosterResetTrainingData_R                   3
23      LGBM_BoosterResetParameter_R                   3
24       LGBM_BoosterGetNumClasses_R                   3
25       LGBM_BoosterUpdateOneIter_R                   2
26 LGBM_BoosterUpdateOneIterCustom_R                   5
27     LGBM_BoosterRollbackOneIter_R                   2
28 LGBM_BoosterGetCurrentIteration_R                   3
29  LGBM_BoosterGetUpperBoundValue_R                   3
30  LGBM_BoosterGetLowerBoundValue_R                   3
31        LGBM_BoosterGetEvalNames_R                   5
32             LGBM_BoosterGetEval_R                   4
33       LGBM_BoosterGetNumPredict_R                   4
34          LGBM_BoosterGetPredict_R                   4
35      LGBM_BoosterPredictForFile_R                  11
36      LGBM_BoosterCalcNumPredict_R                   9
37       LGBM_BoosterPredictForCSC_R                  15
38       LGBM_BoosterPredictForMat_R                  12
39           LGBM_BoosterSaveModel_R                   5
40   LGBM_BoosterSaveModelToString_R                   7
41           LGBM_BoosterDumpModel_R

jameslamb · 2021-04-02T04:25:21Z

R-package/tests/testthat/test_registration.R

+# like the one documented in
+# https://github.com/microsoft/LightGBM/issues/4045#issuecomment-812289182
+test_that("lightgbm routine registration worked", {
+    testthat::skip_if_not(ON_WINDOWS)


this skip_if_not() is to work around this error I saw on Linux jobs in CI

* checking tests ... Running ‘testthat.R’ � � ERROR Running the tests in ‘tests/testthat.R’ failed. Last 13 lines of output: ══ Failed tests ════════════════════════════════════════════════════════════════ ── Failure (test_registration.R:8:5): lightgbm routine registration worked ───── dll_info[["dynamicLookup"]] is not FALSE `actual` is NULL `expected` is a logical vector (FALSE) ── Error (test_registration.R:11:5): lightgbm routine registration worked ────── Error: no applicable method for 'getDLLRegisteredRoutines' applied to an object of class "NULL" Backtrace: █ 1. └─base::getDLLRegisteredRoutines(dll_info[["path"]]) test_registration.R:11:4 [ FAIL 2 | WARN 0 | SKIP 3 | PASS 634 ] Error: Test failures Execution halted

ah! The problem with this test isn't actually about operating system. This test will fail with an error like the one above on LightGBM's CMake-based builds and pass on CRAN builds. That's because, by using install.libs.R to build lib_lightgbm instead of R's built-in machinery, the routine registration stuff doesn't work.

Given that as of this PR the package will now use literal R symbols (e.g. .Call(LGBM_BoosterGetCurrentIteration_R,...), I don't think this test is necessary. Any failures in the registration will result in "object not found errors".

I'm going to remove this test, and will provide more explanation in a separate comment for how to make this work with CMake-based builds.

jameslamb · 2021-04-02T04:49:37Z

hmmm, seeing a few things in CI that I didn't see locally.

* checking foreign function calls ... NOTE

Registration problems:

  symbol 'fun_name' not in namespace:

   .Call(fun_name, ..., ret, call_state, PACKAGE = "lightgbm")

  symbol 'fun_name' not in namespace:

   .Call(fun_name, ..., call_state, PACKAGE = "lightgbm")

See chapter 'System and foreign language interfaces' in the 'Writing R

Extensions' manual.

I'm going to remove reviewers and move this back to draft for now

StrikerRUS · 2021-04-03T11:53:55Z

Can resolving #3016 help with this issue as well?

jameslamb · 2021-04-03T17:19:15Z

Can resolving #3016 help with this issue as well?

Right now, I don't think so.

I think the problem is that {lightgbm} wraps .Call() calls in a function to handle errors called lgb.call() (

LightGBM/R-package/R/utils.R

Line 62 in d517ba1

lgb.call <- function(fun_name, ret, ...) {

), but if you are using routine registration then R CMD check --as-cran expects that the first argument to .Call() is one of the symbols you've registered. Like this: https://github.com/Rdatatable/data.table/blob/ec1259af1bf13fc0c96a1d3f9e84d55d8106a9a4/R/bmerge.R#L181.

I'm experimenting with this today. I think error handling can be moved into the C++ side and then lgb.call() can be factored out. It's an internal function so there wouldn't be any user-facing impact.

jameslamb · 2021-04-26T01:12:15Z

/gha run r-valgrind

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

Status: success ✔️.

jameslamb · 2021-04-26T03:44:54Z

Ok, I think that this PR is ready for review! I know the description has a lot of information and this requires some deep knowledge of how R's build system works, so I'll summarize here:

Today, {lightgbm} code that calls C++ functions with .Call() is using an unsafe strategy to search for symbols, and that can lead to serious conflicts with other loaded packages (see [R-package] FATAL Error when run train() #4045)
The preferred, and safer, way to do this is to use "routine registration" (documented in the PR description)
When you use registration, R will create R objects in the package's namespace and expect you to use them. So, for example, for the C++ function LGBM_DatasetCreateFromCSC_R, you have to use .Call(LGBM_DatasetCreateFromCSC_R, ...), not .Call("LGBM_DatasetCreateFromCSC_R", ..., PACKAGE = "lightgbm").
When you use install.libs.R to build a custom .so/.dll (as {lightgbm} does for CMake-based builds), this automatic creation of R objects will not work. To force that to be done, you can use code like useDynLib(lib_lightgbm, LGBM_DatasetCreateFromCSC_R, ...)

As a result of the changes in this PR:

{lightgbm} should be safer to use with other packages, and the critical issue [R-package] FATAL Error when run train() #4045 should be fixed
{lightgbm}'s CRAN package will be structured in a way that is closer to the recommended practices documented in https://cran.r-project.org/doc/manuals/r-release/R-exts.html

jameslamb · 2021-04-26T03:48:28Z

R-package/R/utils.R

@@ -59,53 +57,6 @@ lgb.last_error <- function() {

 }

-lgb.call <- function(fun_name, ret, ...) {


These lgb.call() functions can be removed because once routine registration is used, referencing routines using strings does not work: #4155 (comment).

jameslamb · 2021-04-26T03:53:18Z

R-package/R/lgb.Dataset.R

@@ -13,7 +13,12 @@ Dataset <- R6::R6Class(
      if (!lgb.is.null.handle(x = private$handle)) {

        # Freeing up handle
-        lgb.call(fun_name = "LGBM_DatasetFree_R", ret = NULL, private$handle)
+        call_state <- 0L


Even though error-handling has now been moved into the C++ side (#4163), as of this PR passing in call_state is still needed. The functions in https://github.com/microsoft/LightGBM/blob/b6c71e5e941b885b1a5169d460c430df9d392fa9/R-package/src/lightgbm_R.h has a return type LGBM_SE, which is the LightGBM equivalent of R's built-in SEXP type.

If you change those routines to all be void (since they modify their inputs by reference and don't technically have to refer to anything), errors are thrown about not being able to convert a null to SEXP.

So the call_state thing is a simple way to allocate an LGBM_SE (an R integer, in this case) that can then be returned.

As part of #3016 we'll be able to remove these unnecessary call_state things, but I believe they're harmless for now.

StrikerRUS

Excellent improvement for the package! I'm approving based on the detailed PR description, linked paragraph of article in comments and green CI checks.
Just two minor comments below.

StrikerRUS · 2021-04-30T12:10:16Z

build_r.R

+# CMake-based builds can't currently use R's builtin routine registration,
+# so have to update NAMESPACE manually, with a statement like this:
+#
+# useDynLib(lib_lightgbm, LGBM_GetLastError_R, LGBM_DatasetCreateFromFile_R, ...)


Maybe it will be easier to have this content by default in the file, and remove it for CRAN build?

hmmm I'm not sure. This looks like more code than having this in the file by default and removing it for CRAN builds, but I thought this R code would be easier to write than the equivalent code with sed in build-cran-package.sh. Just because it involves replacing / writing literal . and literal ) / ( and I was intimidated by figuring out how to do the escaping correctly in sed haha.

Do you have a strong preference for this change? Otherwise, I'd prefer to leave this as-is for now and maybe make that change as some maintenance work in the future. Just because I'd like to keep pushing forward on #3016 and it doesn't seem like there is anything wrong about the current approach.

Not really, this was just a proposal to simplify the code. I'm totally fine with the working current approach.

ok great, thank you very much!

StrikerRUS · 2021-04-30T12:12:40Z

R-package/R/lgb.Booster.R

-      return(
-        lgb.call.return.str(
-          fun_name = "LGBM_BoosterSaveModelToString_R"
+      # Create buffer


Maybe write a new helper function for calls with buffer allocation? I believe it will improve the code readability.

That was exactly what lgb.call.return.str() was. But since you cannot pass a string with a function name to .Call() once registration is used (#4155 (comment)), I couldn't see a good way to keep a shared helper function.

I also think we'll be able to factor this code away completely soon, as part of #3016, by just creating the R character vector on the C++ side and returning it. See #4242 for a first example of that.

Ah, OK, I see!

github-actions · 2023-08-23T22:39:36Z

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.

jameslamb added 2 commits April 2, 2021 04:41

[R-package] prevent symbol lookup conflicts

7cb47d0

add unit tests

791cf6c

jameslamb added the fix label Apr 2, 2021

jameslamb requested review from guolinke and StrikerRUS April 2, 2021 04:10

jameslamb requested a review from Laurae2 as a code owner April 2, 2021 04:10

This was referenced Apr 2, 2021

[R-package] FATAL Error when run train() #4045

Closed

[R-package] predict() breaks when using a Dataset stored in a file #4034

Closed

only run test on Windows

f2a584e

jameslamb commented Apr 2, 2021

View reviewed changes

jameslamb marked this pull request as draft April 2, 2021 04:49

jameslamb removed request for Laurae2, guolinke and StrikerRUS April 2, 2021 04:49

This comment has been minimized.

Sign in to view

jameslamb changed the title ~~[R-package] prevent symbol lookup conflicts (fixes #4045)~~ WIP: [R-package] prevent symbol lookup conflicts (fixes #4045) Apr 2, 2021

This comment has been minimized.

Sign in to view

jameslamb mentioned this pull request Apr 4, 2021

[R-package] Move error handling into C++ side #4163

Merged

jameslamb added the in progress label Apr 6, 2021

nhejazi mentioned this pull request Apr 21, 2021

Lrnr_lightgbm broken on Windows tlverse/sl3#344

Closed

jameslamb mentioned this pull request Apr 22, 2021

[R-package] Factor out custom R interface to lib_lightgbm #3016

Closed

jameslamb added 5 commits April 21, 2021 23:13

Merge branch 'fix/r-registration' of github.com:microsoft/LightGBM

cc3e6fc

Merge branch 'master' of github.com:microsoft/LightGBM

8ed693a

Merge branch 'master' of github.com:microsoft/LightGBM

e7b254d

move to .Call() calls

6668390

fix references

a2ccac5

jameslamb added 3 commits April 25, 2021 11:57

testing registration for CMake builds

f89aa2c

revert NAMESPACE changes

2d8de84

revert testing changes

42d01c8

jameslamb marked this pull request as ready for review April 26, 2021 03:44

jameslamb requested a review from StrikerRUS as a code owner April 26, 2021 03:44

jameslamb changed the title ~~WIP: [R-package] prevent symbol lookup conflicts (fixes #4045)~~ [R-package] prevent symbol lookup conflicts (fixes #4045) Apr 26, 2021

jameslamb requested review from Laurae2 and shiyu1994 April 26, 2021 03:45

jameslamb commented Apr 26, 2021

View reviewed changes

jameslamb added awaiting review and removed in progress labels Apr 26, 2021

jameslamb added 2 commits April 28, 2021 22:46

Merge branch 'master' into fix/r-registration

d70071c

Merge branch 'master' into fix/r-registration

ad229c7

jameslamb mentioned this pull request Apr 30, 2021

[R-package] Convert LGBM_GetLastError_R to use R built-in types #4242

Merged

StrikerRUS approved these changes Apr 30, 2021

View reviewed changes

jameslamb removed the awaiting review label Apr 30, 2021

jameslamb merged commit b27dcfa into master Apr 30, 2021

jameslamb deleted the fix/r-registration branch April 30, 2021 21:29

This was referenced May 1, 2021

[R-package] remove pre-allocated call_state in C++ calls #4244

Merged

[R-package] move creation of character vectors in some methods to C++ side #4256

Merged

jameslamb mentioned this pull request May 10, 2021

[R-package] suppress Wcast-function-type warning in CMake-based gcc and MinGW builds (fixes #4273) #4274

Merged

This was referenced May 20, 2021

[R-package] Very large l2 when training model #4305

Closed

release 3.3.0 #4310

Closed

jameslamb mentioned this pull request Jun 23, 2021

[R-package] [ci] Add a CI job testing the R package with rchk #4400

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R-package] prevent symbol lookup conflicts (fixes #4045) #4155

[R-package] prevent symbol lookup conflicts (fixes #4045) #4155

jameslamb commented Apr 2, 2021 •

edited

Loading

jameslamb Apr 2, 2021

jameslamb Apr 25, 2021

jameslamb commented Apr 2, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

StrikerRUS commented Apr 3, 2021

jameslamb commented Apr 3, 2021

jameslamb commented Apr 26, 2021 •

edited by guolinke

Loading

jameslamb commented Apr 26, 2021

jameslamb Apr 26, 2021

jameslamb Apr 26, 2021

StrikerRUS left a comment

StrikerRUS Apr 30, 2021

jameslamb Apr 30, 2021

StrikerRUS Apr 30, 2021

jameslamb Apr 30, 2021

StrikerRUS Apr 30, 2021

jameslamb Apr 30, 2021

StrikerRUS Apr 30, 2021

github-actions bot commented Aug 23, 2023

		@@ -59,53 +57,6 @@ lgb.last_error <- function() {

		}

		lgb.call <- function(fun_name, ret, ...) {

[R-package] prevent symbol lookup conflicts (fixes #4045) #4155

[R-package] prevent symbol lookup conflicts (fixes #4045) #4155

Conversation

jameslamb commented Apr 2, 2021 • edited Loading

Background

How to test this

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb commented Apr 2, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

StrikerRUS commented Apr 3, 2021

jameslamb commented Apr 3, 2021

jameslamb commented Apr 26, 2021 • edited by guolinke Loading

jameslamb commented Apr 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StrikerRUS left a 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

github-actions bot commented Aug 23, 2023

jameslamb commented Apr 2, 2021 •

edited

Loading

jameslamb commented Apr 26, 2021 •

edited by guolinke

Loading