Skip to content

Commit

Permalink
unix: hide symbols beloning to dependencies on Linux
Browse files Browse the repository at this point in the history
This partially addresses #114.

Previously, we were exporting symbols from dependencies like OpenSSL
and X11 from our dynamic binaries. This can cause problems when other
libraries are loaded into the same address space and attempt to bind
to the symbols being exported by Python.

This commit changes things such that symbols for Python dependencies
are now hidden and can no longer be bound to. Python's own symbols
are still exported of course, as Python extensions (and the `python`
executable) need to call into them.

This change is only implemented on Linux for the moment. Apple
targets require a bit more effort to make work.

As part of this, we update validation to confirm visibility of certain
symbols in ELF binaries. The new validation logic correctly rejects
distributions built before this change.
  • Loading branch information
indygreg committed May 1, 2022
1 parent a9bb790 commit 924693c
Show file tree
Hide file tree
Showing 3 changed files with 148 additions and 2 deletions.
11 changes: 11 additions & 0 deletions cpython-unix/build-cpython.sh
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,17 @@ fi
# So we need to set both.
CFLAGS="${EXTRA_TARGET_CFLAGS} -fPIC -I${TOOLS_PATH}/deps/include -I${TOOLS_PATH}/deps/include/ncursesw"
LDFLAGS="${EXTRA_TARGET_LDFLAGS} -L${TOOLS_PATH}/deps/lib"

