rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
95.14k stars 12.27k forks source link

Poor interaction between NLL-borrowck, async, and c_variadic's `...` desugaring (`VaListImpl<'_>`) #125431

Open nnethercote opened 1 month ago

nnethercote commented 1 month ago

124918 made note_and_explain_region abort if ReVar occurred. Fuzzing found a test case that triggers the abort in #124973, having to do with the c_variadic feature:

async fn multiple_named_lifetimes(_: u8, ...) {}

ReVar was re-allowed in #125054.

@compiler-errors had the following analysis:

So the tl;dr is:

https://github.com/rust-lang/rust/blob/9105c57b7f6623310e33f3ee7e48a3114e5190a7/compiler/rustc_borrowck/src/universal_regions.rs#L517-L520

We create an anonymous free region here for the VaListImpl<'_> that c-variadic is implicitly lowered to.

Async functions capture that var arg list (not exactly certain how they end up doing that, though), and so the lifetime ends up being captured by the async fn's future. That ends up erroring because the future ends up capturing a lifetime that it didn't expect, which should probably not happen, but regardless is a problem because we have no way of providing a useful error message even if we don't expect it to work... the diagnostic isn't useful at all.

tmandry commented 1 month ago

I can't reproduce the abort. Playground

   Compiling playground v0.0.1 (/playground)
error: only foreign or `unsafe extern "C"` functions may be C-variadic
 --> src/lib.rs:3:42
  |
3 | async fn multiple_named_lifetimes(_: u8, ...) {}
  |                                          ^^^

error[E0700]: hidden type for `impl Future<Output = ()>` captures lifetime that does not appear in bounds
 --> src/lib.rs:3:47
  |
3 | async fn multiple_named_lifetimes(_: u8, ...) {}
  | --------------------------------------------- ^^
  | |
  | opaque type defined here
  |
  = note: hidden type `{async fn body of multiple_named_lifetimes()}` captures lifetime `'_`

For more information about this error, try `rustc --explain E0700`.
error: could not compile `playground` (lib) due to 2 previous errors

The second error is a little weird, but maybe we can close this issue?

traviscross commented 1 month ago

@rustbot labels +AsyncAwait-Triaged

As above, we discussed this in the async call. We were a bit unclear about whether this is still an issue or if something else was being reported here.

compiler-errors commented 1 month ago

The fact that the error is weird is the bug. Also, there's no clear justification why this code shouldn't compile.

tmandry commented 1 month ago

Also, there's no clear justification why this code shouldn't compile.

As in we could have an extern "C" function defined in Rust which returns an opaque type? I.. guess that's possible, but it would be weird and not actually callable from C.

compiler-errors commented 1 month ago

No, I mean the fact that there's a varargs argument is why this has a borrowck error. This leaks the implementation details of how we implement varargs.

workingjubilee commented 3 weeks ago

Also, there's no clear justification why this code shouldn't compile.

As in we could have an extern "C" function defined in Rust which returns an opaque type? I.. guess that's possible, but it would be weird and not actually callable from C.

I mean, we don't even lint on this program:

use std::ffi::{c_char, c_int};
extern "C" {
    fn printf(c: *const c_char, ...) -> c_int;
}

fn main() {
    let allocated_cstring = c", but it is still UB\n".to_owned();
    unsafe { printf(c"this kinda makes sense%s".as_ptr(), allocated_cstring) };

    let allocated_repr_rust = vec!["lol what"];
    unsafe { printf(c"but this doesn't: %s".as_ptr(), allocated_repr_rust) };
}

we don't seem to be too fussed about "makes sense" versus "technically possible" here. (though, this isn't a program that uses feature(c_variadic) to be clear, but to me it raises the question of "what is Rust's C variable argument support for?").