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

null symbols cause unsoundness with extern static #139128

Open
jasondyoungberg opened this issue Mar 30, 2025 · 8 comments
Open

null symbols cause unsoundness with extern static #139128

jasondyoungberg opened this issue Mar 30, 2025 · 8 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-prioritize Issue: Indicates that prioritization has been requested for this issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness needs-triage This issue may need triage. Remove it if it has been sufficiently triaged.

Comments

@jasondyoungberg
Copy link

Pointers to external statics are incorrectly assumed to be non-null and well-aligned. Edition 2024 marking extern as unsafe does help, but there is no documentation mentioning this, and this still works in 2021 despite forbidding unsafe_code.

#![forbid(unsafe_code)]

extern "C" {
    static mut GLIBC_PRIVATE: u8;
    static perror: u64;
}

fn main() {
    let x = std::ptr::NonNull::new(&raw mut GLIBC_PRIVATE).unwrap();
    println!("{x:?}"); // 0x0

    let y = &raw const perror;
    println!("{y:?} {}", y.is_aligned()); // 0x7c5aa9e5ea93 true
}

https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=4d1319d8ff14e454c3e391474ef299e6

@jasondyoungberg jasondyoungberg added the C-bug Category: This is a bug. label Mar 30, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 30, 2025
@workingjubilee workingjubilee added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Mar 30, 2025
@workingjubilee
Copy link
Member

this is an LLVM bug: it e.g. applies the following transformation to an is_null check over SROA (which is harmless) and EarlyCSEPass (which does the folding to ret i1 false) which seems to be due to assuming globals must be linked to some location.

@GLIBC_PRIVATE = external global i64

define noundef zeroext i1 @example_foo_h402e223ff693409d() unnamed_addr {
start:
-  %self.dbg.spill = alloca [8 x i8], align 8
-  store ptr @GLIBC_PRIVATE, ptr %self.dbg.spill, align 8
-  %_0 = icmp eq i64 ptrtoint (ptr @GLIBC_PRIVATE to i64), 0
-  ret i1 %_0
+  ret i1 false
}

@workingjubilee workingjubilee added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Mar 30, 2025
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 30, 2025
@workingjubilee
Copy link
Member

workingjubilee commented Mar 30, 2025

It is not clear to me that it is permissible to use unsafe extern "C" { static } to declare linkage to a symbol that is not linked. The bug remains even if you use ZSTs, which seems to be a bigger problem.

@workingjubilee
Copy link
Member

I have filed an issue at llvm/llvm-project#133621

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Mar 30, 2025

Responses to the LLVM issue say that the global must be marked with weak linkage (I assume this is ends up as a weak symbol, otherwise I’d expect a linker error). From a Rust POOV I would also argue this is “just” the fault of the unsafe extern "C" block: it claims that there is an allocated object under the name GLIBC_PRIVATE and allocated objects have non-null addresses (and must be aligned for the given type, and can’t overlap other statics). Rust doesn’t have a stable feature for weak linkage yet (#29603) but whatever form it takes, it’ll lower to different LLVM IR and probably also look different in surface Rust than an ordinary static (e.g., forced to always be pointers representing the address the symbol resolved to, if any - cf. #31508).

@workingjubilee
Copy link
Member

If we wish to accept LLVM's decision on this point, instead of implementing some sus workaround, then it seems we should update the reference accordingly. cc @rust-lang/opsem

@RalfJung
Copy link
Member

From a Rust POOV I would also argue this is “just” the fault of the unsafe extern "C" block: it claims that there is an allocated object under the name GLIBC_PRIVATE and allocated objects have non-null addresses (and must be aligned for the given type, and can’t overlap other statics).

Fully agreed. Alignment seems entirely obvious (just like the type of the exten static determines its minimum size), and requiring them to be non-null is entirely consistent as well.

@jasondyoungberg
Copy link
Author

From a Rust POOV I would also argue this is “just” the fault of the unsafe extern "C" block: it claims that there is an allocated object under the name GLIBC_PRIVATE and allocated objects have non-null addresses (and must be aligned for the given type, and can’t overlap other statics).

Fully agreed. Alignment seems entirely obvious (just like the type of the exten static determines its minimum size), and requiring them to be non-null is entirely consistent as well.

What about for unit, and other ZSTs? Since a null pointer is valid for them, then should they be allowed to be linked to address zero / null?

@RalfJung
Copy link
Member

RalfJung commented Mar 31, 2025

A null reference is not valid though, and no actual allocation can exist at the null address, not even allocations of size 0. So I don't think they are special here.

The key point is that a static represents an allocation, not just a pointer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-prioritize Issue: Indicates that prioritization has been requested for this issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness needs-triage This issue may need triage. Remove it if it has been sufficiently triaged.
Projects
None yet
Development

No branches or pull requests

5 participants