elf: fix is_import to check st_shndx #436
Conversation
There was a problem hiding this comment.
As mentioned in comment, swapping out these functions to suddenly use shndx doesn't seem right to me; adding the STB_WEAK check in the is_import function is good catch (though it now has me wondering whether all these functions should be marked private).
However, we can review adding a new api that uses the shndx in another patch if that is something people really want, or some other design that incorporates it somehow.
| @@ -94,9 +94,9 @@ pub fn st_visibility(other: u8) -> u8 { | |||
|
|
|||
| /// Is this information defining an import? | |||
| #[inline] | |||
| pub fn is_import(info: u8, value: u64) -> bool { | |||
| pub fn is_import(info: u8, shndx: u64) -> bool { | |||
There was a problem hiding this comment.
idly wondering if we should de-pub this along with the other functions; i wonder why they were made pub, could have been a historical accident or perhaps someone uses them for whatever reason; i don't see bingrep for example using it anywhere
| let bind = st_bind(info); | ||
| bind == STB_GLOBAL && value == 0 | ||
| (bind == STB_GLOBAL || bind == STB_WEAK) && shndx == SHN_UNDEF as _ |
There was a problem hiding this comment.
this is a good catch (the missing STB_WEAK), but changing this function to operate semantically on shndx values instead of st_value, while backwards compatible, has wider implications.
for example, any clients relying on st_value being used in these computations will now break when section headers are stripped.
imho my general thesis that what a dynamic linker can import is how we've defined import, while arguably more nuanced than this, is a good one and has value (no pun intended). using st_value is a heuristic for that thesis.
that being said, it seems like people are hitting some issues with this, so we can consider adding something like is_import_from_sections or something which is new api to opt into using the section header (perhaps along with st_value) as the determinant.
Closes #288
Closes #419
I also found this bug on with ELF produced on illumos.
st_valuemay not be zero because it may indicate the offset of a import function.