Dynamic Library Loading Support by mseth10 · Pull Request #15489 · apache/mxnet

mseth10 · 2019-07-08T23:30:39Z

Description

This PR is part of a larger effort of adding new functionality to MXNet that was compiled separately from MXNet. See here and here for more info.

This PR is the first step that enables libraries to be dynamically loaded into MXNet at runtime .

Features:

Dynamic library loading functions for both Linux and Windows to load libraries and access its functions
MXLoadLib that loads and initializes a library
A new Python API: mx.library.load that calls the new C-API
Enhancements to both the Makefile and CMake flows to link with the dynamic loader library (dl)

This PR includes the following:

An example, showing how to write a sample library in C++ (mylib.cc) with its own Makefile, a unittest to verify correctness outside of MXNet (libtest.cc), and a test script showing how to load the library in MXNet (test.py).
A unit test that loads a precompiled library into MXNet and initializes it. It runs as part of the CI for both Linux and Windows.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Feature1, tests, (and when applicable, API doc)
Feature2, tests, (and when applicable, API doc)

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

karan6181 · 2019-07-15T21:30:12Z

@mxnet-label-bot add [backend, C API, pr-work-in-progress]

leleamol · 2019-08-02T00:05:25Z

src/common/library.cc

+    return nullptr;
+  }
+#else
+  handle = dlopen(path, RTLD_LAZY);


Where are we calling the 'dlclose()'?

How are we checking that we are not calling "dlopen" multiple times for the same library? If we are allowing calling multiple dlopen for the same file, it should match with the same number of dlclose.

Thanks @leleamol, per our offline discussion we will take the following actions:

Create a map to track file handles and paths to the libraries when opened

We'll only allow dlopen to be called once, but if the library is "reloaded" by the user, we'll skip dlopen and go through the same "library registration process" again.

Add code to call dlclose for each library opened in the Engine descructors for each of the engine types in:
https://github.com/apache/incubator-mxnet/blob/eab6da6208fdc7b68437dadc6d7d58ee11c2f528/src/engine/naive_engine.cc#L70
https://github.com/apache/incubator-mxnet/blob/3a4698058840ff79e58709b6135c8fb757a99606/src/engine/threaded_engine.h#L327

leleamol · 2019-08-02T00:20:42Z

src/common/library.cc

+    LocalFree(err_msg);
+  }
+#else
+  *func = dlsym(handle, name);


The dlsym can return NULL and it may not necessarily indicate error condition. The recommended error handling is mentioned in this document.
https://linux.die.net/man/3/dlsym

Thanks @leleamol, per this: https://stackoverflow.com/a/53590728 I think we can just take the shortcut and assume dlsym returning NULL means the symbol is not found. You have to try pretty hard to get gcc to put a function at the global 0 address.

Ok.. Sounds good.

samskalicky · 2019-08-03T17:42:23Z

@leleamol Can you please review again to see if the implementation of the dlclose matches what you were thinking?

@anirudh2290 Thanks for the help with the engine destructor info, can you take a look at the changes to the engines:
https://github.com/apache/incubator-mxnet/blob/134cc37d6b306514fde1a405d3b87cc946a56bd1/src/engine/naive_engine.cc#L88-L91
https://github.com/apache/incubator-mxnet/blob/134cc37d6b306514fde1a405d3b87cc946a56bd1/src/engine/threaded_engine.h#L335-L338

leleamol

Looks good to me.

leleamol · 2019-08-05T17:50:03Z

src/common/library.cc

+    LocalFree(err_msg);
+  }
+#else
+  *func = dlsym(handle, name);


Ok.. Sounds good.

rondogency

LGTM

marcoabreu · 2019-08-05T19:31:20Z

tests/python/unittest/test_library_loading.py

+    elif (os.name=='nt'):
+        lib = 'mylib.dll'
+
+    fname = mx.test_utils.download('https://mxnet-demo-models.s3.amazonaws.com/lib_binary/'+lib)


@mseth10

Unfortunately, this test introduces a security vulnerability. Please replace it as soon as possible as this poses a security threat to our system as well as our users since this allows arbitrary code execution. I'd appreciate it if you would provide a replacement within the next 24h. #15755 is the corresponding revert PR.

I suggested before to build this stub during the build process instead of loading from S3. See previous comment above: https://github.com/apache/incubator-mxnet/pull/15489/files#r308932209

samskalicky · 2019-08-05T20:47:09Z

Thanks @marcoabreu for the catch. As we discussed offline, the problem is not with the feature in this PR but with how test_library_loading is pulling from the S3 bucket in the CI.

We'll work to build the library as part of the build so that it can be used internally in testing the CI without security risks. We'll move the test pulling from the S3 bucket to the examples directory so that it doesnt run in the CI.

We may need some help @marcoabreu to get this working in the CI, since we'll need to build this library and then find the path where it lives during the test so we can pass the path to the load API.

In the meantime, we'll make these changes and then resubmit this PR.

marcoabreu · 2019-08-05T21:04:29Z

We'll move the test pulling from the S3 bucket to the examples directory so that it doesnt run in the CI.

Could you elaborate?

samskalicky · 2019-08-05T21:20:30Z

@marcoabreu we added an example directory at: example/lib_api. Currently the test.py includes a test that uses the locally built library (built using the included Makefile). But this only provides an example for how to build a library.

