Incorrect size for long double when using old rust_target

[Coder-256]

Long story short, long doubles are mistreated, leading to a SEVERE security/stability issue. I don't think I am overreacting. On some platforms (for example, my own computer), long doubles are 16 bytes! Not 8! This can cause a stack/heap overflow and all sorts of issues. This is enough to trigger the bug!:

foo.h:

void mul_assign_long_double(long double *input, double multiplier);

foo.c:

void mul_assign_long_double(long double *input, double multiplier) {
    *input *= multiplier;
}

test.rs:

#[derive(Debug)]
#[repr(C)]
struct StackOverflowStruct {
    a: f64, // Target of our C function
    b: u64,
    c: u64
}

unsafe fn to_bytes(input: &StackOverflowStruct) -> std::string::String {
    let x = std::mem::transmute_copy::<StackOverflowStruct, [u64; 3]>(input);
    format!("[{:#X},{:#X},{:#X}]", x[0], x[1], x[2])
}

fn stack_overflow() {
    unsafe {
        let mut test_struct = StackOverflowStruct {
            a: std::mem::transmute::<u64, f64>(9367522409571165184), // constructed
            b: 16391, // constructed
            c: 12345 // random
        };

        println!("BEFORE SO: {:?}", test_struct);
        println!("BEFORE SO (bytes): {}", to_bytes(&test_struct));

        mul_assign_long_double(&mut test_struct.a as *mut _, 89237843.0); // random

        println!("AFTER SO: {:?}", test_struct);
        println!("AFTER SO (bytes): {}", to_bytes(&test_struct));
    }
}

Output:

BEFORE SO: StackOverflowStruct { a: -0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004815640556295841, b: 16391, c: 12345 }
BEFORE SO (bytes): [0x8200200010002400,0x4007,0x3039]
AFTER SO: StackOverflowStruct { a: -0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000014459992918754277, b: 16417, c: 12345 }
AFTER SO (bytes): [0xACDE2996DFDED52B,0x4021,0x3039]

Notice that b has been affected! (the middle value of the array). It changed from 16391 to 16417. This is because although we only pass a pointer to a, it is interpreted as a 16-byte pointer, but a only has enough space for 8 bytes so it overwrites into 2 bytes of b! Sorry for all the exclamation points, but this is a serious issue!

This is a very very bad thing.

In addition to fixing this issue, this must be treated as a severe vulnerability. Personally I would:

• "Yank" all unfixed versions
• Notify users ASAP, anybody using rust-bindgen with mission-critical code should stop anything built with rust-bindgen until they get a fixed version and update their code
• Provide a fixed version, and possibly backport the fix to old versions

It would be very easy to import a large C header/library, see a f64 where it would make sense, and assume everything is OK. And all tests might pass. But then when you run your code in production, the stack is corrupted! Not only could this happen randomly with well-behaved code, but it could also be exploited to overwrite up to 2 bytes (long double is usually only 10 important bytes; 10 - 8 = 2) on the stack with attacker-controlled values. Plus it would be very hard to debug. That is why I am treating this so seriously! In issues like #1338 and #550, it has been brushed off and treated as a small bug because there is no easy fix, but currently, it is not just a bug, it is actually unsafe/undefined behavior, and AFAIK there is no warning about it at all.

Since Rust does not have a f128, there is no clear replacement, but that debate can be held later (u128? [u64; 2]?). Right now, I think the main concern should be at the very least notifying people using rust-bindgen about this issue.

[Coder-256]

Related issue: #317

I did some digging, and now I see a host of other, also potentially severe, issues that have been IMHO mishandled: #465 #743 #867 #1194. All these issues silently output incorrect code. Considering that rust-bindgen is maintained on the official rust-lang GitHub organization, yet issues like these are not even mentioned to users (again, I may have missed warnings) and the code is treated as stable, this is making me seriously reconsider using Rust at all for anything important or secure, because if major security issues are mishandled and ignored and never even kept track of (unlike basically every major programming language), then there is no guarantee that things will truly be safe (which isn't that the whole point of Rust over C++?)