# Some target configurations use `-fvisibility=hidden`. Python's configure handles
# symbol visibility properly itself. So let it do its thing.
CFLAGS=${CFLAGS//-fvisibility=hidden/}

# But some symbols from some dependency libraries are still non-hidden for some
# reason. We force the linker to do our bidding.
if [ "${PYBUILD_PLATFORM}" != "macos" ]; then
LDFLAGS="${LDFLAGS} -Wl,--exclude-libs,ALL"
fi

EXTRA_CONFIGURE_FLAGS=

if [ "${PYBUILD_PLATFORM}" = "macos" ]; then
Expand Down
11 changes: 11 additions & 0 deletions cpython-unix/targets.yml
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ i686-unknown-linux-gnu:
target_cc: clang
target_cflags:
- '-m32'
- '-fvisibility=hidden'
target_ldflags:
- '-m32'
needs:
Expand Down Expand Up @@ -606,6 +607,8 @@ x86_64-unknown-linux-gnu:
host_cc: clang
host_cxx: clang++
target_cc: clang
target_cflags:
- '-fvisibility=hidden'
needs:
- bdb
- binutils
Expand Down Expand Up @@ -644,6 +647,7 @@ x86_64_v2-unknown-linux-gnu:
target_cc: clang
target_cflags:
- '-march=x86-64-v2'
- '-fvisibility=hidden'
needs:
- bdb
- binutils
Expand Down Expand Up @@ -682,6 +686,7 @@ x86_64_v3-unknown-linux-gnu:
target_cc: clang
target_cflags:
- '-march=x86-64-v3'
- '-fvisibility=hidden'
needs:
- bdb
- binutils
Expand Down Expand Up @@ -720,6 +725,7 @@ x86_64_v4-unknown-linux-gnu:
target_cc: clang
target_cflags:
- '-march=x86-64-v4'
- '-fvisibility=hidden'
needs:
- bdb
- binutils
Expand Down Expand Up @@ -756,6 +762,8 @@ x86_64-unknown-linux-musl:
host_cc: clang
host_cxx: clang++
target_cc: musl-clang
target_cflags:
- '-fvisibility=hidden'
needs:
- bdb
- binutils
Expand Down Expand Up @@ -795,6 +803,7 @@ x86_64_v2-unknown-linux-musl:
target_cc: musl-clang
target_cflags:
- '-march=x86-64-v2'
- '-fvisibility=hidden'
needs:
- bdb
- binutils
Expand Down Expand Up @@ -834,6 +843,7 @@ x86_64_v3-unknown-linux-musl:
target_cc: musl-clang
target_cflags:
- '-march=x86-64-v3'
- '-fvisibility=hidden'
needs:
- bdb
- binutils
Expand Down Expand Up @@ -873,6 +883,7 @@ x86_64_v4-unknown-linux-musl:
target_cc: musl-clang
target_cflags:
- '-march=x86-64-v4'
- '-fvisibility=hidden'
needs:
- bdb
- binutils
Expand Down
128 changes: 126 additions & 2 deletions src/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ use {
anyhow::{anyhow, Context, Result},
clap::ArgMatches,
object::{
elf::{FileHeader32, FileHeader64},
elf::{
FileHeader32, FileHeader64, ET_DYN, ET_EXEC, STB_GLOBAL, STB_WEAK, STV_DEFAULT,
STV_HIDDEN,
},
macho::{MachHeader32, MachHeader64},
read::{
elf::{Dyn, FileHeader, SectionHeader, Sym},
Expand Down Expand Up @@ -440,6 +443,75 @@ const MACHO_ALLOWED_WEAK_SYMBOLS_38_NON_AARCH64: &[&str] = &[
"_statvfs",
];

/// Symbols defined in dependency packages.
///
/// We use this list to spot test behavior of symbols belonging to dependency packages.
/// The list is obviously not complete.
const DEPENDENCY_PACKAGE_SYMBOLS: &[&str] = &[
// libX11
"XClearWindow",
"XFlush",
// OpenSSL
"BIO_ADDR_new",
"BN_new",
"DH_new",
"SSL_extension_supported",
"SSL_read",
"CRYPTO_memcmp",
"ecp_nistz256_neg",
"OPENSSL_instrument_bus",
"x25519_fe64_add",
// libdb
"__txn_begin",
// libedit / readline
"rl_prompt",
"readline",
"current_history",
"history_expand",
// libffi
"ffi_call",
"ffi_type_void",
// ncurses
"new_field",
"set_field_term",
"set_menu_init",
"winstr",
// gdbm
"gdbm_close",
"gdbm_import",
// sqlite3
"sqlite3_initialize",
"sqlite3_close",
// libxcb
"xcb_create_window",
"xcb_get_property",
// libz
"deflateEnd",
"gzclose",
"inflate",
// tix
"Tix_DItemCreate",
"Tix_GrFormat",
// liblzma
"lzma_index_init",
"lzma_stream_encoder",
// tcl
"Tcl_Alloc",
"Tcl_ChannelName",
"Tcl_CreateInterp",
// tk
"TkBindInit",
"TkCreateFrame",
"Tk_FreeGC",
];

const PYTHON_EXPORTED_SYMBOLS: &[&str] = &[
"Py_Initialize",
"PyList_New",
// From limited API.
"Py_CompileString",
];

static WANTED_WINDOWS_STATIC_PATHS: Lazy<BTreeSet<PathBuf>> = Lazy::new(|| {
[
PathBuf::from("python/build/lib/libffi.lib"),
Expand Down Expand Up @@ -478,6 +550,9 @@ pub struct ValidationContext {
/// Dynamic libraries required to be loaded.
pub seen_dylibs: BTreeSet<String>,

/// Symbols exported from dynamic libpython library.
pub libpython_exported_symbols: BTreeSet<String>,

/// Undefined Mach-O symbols that are required / non-weak.
pub macho_undefined_symbols_strong: RequiredSymbols,

Expand All @@ -490,6 +565,8 @@ impl ValidationContext {
pub fn merge(&mut self, other: Self) {
self.errors.extend(other.errors);
self.seen_dylibs.extend(other.seen_dylibs);
self.libpython_exported_symbols
.extend(other.libpython_exported_symbols);
self.macho_undefined_symbols_strong
.merge(other.macho_undefined_symbols_strong);
self.macho_undefined_symbols_weak
Expand Down Expand Up @@ -699,6 +776,38 @@ fn validate_elf<'data, Elf: FileHeader<Endian = Endianness>>(
}
}
}

// Ensure specific symbols in dynamic binaries have proper visibility.
if matches!(elf.e_type(endian), ET_EXEC | ET_DYN) {
// Python 3.8 exports ffi symbols for legacy reasons.
let is_exception = name == "ffi_type_void" && python_major_minor == "3.8";

// Non-local symbols belonging to dependencies should have hidden visibility
// to prevent them from being exported.
if DEPENDENCY_PACKAGE_SYMBOLS.contains(&name.as_ref())
&& matches!(symbol.st_bind(), STB_GLOBAL | STB_WEAK)
&& symbol.st_visibility() != STV_HIDDEN
&& !is_exception
{
context.errors.push(format!(
"{} contains non-hidden dependency symbol {}",
path.display(),
name
));
}

if let Some(filename) = path.file_name() {
let filename = filename.to_string_lossy();

if filename.starts_with("libpython") && filename.ends_with(".so.1.0") {
if matches!(symbol.st_bind(), STB_GLOBAL | STB_WEAK)
&& symbol.st_visibility() == STV_DEFAULT
{
context.libpython_exported_symbols.insert(name.to_string());
}
}
}
}
}
}
}
Expand Down Expand Up @@ -1121,7 +1230,7 @@ fn validate_distribution(
}
}

// We've now read the contents of the archive. Move on to analyizing the results.
// We've now read the contents of the archive. Move on to analyzing the results.

for path in seen_symlink_targets {
if !seen_paths.contains(&path) {
Expand Down Expand Up @@ -1160,6 +1269,21 @@ fn validate_distribution(
}
}

// If we've collected symbols exported from libpython, ensure the Python symbols are
// in the set.
if !context.libpython_exported_symbols.is_empty() {
for symbol in PYTHON_EXPORTED_SYMBOLS {
if !context
.libpython_exported_symbols
.contains(&symbol.to_string())
{
context
.errors
.push(format!("libpython does not export {}", symbol));
}
}
}

// On Apple Python 3.8 we need to ban most weak symbol references because 3.8 doesn't have
// the proper runtime guards in place to prevent them from being resolved at runtime,
// which would lead to a crash. See
Expand Down

0 comments on commit 924693c

Please sign in to comment.