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

os/linux/elf: avoid using ldd for listing dynamic dependencies #16941

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

alebcay
Copy link
Member

@alebcay alebcay commented Mar 23, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Current approach is to query ldd and capture its output (which includes resolved paths to shared libraries), then filters based on the needed libraries determined through checking the dynamic section.

New approach is to search for each needed library listed in the dynamic section, trying to locate it via runpath/rpath and the system ld.so.conf paths.

With the new approach, we avoid passing (potentially untrusted) files to ldd. Instead, we analyze the ELF file statically (inspect the dynamic section) and only try to perform lookups for the shared libraries indicated in the dynamic section.

This change is intended to modify how the dynamic dependencies lookup happens, but should not alter any internal or public-facing API (only private methods/classes changed) - for instance, the output semantics, format, and type of Pathname("/some/path/to/an/executable").dynamically_linked_libraries should remain unchanged.

@alebcay alebcay force-pushed the elf-avoid-ldd branch 2 times, most recently from b630d4f to 63bb563 Compare March 23, 2024 03:59
@alebcay alebcay force-pushed the elf-avoid-ldd branch 3 times, most recently from 990b4a3 to b692204 Compare March 25, 2024 06:02
@MikeMcQuaid MikeMcQuaid requested a review from Bo98 March 25, 2024 08:39
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Looks good!

Just as a couple questions:

  • Have you tested this on a few formulae?
  • What's the performance difference? Faster/slower and by how much?

Library/Homebrew/os/linux/ld.rb Outdated Show resolved Hide resolved
@alebcay alebcay force-pushed the elf-avoid-ldd branch 2 times, most recently from a207fe1 to 18d1842 Compare March 29, 2024 16:07
@alebcay
Copy link
Member Author

alebcay commented Mar 29, 2024

So far I've spot-tested a few binaries/libraries, like zsh binary from the zsh formula and libleptonica.so.6 from leptonica, to confirm that the output of Pathname(<path to ELF file>).dynamically_linked_libraries is the same. Open to any ideas on how to test further, especially for covering the case where DF_1_NODEFLIB is set.

As far as performance, the performance seems to be about the same. Testing with the following:

def snippet()= 100.times { Pathname("/home/linuxbrew/.linuxbrew/Cellar/leptonica/1.84.1/lib/libleptonica.so.6").dynamically_linked_libraries }
Benchmark.measure { snippet }

Before:

=> #<Benchmark::Tms:0x00000040084902a8
 @cstime=1.1855609999999928,
 @cutime=7.002927,
 @label="",
 @real=13.041561631999684,
 @stime=0.21336699999999809,
 @total=11.345018999999986,
 @utime=2.943163999999996>

After:

=> #<Benchmark::Tms:0x0000004008601da8
@cstime=1.219255,
@cutime=7.094854,
@label="",
@real=13.061064489999808,
@stime=0.18731199999999998,
@total=11.323314,
@utime=2.821893>

The above "after" result is with no caching mechanism implemented.

As mentioned earlier, there is possibly room for caching-related improvements here:

  • Cache the output of ld.so --list-diagnostics
    • Per brew invocation, i.e. memoize (don't persist to disk)
    • Persistent cache - need to figure out what is the right interval for invalidation (this feels a bit overkill)
  • Read/parse/use ld.so.cache before trying to check library paths (and memoize/cache this)

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Nice work so far, a few suggested tweaks, feel free to push back on any.

Library/Homebrew/extend/pathname.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux/ld.rb Outdated Show resolved Hide resolved
@alebcay alebcay force-pushed the elf-avoid-ldd branch 2 times, most recently from 2efd255 to 1b120d2 Compare April 8, 2024 21:35
dduugg
dduugg previously requested changes Apr 13, 2024
Library/Homebrew/os/linux/ld.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux/ld.rb Outdated Show resolved Hide resolved
Library/Homebrew/utils/path.rb Outdated Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good, thanks again @alebcay!

@MikeMcQuaid MikeMcQuaid merged commit 0fc9c9f into Homebrew:master Apr 15, 2024
25 checks passed
@carlocab
Copy link
Member

Might need a revert. Homebrew/homebrew-core#169088

On mobile at the moment, so would appreciate if someone else got to it.

@github-actions github-actions bot added the outdated PR was locked due to age label May 16, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants