Skip to content

FFI + release mode + passing pointer to temporary value to C function produces an invalid result #136676

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

Open
Maykeye opened this issue Feb 7, 2025 · 4 comments
Labels
C-gub Category: the reverse of a compiler bug is generally UB

Comments

@Maykeye
Copy link

Maykeye commented Feb 7, 2025

Playing around with SDL3 crate I've met behavior which is interestingly even caught in C++: foo(&bar()) where bar returns struct in C gives error like "Taking the address of a temporary object of type 'T'". When rust calls C function, it allows similar code and bad things might happen in release mode:

The code (optionally) creates C struct and calls C func passing a pointer to it.

use std::ffi::c_float;
#[repr(C)]
struct DataFFI { pub a: c_float, pub b: c_float }
fn make_data(x:f32) -> DataFFI { DataFFI { a: x, b: x } }
extern "C" { fn sum_me(dat: *const DataFFI) -> c_float; }
fn do_sum(dat: Option<f32>) -> f32
{
    unsafe {
        sum_me(
            match dat { Some(x) => &make_data(x), None => std::ptr::null() },
        )
    }
}

fn main()
{
    println!("expect: 0, got: {}", do_sum(None));
    println!("expect: 8, got: {}", do_sum(Some(4.0)));
}

build.rs is done like this

fn main()
{
    println!("cargo::rustc-flags=-L/home/fella/src/memtest/c");
    println!("cargo::rustc-flags=-lbackend");
}   

C function is

//gcc -shared -o libbackend.so -fPIC backend.c , compiled inside `c` folder
#include <stdio.h>
typedef struct Data { float a,b; } Data;
const Data nul_data={0.0};
float sum_me(const Data* dat)
{
    float res=0.0;
    printf("DAT(%p):\n", dat);
    dat = dat ? dat : &nul_data;
    res += dat->a; printf("%f ", res); res += dat->b; printf("%f\n", res);
    return res;
}

I expected to see this happen: explanation

2  ╰ LD_LIBRARY_PATH=/home/fella/src/memtest/c/ cargo run 
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
     Running `target/debug/memtest`
DAT((nil)):
0.000000 0.000000
expect: 0, got: 0
DAT(0x7ffef9cfdb3c):
4.000000 8.000000
expect: 8, got: 8

And this happens in debug mode: when dat is passed it contains what we want. But in release mode

Instead, this happened:

✦2  ╰ LD_LIBRARY_PATH=/home/fella/src/memtest/c/ cargo run --release
    Finished `release` profile [optimized] target(s) in 0.00s
     Running `target/release/memtest`
DAT((nil)):
0.000000 0.000000
expect: 0, got: 0
DAT(0x7fff5f15e0c0):
-1833370141065216.000000 -1833370141065216.000000
expect: 8, got: -1833370100000000

(Also interestingly in this run we have -1833370100000000 without 41065216)

It relies on both using address from return value and match. If we use

fn do_sum(dat: Option<f32>) -> f32
{
    unsafe {
        let real_dat;
        sum_me(
            match dat { Some(x) => {
                real_dat = make_data(x);
                &real_dat
            },
            None => std::ptr::null() },
        )
    }
}

or sum_dat(&make_data()) it works.

Meta

rustc --version --verbose:

rustc 1.84.1 (e71f9a9a9 2025-01-27)
binary: rustc
commit-hash: e71f9a9a98b0faf423844bf0ba7438f29dc27d58
commit-date: 2025-01-27
host: x86_64-unknown-linux-gnu
release: 1.84.1
LLVM version: 19.1.5

Backtrace

<backtrace>

@Maykeye Maykeye added the C-bug Category: This is a bug. label Feb 7, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 7, 2025
@Maykeye Maykeye changed the title FFI + release mode + and passing pointer to temporary value to C function produces an invalid result FFI + release mode + passing pointer to temporary value to C function produces an invalid result Feb 7, 2025
@theemathas
Copy link
Contributor

As per https://doc.rust-lang.org/stable/reference/destructors.html#r-destructors.scope.match-arm, as soon as the match arm ends, the result of make_data(x) is dropped, causing the reference to be dangling. Therefore, your code has undefined behavior, and this is not a bug in Rust.

It seems like rust didn't detect this because the other match arm had null(), which is a *const DataFFI, so rust coerced the &DataFFI to *const DataFFI, which immediately became dangling.

Here's the same behavior reproduced in pure Rust code (This code prints garbage values in release mode):

struct DataFFI {
    pub a: f32,
    pub b: f32,
}

fn make_data(x: f32) -> DataFFI {
    DataFFI { a: x, b: x }
}

fn do_sum(dat: Option<f32>) {
    sum_me(match dat {
        Some(x) => &make_data(x),
        None => std::ptr::null(),
    })
}

fn main() {
    do_sum(None);
    do_sum(Some(4.0));
}

#[inline(never)]
fn sum_me(dat: *const DataFFI) {
    unsafe {
        if !dat.is_null() {
            println!("{}", (*dat).a);
        }
    }
}

Running this code in Miri causes an error of use-after-free, as expected.

@jieyouxu jieyouxu added C-gub Category: the reverse of a compiler bug is generally UB and removed C-bug Category: This is a bug. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 7, 2025
@Maykeye
Copy link
Author

Maykeye commented Feb 7, 2025

Shouldn't rustc by itself be more aggressive at detecting such leaking dangling references/pointers?

Miri doesn't find anything if it's not being run and this bug exists in sdl3-rs for two years.

In case like this rustc knows object is dropped, as it drops automatically and copies pointer outside of drop.

@jieyouxu jieyouxu added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. and removed C-gub Category: the reverse of a compiler bug is generally UB labels Feb 7, 2025
@theemathas
Copy link
Contributor

Clippy issues that propose detecting similar stuff: rust-lang/rust-clippy#2045, rust-lang/rust-clippy#5965, rust-lang/rust-clippy#7311, rust-lang/rust-clippy#10959.

Existing rustc lint that detects similar stuff, but surprisingly doesn't detect the case of &foo() as *const _: https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#dangling-pointers-from-temporaries

@saethlin saethlin added C-gub Category: the reverse of a compiler bug is generally UB and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 7, 2025
@Maykeye
Copy link
Author

Maykeye commented Mar 2, 2025

Found a relatively easy way to detect this UB(at least the case as was in sdl3, it now fixed). It "justs" needs MIR as the code gets down to two consequent MIR instructions: getting a pointer to something, then dropping that something.

It can be detected by grep:

$ grep -nP "= &raw (const|mut) (_\d+);\n\s+StorageDead\(\2\)"  sdl3-0.12.0-5b12244.mir
118453:        _0 = &raw const _3;
118454|        StorageDead(_3);
118792:        _8 = &raw const _12;
118793|        StorageDead(_12);
118830:        _13 = &raw const _18;
118831|        StorageDead(_18);
119069:        _13 = &raw const _17;
119070|        StorageDead(_17);
119107:        _18 = &raw const _23;
119108|        StorageDead(_23);
119139:        _24 = &raw const _29;
119140|        StorageDead(_29);
151644:        _5 = &raw const _9;
151645|        StorageDead(_9);

(It may not detect all cases, probably will not if pointer is taken to not the latest expr, but to expr in the middle)

does compiler have a stage where it runs through MIR to check that generated code makes sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-gub Category: the reverse of a compiler bug is generally UB
Projects
None yet
Development

No branches or pull requests

5 participants