mozilla / cbindgen

A project for generating C bindings from Rust code
Mozilla Public License 2.0
2.37k stars 306 forks source link

Code generated from u64 bitflags may have undefined behavior #849

Closed nical closed 5 months ago

nical commented 1 year ago

code like this:

bitflags::bitflags! {
    pub struct Bits: u64 {
       const FOO = 1 << 40;
    }
}

produces some C++ that look like:

#define SomethingBits_FOO (uint64_t)(1 << 40)

Clang complains that the shift overflows.

Adapter.cpp:176:25: warning: shift count >= width of type [-Wshift-count-overflow]

And the code crashes.

My understanding is that the produced C++ code shifts a 32bit integer and then cast the result to uint64_t.

nical commented 1 year ago

Looks like this generalizes to constant expressions.

I'm new to cbindgen's code but it looks like a fix for this would have to go somewhere around here: https://github.com/mozilla/cbindgen/blob/0b43f0bc6ce8aad77a5d1417657daddd32905853/src/bindgen/ir/constant.rs#LL342C33-L342C33

Unfortunately since this is based syn and not on something that has rust types resolved, it may not be straightforward. Maybe something that propagates the expected output type up the expression to insert casts or suffixes.

jschwe commented 1 year ago

To me it looks like for binary expressions like x << y (with constants), the x and y are just literally pasted (unless they have explicit type annotations): https://github.com/mozilla/cbindgen/blob/0b43f0bc6ce8aad77a5d1417657daddd32905853/src/bindgen/ir/constant.rs#LL327C20-L327C34

cbindgen wouldneed to add some operator dependent logic, since e.g. 1 always fits into an integer, it only becomes a problem because of the left shift operator requiring a larger type on the left hand side. The better solution would be to use the implicit rust type (the type of the bitflags struct), but I'm not sure how easy or hard it would be to pass down that information to the load function.

emilio commented 5 months ago

@nical what cbindgen version were you using?

764 is related here, and the test is basically your reduced test-case, so I think this should work now?

nical commented 5 months ago

I don't know off hands but it indeed looks like the exact same thing so I'll close this. Thanks @emilio !