[emilio]

All these issues silently output incorrect code.

That is just not true. Bindgen outputs layout tests that detect these issues. In the case of a single long double used as an argument to the function we don't emit anything. The rest of the issues mentioned there do cause failing unit tests in the final crate.

[Coder-256]

Sorry you might be right about the unit tests, that's my mistake. Still this issue is not fixed and really needs to be addressed, this one does not trigger any test failure for me!

Running bindgen with no arguments on this file:

void mul_assign_long_double(long double *input, double multiplier);

Gives this output (which passes unit tests):

/* automatically generated by rust-bindgen */

extern "C" { # [ link_name = "\u{1}_mul_assign_long_double" ] pub fn mul_assign_long_double ( input : * mut f64 , multiplier : f64 ) ; }

The * mut f64 is the issue, sorry if that was not clear.

[emilio]

Yes, I understand the issue, but to be clear, for context:

• This was fixed long ago (fix is present from c4433e0, which was released in september 30, 2018).
• Before it being fixed, there is literally no way to generate both the correct layout and ABI in rust (there was no support for neither i128 / u128, nor #[repr(transparent)] / #[repr(align)].
• As soon as long double is used in a struct, the problem makes unit tests fail.

In addition to fixing this issue, this must be treated as a severe vulnerability. Personally I would:
"Yank" all unfixed versions
Notify users ASAP, anybody using rust-bindgen with mission-critical code should stop anything built with rust-bindgen until they get a fixed version and update their code
Provide a fixed version, and possibly backport the fix to old versions

I personally disagree. If that was the procedure, all compiler versions that ever miscompiled something should be yanked. Rust (and clang, and gcc, and every other compiler, for that matter) has miscompilation bugs.

As I said in the other issue, I agree its maybe a bit unfortunate that the default configuration is rust_target = 1.0. As I said, you cannot make something that compiles with Rust 1.0 and has the right layout in this case. That being said, probably updating the default rust target version is overdue.

Still this issue is not fixed and really needs to be addressed, this one does not trigger any test failure for me!

Please read the other issue. This is fixed if you pass a rust-target that allows bindgen to generate u128, like: --rust-target 1.26.

[Coder-256]

Ok I see that it has been fixed when you pass a higher rust-target. Still, under no circumstances should bindgen produce corrupted code; especially when using the default arguments. Additionally, I don't see any reason why [u64; 2] would not work, but the point is that under no circumstances should you silently get an incorrect output.

[Coder-256]

And there is no indication anywhere that you need to do anything other than use bindgen::Builder::default() to get the proper Rust target, in fact the documentation says that it automatically should be the latest version, but under 1.33 stable and using a build script adapted directly from the tutorial, the bindings are still wrong.

[emilio]

Additionally, I don't see any reason why [u64; 2] would not work.

That has the wrong alignment. Anyhow, #1530.

[Coder-256]

Ok that should fix it for most people, but still it's important that 1. despite the rust target, the proper code (or an error message) is produced, and 2. people are aware of this issue in more than just release notes (I'm not sure what the best way to do this is, but just to make sure that people who are currently using old versions of bindgen are aware). I understand if you don't want to remove older versions.

[Coder-256]

I apologize if I made too big a deal of this. The main reason for my concern is that this issue slips through unit tests and before just now, it was the default behavior for both command-line and build script usage.

[emilio]

No worries. I'll just note that I maintain bindgen on my free time (I don't get paid at all to do this), and that overly aggressive comments is not the most pleasant thing to deal with on a Sunday evening. Anyhow, I appreciate the report, this was overdue in any case.

[Coder-256]

I'm sorry if I came off as aggressive, and I realize that I should not have unrealistic expectations for a community-maintained project, I'm not paid either after all 😂