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

Check for a branded ELF note when OS/ABI is NONE #1344

Merged
merged 8 commits into from
Mar 3, 2025
Merged

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Sep 12, 2024

Currently the auditor is only checking for the correct OS/ABI and producing a warning on FreeBSD when it doesn't match. This warning is incorrect on AArch64 though, as FreeBSD instead uses an ELF note to declare the OS/ABI. The change implemented here looks for such a note when the OS/ABI in the ELF header is NONE.

Note that there are no tests added... I'd be interested in any suggestions for how best to test this given that the functions I modified are not currently tested, at least directly (AFAICT).

Currently the auditor is only checking for the correct OS/ABI and
producing a warning on FreeBSD when it doesn't match. This warning is
incorrect on AArch64 though, as FreeBSD instead uses an ELF note to
declare the OS/ABI. The change implemented here looks for such a note
when the OS/ABI in the ELF header is NONE.
@giordano
Copy link
Member

I haven't looked at the implementation yet, but I want to quickly answer the question

Note that there are no tests added... I'd be interested in any suggestions for how best to test this given that the functions I modified are not currently tested, at least directly (AFAICT).

Yeah, the problem is that we don't know (or forgot? who knows) how libraries with the wrong OS/ABI field were generated on FreeBSD. An alternative should be to compile a simple dummy library (something like echo '' | cc -x c - -shared -fPIC -o libdummy.${dlext}) and then patchelf --set-os-abi it

@ararslan
Copy link
Member Author

Ah rip, the patchelf we ship is old and doesn't have --set-os-abi

@giordano
Copy link
Member

Sigh, never mind the test then, we can't upgrade patchelf because newer version is broken (JuliaPackaging/Yggdrasil#7728 (comment), caused by NixOS/patchelf#492)

@ararslan
Copy link
Member Author

Finally, 168 days after this PR was first opened, it actually works, is tested, and passes CI.

end

os_from_elf_note(::ObjectHandle) = nothing

Copy link
Member

Choose a reason for hiding this comment

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

Good to know a blank line was not covered by tests!

image

Copy link
Member Author

Choose a reason for hiding this comment

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

@test isempty(readlines("src/auditor/dynamic_linkage.jl")[27])

@ararslan ararslan requested a review from giordano February 28, 2025 01:40
@ararslan
Copy link
Member Author

ararslan commented Mar 3, 2025

Bump :)

@ararslan ararslan merged commit 44cfc61 into master Mar 3, 2025
10 checks passed
@ararslan ararslan deleted the aa/elf-note branch March 3, 2025 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants