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

[repnop]

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.

[nagisa]

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

[repnop]

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

[memoryruins]

@rustbot label F-track_caller

[nagisa]

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]

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]

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]

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]

The signature of the handler is checked here:

rust/compiler/rustc_typeck/src/check/check.rs

Lines 372 to 411 in 53cb7b0

	// 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]

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]

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]

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