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

find_library: Prefer currently loaded libraries over searching LD_LIBRARY_PATH #143

Closed
EricCousineau-TRI opened this issue Mar 25, 2019 · 22 comments
Assignees

Comments

@EricCousineau-TRI
Copy link

@EricCousineau-TRI EricCousineau-TRI commented Mar 25, 2019

At present, find_library drops straight into searching libraries along LD_LIBRARY_PATH, rather than first querying loaded libraries. One of the 3 current implementations:
https://github.com/ros2/rmw_implementation/blob/32c3de1/rmw_implementation/src/functions.cpp#L77

It'd be nice if this code instead preferred currently loaded libraries, to support application code that don't want the ability to switch DDS implementations (e.g. simplifying repro cases, etc.) at runtime, and instead RPATH link the appropriate libraries.

@EricCousineau-TRI
Copy link
Author

@EricCousineau-TRI EricCousineau-TRI commented Mar 25, 2019

At this commit for this Bazel repro project, I can now either specify the RMW implementation at linking time, or defer to environment variables:
EricCousineau-TRI/repro@cea1102

@dirk-thomas I would find this extremely useful. Can I ask what (if any) evidence might sway you to consider this? :D

@dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Mar 25, 2019

Can I ask what (if any) evidence might sway you to consider this?

Since the goal of having a distro is to ensure stability backports are for bug fixes only. Enhancements should target the next distro.

The first test packages of Dashing are targeted to be available in the first week of April.

@EricCousineau-TRI
Copy link
Author

@EricCousineau-TRI EricCousineau-TRI commented Mar 26, 2019

Aye, sounds good. Will await the merge of ros2/rcpputils#3, and then file this against the dev branch. Thanks!

@wieset
Copy link

@wieset wieset commented Jan 24, 2020

We have a ros2 node that needs capabilities set via setcap which leads to LD_LIBRARY_PATH being omitted during execution. So we are setting RPATH in the binary to find shared libraries. However, as find_library_path relies on LD_LIBRARY_PATH the node fails with

terminate called after throwing an instance of 'rclcpp::exceptions::RCLError'
  what():  failed to initialized rcl init options: failed to find shared library of rmw implementation. Searched rmw_fastrtps_cpp, at /tmp/binarydeb/ros-eloquent-rmw-implementation-0.8.2/src/functions.cpp:130, at /tmp/binarydeb/ros-eloquent-rcl-0.8.3/src/rcl/init_options.c:55

If I understand correctly, the proposal by @EricCousineau-TRI would solve this problem. Any chance to get this functionality in the next release? Or any suggestions for a workaround?

@ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Feb 6, 2020

@EricCousineau-TRI are you planning to iterate on this?

@dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Feb 6, 2020

If I understand correctly, the proposal by @EricCousineau-TRI would solve this problem.

@wieset How would it solve the problem if the library you are trying to find / load isn't already loaded?

@EricCousineau-TRI
Copy link
Author

@EricCousineau-TRI EricCousineau-TRI commented Feb 6, 2020

@ivanpauno Probably not anytime in the near future (~6mo to 1yr), unfortunately :(

@wieset
Copy link

@wieset wieset commented Feb 10, 2020

@dirk-thomas I looked at @EricCousineau-TRI's example code and you are right, it would not solve my problem if the library isn't already loaded. I guess I could link it at build time so that it gets loaded at program startup?

However, to preserve loading at run time, I see two options: Either RPATH would have to be searched, which I can set through CMAKE_INSTALL_RPATH, or another environment variable could be used instead of LD_LIBRARY_PATH, e.g. RMW_LIBRARY_PATH. I mentioned both those options in ros2/rcpputils#40.

@dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Feb 10, 2020

@wieset Customizing your build to use rpath for this use case sounds like the way to go.

@wieset
Copy link

@wieset wieset commented Feb 10, 2020

@dirk-thomas Agreed, and I am already using RPATH to load all other libraries. But that doesn't get me around the fact that find_library_path() for the rmw library currently doesn't look there. Or am I missing something?

@dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Feb 10, 2020

But that doesn't get me around the fact that find_library_path() for the rtw library currently doesn't look there.

You are right. I missed the context on this ticket.

I wonder if it makes sense to introduce a separate environment variable for this or if you should just make sure when running executables as root to set the existing environment variables yourself explicitly.

@wieset
Copy link

@wieset wieset commented Feb 10, 2020

As far as I understand, LD_LIBRARY_PATH is ignored completely for setcap / setuid executables, so setting it myself wouldn't help unfortunately. See http://man7.org/linux/man-pages/man8/ld.so.8.html

@dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Feb 10, 2020

Not sure if introducing a new env var like ROS_LIBRARY_PATH to bypass that security limitation is a good idea. ld only wouldn't filter that because it doesn't know about it and it ignores LD_LIBRARY_PATH due to security considerations.

Please feel free to propose PRs to add this enhancement (as indicated by the "help wanted" label).

@wieset
Copy link

@wieset wieset commented Feb 11, 2020

Will look into into it!

@wieset
Copy link

@wieset wieset commented Feb 12, 2020

Created a pull request at ros2/rcpputils#44

@EricCousineau-TRI
Copy link
Author

@EricCousineau-TRI EricCousineau-TRI commented Dec 10, 2020

FWIW @hidmic, @IanTheEngineer, and I are starting to discuss this with other folk at TRI. First towards our internal uses, then towards integration with things like Drake.

@hidmic hidmic self-assigned this Dec 10, 2020
@hidmic
Copy link
Contributor

@hidmic hidmic commented Dec 11, 2020

Hmm, I'm starting to wonder why we need rcpputils::find_library_path() to begin with. If we simply provide rcutils_load_shared_library() or its rcpputils::SharedLibrary wrapper with a relative path, dlopen and LoadLibrary will search paths and yield loaded object files. Documentation for both suggests that already loaded object files will not be pulled in again. By doing this, RPATHs, LD_LIBRARY_PATHs, RUNPATHs, PATHs, and preloaded libraries would be honored by default.

CC @EricCousineau-TRI @wieset @clalancette.

@hidmic
Copy link
Contributor

@hidmic hidmic commented Dec 11, 2020

Alright, see #320 and connected PRs for a first stab at this. @wieset this should (indirectly) solve your issues as well. These changes render rcpputils::find_library_path obsolete though (for usage in core packages).

@clalancette
Copy link
Contributor

@clalancette clalancette commented Jan 4, 2021

@hidmic Now that we've landed #320 and friends, can we close this out?

@hidmic
Copy link
Contributor

@hidmic hidmic commented Jan 4, 2021

Indeed we can. Thanks for the bump!

@hidmic hidmic closed this Jan 4, 2021
@wieset
Copy link

@wieset wieset commented Jan 8, 2021

Thanks a lot @hidmic, I think this gets me halfway there. RUNPATH still needs to get populated properly. I'll continue the discussion with @clalancette in ros2/rcpputils#44.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants