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.

Sign up for GitHub

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

elf: fix is_import to check st_shndx #436

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

elf: fix is_import to check st_shndx #436

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/elf/sym.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Owner

Choose a reason for hiding this comment

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

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 _
Copy link
Owner

Choose a reason for hiding this comment

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

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.

}

/// Convenience function to get the &'static str type from the symbols `st_info`.
Expand Down Expand Up @@ -170,8 +170,7 @@ macro_rules! elf_sym_std_impl {
/// Checks whether this `Sym` has `STB_GLOBAL`/`STB_WEAK` bind and a `st_value` of 0
#[inline]
pub fn is_import(&self) -> bool {
let bind = self.st_info >> 4;
(bind == STB_GLOBAL || bind == STB_WEAK) && self.st_value == 0
is_import(self.st_info, self.st_shndx as _)
}
/// Checks whether this `Sym` has type `STT_FUNC`
#[inline]
Expand Down Expand Up @@ -336,6 +335,8 @@ use core::fmt;
use scroll::ctx;
use scroll::ctx::SizeWith;

use super::section_header::SHN_UNDEF;

#[derive(Clone, Copy, PartialEq, Default)]
/// A unified Sym definition - convertible to and from 32-bit and 64-bit variants
pub struct Sym {
Expand All @@ -355,8 +356,7 @@ impl Sym {
/// Checks whether this `Sym` has `STB_GLOBAL`/`STB_WEAK` bind and a `st_value` of 0
#[inline]
pub fn is_import(&self) -> bool {
let bind = self.st_bind();
(bind == STB_GLOBAL || bind == STB_WEAK) && self.st_value == 0
is_import(self.st_info, self.st_shndx as _)
}
/// Checks whether this `Sym` has type `STT_FUNC`
#[inline]
Expand Down
Loading