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

Wrong bindings for long double on Linux, but not on macOS #1338

Closed
lovesegfault opened this issue Jun 24, 2018 · 9 comments
Closed

Wrong bindings for long double on Linux, but not on macOS #1338

lovesegfault opened this issue Jun 24, 2018 · 9 comments

Comments

@lovesegfault
Copy link

Input C/C++ Header

typedef struct {
  long double a
} max_align_t;

Bindgen Invocation

fn get_include_paths() -> Vec<String> {
    let mut paths: Vec<String> = vec![];
    pkg_config::Config::new()
        .print_system_libs(false)
        .statik(true)
        .probe("aravis-0.6")
        .unwrap()
        .include_paths
        .into_iter()
        .map(|p| format!("{}", p.display()))
        .for_each(|p| paths.push(p));
    paths
}

let mut bindings = bindgen::Builder::default()
    .header("wrapper.h")
    .no_copy("(?i)mutex");
for header in get_include_paths() {
     bindings = bindings.clang_arg(format!("-I{}", header))
}
bindings.dump_preprocessed_input().unwrap();
let bindings = bindings.generate().expect("Unable to generate bindings");

Actual Results

---- bindgen_test_layout_max_align_t stdout ----
        thread 'bindgen_test_layout_max_align_t' panicked at 'assertion failed: `(left == right)`
  left: `24`,
 right: `32`: Size of: max_align_t', /home/bemeurer/src/standard/sensord/target/debug/build/aravis_sys-cc274a742a798c12/out/bindings.rs:85:51815
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:221
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:463
   5: std::panicking::begin_panic_fmt
             at libstd/panicking.rs:350
   6: aravis_sys::bindgen_test_layout_max_align_t
             at /home/bemeurer/src/standard/sensord/target/debug/build/aravis_sys-cc274a742a798c12/out/bindings.rs:85
   7: aravis_sys::__test::TESTS::{{closure}}
             at /home/bemeurer/src/standard/sensord/target/debug/build/aravis_sys-cc274a742a798c12/out/bindings.rs:85
   8: core::ops::function::FnOnce::call_once
             at /checkout/src/libcore/ops/function.rs:223
   9: <F as alloc::boxed::FnBox<A>>::call_box
             at libtest/lib.rs:1451
             at /checkout/src/libcore/ops/function.rs:223
             at /checkout/src/liballoc/boxed.rs:638
  10: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:105

Expected Results

For some reason I cannot generate bindings for max_align_t, the generated bindings are really weird, but only on Linux. On macOS, using the exact same code, bindgen generates the following

pub type max_align_t = f64;

Which passes tests and, as far as I can see, works just fine, although it seems to me like a long double should be represented by an unexisting f128 type?

On Linux, the generated bindings are such

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct max_align_t {
    pub __clang_max_align_nonce1: ::std::os::raw::c_longlong,
    pub __bindgen_padding_0: u64,
    pub __clang_max_align_nonce2: f64,
}

Now, not only is this a really weird way to represent the type, I don't understand why it mapped it into a struct, but it is also wrong, and fails tests as show above.

For what it's worth, I'm generating bindings for Aravis. Is there a workaround for now? Why is this happening in an OS-dependent way?

@emilio
Copy link
Contributor

emilio commented Jun 25, 2018

I suspect it's not picking up that struct definition but the builtin max_align_t?

In any case for now the most you can do is basically blacklist it. As long as you don't use it it should be fine.

@lovesegfault
Copy link
Author

Well, I found that max_align_t isn't really used anywhere, so I just made it opaque and called it a day.

Why would this behavior change between macOS and Linux?

@emilio
Copy link
Contributor

emilio commented Jun 26, 2018

Clang version installed, I'd say.

bors-servo pushed a commit that referenced this issue Sep 19, 2018
codegen: Generate u128 / i128 when available.

This is the first step to fix #1370 / #1338 / etc.

Fix for that will build up on this.
@emilio
Copy link
Contributor

emilio commented Feb 18, 2019

This is fixed with newer rust_targets, that support repr(align) or u128 / i128.

@emilio emilio closed this as completed Feb 18, 2019
@Coder-256
Copy link

Coder-256 commented Mar 3, 2019

@emilio I know this is closed, but I just realized... for a function void foo(long double *bar) bindgen outputs: fn foo(bar: *mut f64), but wouldn't that mean that the C code would cause an 8-byte stack overflow?! That is a huge vulnerability, and I'm sure that it would go unnoticed when importing large headers. I think this is something that people really ought to know about, there isn't even a warning about the size mismatch!

@emilio
Copy link
Contributor

emilio commented Mar 3, 2019

That's fair. I think we could do multiple things about it:

  • Panic when generating a long double without the appropriate size. Not a huge fan of this one, but not terribly hard.
  • Somehow propagate the error up the stack, and generate an opaque type.
  • Change default rust target to something that supports u128 / i128. This is probably the easiest and probably ok, these days, and we can always allow overriding it.

In any case that's probably worth a separate issue.

@Coder-256
Copy link

No matter what, you definitely need to notify users who currently might be using code that overflows ASAP, it's a matter of security! Then we can worry about fixing it.

@Coder-256
Copy link

Additionally, I would "yank" all versions affected by this bug and backport the fix if necessary

@Coder-256
Copy link

Also, please reopen this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants