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

Correctly parse ELF for musllinux on Big Endian #538

Merged
merged 4 commits into from
Jun 17, 2022

Conversation

uranusjr
Copy link
Member

Based on test binary provided in #473 (comment)

Note that this does not actually fix anything. The current implementation would yield the correct result on a BE system when reading a BE Python interpreter, which is what should happen (a Python interpreter with non-matching endian won’t run to begin with). This simply makes the logic more explicit so the unit tests pass on a BE system, even though they only test parsing LE executables.

@glaubitz
Copy link

This still fails for me with the following error:

[   98s] =================================== FAILURES ===================================
[   98s] ______________ test_parse_ld_musl_from_elf_no_interpreter_section ______________
[   98s] 
[   98s]     def test_parse_ld_musl_from_elf_no_interpreter_section():
[   98s]         with BIN_MUSL_X86_64.open("rb") as f:
[   98s]             data = f.read()
[   98s]     
[   98s]         # Change all sections to *not* PT_INTERP.
[   98s]         unpacked = struct.unpack("16BHHIQQQIHHH", data[:58])
[   98s]         *_, e_phoff, _, _, _, e_phentsize, e_phnum = unpacked
[   98s]         for i in range(e_phnum + 1):
[   98s]             sb = e_phoff + e_phentsize * i
[   98s]             se = sb + 56
[   98s] >           section = struct.unpack("IIQQQQQQ", data[sb:se])
[   98s] E           struct.error: unpack requires a buffer of 56 bytes
[   98s] 
[   98s] tests/test_musllinux.py:110: error
[   98s] ============================== 2 tests deselected ==============================
[   98s] ======= 1 failed, 29293 passed, 2 deselected, 1 xfailed in 47.12 seconds =======

Tested on SUSE Linux Enterprise Server 15 with Python 3.6.

@uranusjr
Copy link
Member Author

Ah, right, it’s because the test still uses endian-sensitive rules.

@uranusjr
Copy link
Member Author

I pushed a change for that specific test; does this work?

In the long term I think we should probably extract the ELF parsing logic into a common class that can be used across musllinux, manylinux, and in test code.

@glaubitz
Copy link

I pushed a change for that specific test; does this work?

Unfortunately not:

[   87s] =================================== FAILURES ===================================
[   87s] ______________ test_parse_ld_musl_from_elf_no_interpreter_section ______________
[   87s] 
[   87s]     def test_parse_ld_musl_from_elf_no_interpreter_section():
[   87s]         with BIN_MUSL_X86_64.open("rb") as f:
[   87s]             data = f.read()
[   87s]     
[   87s]         # Change all sections to *not* PT_INTERP. We are explicitly using LSB rules
[   87s]         # because the binaries are in LSB.
[   87s] >       unpacked = struct.unpack("16B<HHIQQQIHHH", data[:58])
[   87s] E       struct.error: bad char in struct format
[   87s] 
[   87s] tests/test_musllinux.py:106: error
[   87s] ============================== 2 tests deselected ==============================

@uranusjr
Copy link
Member Author

LOL because I messed it up. It should be <16BHHIQQQIHHH, not 16B<HHIQQQIHHH. Try that instead.

@uranusjr uranusjr force-pushed the musllinux-on-big-endian branch from 288a13e to a339dd3 Compare May 29, 2022 09:07
@glaubitz
Copy link

Works now, thanks!

@brettcannon brettcannon enabled auto-merge (squash) June 17, 2022 21:43
@brettcannon brettcannon merged commit c4647b9 into pypa:main Jun 17, 2022
@uranusjr uranusjr deleted the musllinux-on-big-endian branch June 22, 2022 15:10
hrnciar pushed a commit to hrnciar/packaging that referenced this pull request Jul 1, 2022
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.

3 participants