Skip to content

Adding #[track_caller] on #[alloc_error_handler] seems to cause undefined behavior #83430

Open
Listed in
@repnop

Description

@repnop
Contributor

Playground link: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=c830f805f27e1be14b718b4d7510bf66

Example playground output:

Standard Error

   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 1.67s
     Running `target/debug/playground`
timeout: the monitored command dumped core
/playground/tools/entrypoint.sh: line 11:     8 Segmentation fault      timeout --signal=KILL ${timeout} "$@"

Standard Output

panicked at 'oof: Layout { size_: 4, align_: 4 }', 

(It seems to print garbage in release mode)

While trying to debug my OS kernel, I slapped #[track_caller] on my #[alloc_error_handler] to try and figure out what was attempting to allocate memory that caused an OoM, however every time I hit that it would hang on an invalid memory access when it went to print the source location for the panic. I was able to reproduce the issue by... sternly talking the playground into producing a #![no_std] binary and running it with a global allocator that always fails.

Activity

changed the title [-]Adding #[track_caller] on #[alloc_error_handler] seems to have undefined behavior[/-] [+]Adding #[track_caller] on #[alloc_error_handler] seems to cause undefined behavior[/+] on Mar 24, 2021
nagisa

nagisa commented on Mar 24, 2021

@nagisa
Member

#[track_caller] adjusts the signature of a function, and the caller has its own specific expectations. The #[alloc_error_handler] should be unsafe.

repnop

repnop commented on Mar 29, 2021

@repnop
ContributorAuthor

I don't understand how making the alloc error handler function unsafe helpers here.

memoryruins

memoryruins commented on Mar 29, 2021

@memoryruins
Contributor

@rustbot label F-track_caller

nagisa

nagisa commented on Mar 29, 2021

@nagisa
Member

At the machine code level the function is expected to be defined with a specific signature. Defining the function with any other signature results in the caller invoking UB. Making it the attribute to imply its unsafe somehow (cf. #82499) would then properly indicate that there are invariants user must satisfy. It seems, however, that we're already checking the signature for correctness in some ways, so the check could be simply adjusted to also disallow any attribute that adjusts the function ABI such as track_caller.

repnop

repnop commented on Mar 29, 2021

@repnop
ContributorAuthor

after searching for how the alloc error handler is implemented, I see what you're getting at. (I assumed it directly inserted a call to that and didn't go through a bunch of unsafe extern "C" machinery) I think erroring on any ABI other than the Rust ABI would be far better than marking the function as unsafe and letting things explode if the user specifies an invalid ABI (and I just tested that you can apply any ABI to the function... including x86-interrupt which is fun :D)

however, I also think it would be worth trying to figure out a solution here, if possible, to make #[track_caller] work as I would imagine most users who need to implement it themselves will panic anyway, and as it stands right now, it only gives you the span of the panic inside of the alloc error function, which is pretty useless in terms of debugging.

Chronos-Sk

Chronos-Sk commented on May 12, 2021

@Chronos-Sk

The original playground example seems to have stopped working. I think this is a reproducing version in CE, though: https://rust.godbolt.org/z/KWK3vzqqG.

gilescope

gilescope commented on Jul 5, 2021

@gilescope
Contributor

I'm concious that alloc_error_handler is the last unstable piece of the puzzle for some people getting off nightly.
If we ban non-rust abis calling the fn, are we then happy to stabilise alloc_error_handler? - I.e. do we think stabilising alloc_error_handler will have any bearing on whatever potential solution we plan for getting track_caller working? If we don't have to have a hard dependency between these two things then we can split them into two separate issues and unblock the route to stabilisation.

SimonSapin

SimonSapin commented on Jul 11, 2021

@SimonSapin
Contributor

The signature of the handler is checked here:

// Check that a function marked as `#[alloc_error_handler]` has signature `fn(Layout) -> !`
if let Some(alloc_error_handler_did) = tcx.lang_items().oom() {
if alloc_error_handler_did == hir.local_def_id(fn_id).to_def_id() {
if let Some(alloc_layout_did) = tcx.lang_items().alloc_layout() {
if *declared_ret_ty.kind() != ty::Never {
sess.span_err(decl.output.span(), "return type should be `!`");
}
let inputs = fn_sig.inputs();
let span = hir.span(fn_id);
if inputs.len() == 1 {
let arg_is_alloc_layout = match inputs[0].kind() {
ty::Adt(ref adt, _) => adt.did == alloc_layout_did,
_ => false,
};
if !arg_is_alloc_layout {
sess.span_err(decl.inputs[0].span, "argument should be `Layout`");
}
if let Node::Item(item) = hir.get(fn_id) {
if let ItemKind::Fn(_, ref generics, _) = item.kind {
if !generics.params.is_empty() {
sess.span_err(
span,
"`#[alloc_error_handler]` function should have no type \
parameters",
);
}
}
}
} else {
let span = sess.source_map().guess_head_span(span);
sess.span_err(span, "function should have one argument");
}
} else {
sess.err("language item required, but not found: `alloc_layout`");
}
}
}

We should either find a way to make that code check the effective signature after modifications such as by #[track_caller], or make it check for signature-affecting attributes like #[track_caller].

RalfJung

RalfJung commented on Apr 17, 2024

@RalfJung
Member

Does this issue still exist? The example doesn't build any more as a bunch of the unstable features changed. I'm not sure what the best way is to even try a no_std binary.

I think these days alloc_error_handler is a fairly simple macro that defines a symbol and then calls the function carrying the attribute. So #[track_caller] should work just fine, but won't be very useful as it will just point to inside the macro-generated code that calls your alloc_error_handler.

I do wonder, however, if things like #[panic_handler] or #[start] will work with track_caller...

veluca93

veluca93 commented on Dec 12, 2024

@veluca93
Contributor

Does this issue still exist? The example doesn't build any more as a bunch of the unstable features changed. I'm not sure what the best way is to even try a no_std binary.

I think these days alloc_error_handler is a fairly simple macro that defines a symbol and then calls the function carrying the attribute. So #[track_caller] should work just fine, but won't be very useful as it will just point to inside the macro-generated code that calls your alloc_error_handler.

I do wonder, however, if things like #[panic_handler] or #[start] will work with track_caller...

It seems possible to combine alloc_error_handler and track_caller in invalid ways still, i.e. the following snippet compiles:

#![feature(alloc_error_handler)]

#[track_caller]
#[alloc_error_handler]
fn f(_: std::alloc::Layout) -> ! {
    panic!()
}
RalfJung

RalfJung commented on Dec 12, 2024

@RalfJung
Member

It seems likely that that's useless but harmless, according to my comment that you quoted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    C-bugCategory: This is a bug.F-track_caller`#![feature(track_caller)]`requires-nightlyThis issue requires a nightly compiler in some way.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @SimonSapin@RalfJung@nagisa@gilescope@veluca93

        Issue actions

          Adding #[track_caller] on #[alloc_error_handler] seems to cause undefined behavior · Issue #83430 · rust-lang/rust