We'll create another example python script that pulls from the S3 bucket and loads that one. But it will just be an example, not a unittest that will run in the CI.

Do you have other ideas/suggestions on how we can improve testing or provide more examples?

marcoabreu · 2019-08-05T22:54:54Z

No pulling of any prebuilt binaries due to the same reasons, please. I understand you trying to make it more convenient, but I think that we're stepping over a line in that case. Give instructions how to compile the example and the user should be fine.

Just to confirm: we will have a unit test running in ci at the end of the day, correct?

samskalicky · 2019-08-05T23:10:40Z

@marcoabreu I think we're in agreement here. We will be distributing some pre-built libraries in an S3 bucket as examples. We will provide documented examples for how to pull and load these binaries. But these will not be tests that will be automatically run. They will be examples.

Yes, we will have the unit test running in the CI that uses a library that is built in the CI.

wkcn · 2019-08-07T12:25:00Z

I have a suggestion. We can check the sha1_hash code of pre-built binary files in the S3 bucket before executing them.

marcoabreu · 2019-08-07T12:30:42Z

While that eliviates the problem from a ci perspective (if the bucket would be owned by the CI owners), it's still a red flag for our users.

I'm afraid that companies prefer to stay away from the execution of an arbitrary native library that is downloaded from the internet - no matter whether it is backed by a hash or not. It's inevitable in some cases, but we want to reduce the possiblity of compliance scanners flagging us and thus making the usage of the MXNet more complicated than it has to be.

I think the only acceptable way is if the library is getting produced as part of the build process that is entirely retrieved from source. These are also the tenants Apache employs - Apache project must not make any binary releases in any form. Thus let's best stick to that as well :)

mseth10 requested review from anirudh2290 and eric-haibin-lin as code owners July 8, 2019 23:30

mseth10 force-pushed the acc_api branch 8 times, most recently from cc55e0d to 94fda6e Compare July 10, 2019 00:50

mseth10 requested a review from szha as a code owner July 11, 2019 01:02

mseth10 force-pushed the acc_api branch 14 times, most recently from 671df6f to ddd0e59 Compare July 12, 2019 21:59

mseth10 requested a review from rahul003 as a code owner July 12, 2019 21:59

mseth10 force-pushed the acc_api branch 3 times, most recently from 448e950 to c490c23 Compare July 15, 2019 09:18

mseth10 added 8 commits August 1, 2019 18:39

adding unit test to gpu suite

7551313

correcting windows pointer

68a4e6c

requiring absolute path to library

e8c8d2f

changing file description

71ac937

addressing review comments - adding more docs, windows error msg

4cb4caf

addressing PR comments

4e2dba8

checking machine type for unit test

7f8a9fb

“re-trigger”

672b2b9

zheng-da approved these changes Aug 1, 2019

View reviewed changes

leleamol suggested changes Aug 2, 2019

View reviewed changes

Ubuntu and others added 7 commits August 2, 2019 06:26

added map to store loaded libraries

2257ff3

added dlclose calls in naive & threaded engines

0d05d1c

removed library map declaration in cc file

3879f61

added windows free

289c3a5

fixed formatting

a3b545d

added cast to HMODULE for void* for windows

e512d64

retrigger CI for flaky unix_cpu

134cc37

leleamol approved these changes Aug 5, 2019

View reviewed changes

src/common/library.cc

LocalFree(err_msg);

}

#else

*func = dlsym(handle, name);

Copy link

Contributor

leleamol Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.. Sounds good.

rondogency approved these changes Aug 5, 2019

View reviewed changes

marcoabreu mentioned this pull request Aug 5, 2019

Revert "Dynamic Library Loading Support" #15755

Merged

marcoabreu reviewed Aug 5, 2019

View reviewed changes

mseth10 mentioned this pull request Aug 6, 2019

Fix PR #15489 (Dynamic Library Loading Support) #15760

Merged

7 tasks

Conversation

mseth10 commented Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Changes

Comments

Uh oh!

karan6181 commented Jul 15, 2019

Uh oh!

leleamol Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

samskalicky Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leleamol Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

samskalicky Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leleamol Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

samskalicky commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leleamol left a comment

Choose a reason for hiding this comment

Uh oh!

leleamol Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

rondogency left a comment

Choose a reason for hiding this comment

Uh oh!

marcoabreu Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larroy Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoabreu Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

samskalicky commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcoabreu commented Aug 5, 2019

Uh oh!

samskalicky commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcoabreu commented Aug 5, 2019

Uh oh!

samskalicky commented Aug 5, 2019

Uh oh!

wkcn commented Aug 7, 2019

Uh oh!

marcoabreu commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

mseth10 commented Jul 8, 2019 •

edited

Loading

samskalicky Aug 2, 2019 •

edited

Loading

samskalicky Aug 2, 2019 •

edited

Loading

samskalicky commented Aug 3, 2019 •

edited

Loading

marcoabreu Aug 5, 2019 •

edited

Loading

larroy Aug 5, 2019 •

edited

Loading

samskalicky commented Aug 5, 2019 •

edited

Loading

samskalicky commented Aug 5, 2019 •

edited

Loading

marcoabreu commented Aug 7, 2019 •

edited

Loading