Should slice::from_raw_parts have a debug assertion against null pointers?

[Paul-E]

Background: I was dealing with a bug in the project citybound, there was an bug where the program would crash in release mode, but was fine in debug mode. It took a bit of work getting things down to a minimal example, but eventually we found the issue. In the end it turned out that a null pointer was being passed to std::slice::from_raw_parts, which is against that functions contract, even if the given length is 0. My understanding is that never passing null enables some optimization. I found this to be a subtle bug for someone who is new to the citybound project. Consider that the person who wrote the code calling from_raw_parts (and read its docs) might not be the same person debugging that part of the code, and it isn’t obvious from the function name or signature that passing null pointers along with 0 length is UB.

Although at the moment it is UB to pass null into this function, it would more helpful for debug builds to assert that p is not null. This way it would emit a useful error message. I understand that half UB might be undesirable, in which case might might be useful instead to have a tool that detects UB, even if it is at runtime (al la UBSan).

The citybound bug in question is here, but the more relevant discussion starts here.

[scottmcm]

My recollection is that there are a bunch of places where std would like to add additional checks like this, but that currently the same std library is used for both debug and release, and thus debug_assert is never there in stable (only if you enable them in config.toml in a custom build).

[sfackler]

We have the ability to flag certain functions as being compiled under the user’s overflow checking state. We use it for things like Add::add: https://github.com/rust-lang/rust/blob/45fba43b3d5b4d1944268cf973099bfacb11bf4c/src/libcore/ops/arith.rs#L107. We could probably do something similar for debug_assert.

[rkruppe]

rustc_inherit_overflow_checks is a pretty big hack and work by hooking directly into the compiler’s decision whether to generate overflow checks (see https://github.com/rust-lang/rust/blob/5f7aeaf6e2b90e247a2d194d7bc0b642b287fc16/src/librustc_mir/hair/cx/mod.rs#L82). Since debug_asserts are enabled or disabled via the cfg system, an equivalent feature for debug assertions would require an even bigger hack in the cfg system.

[oli-obk]

debug assertions generally are meant to reduce the final binary’s amount of machine code (and thus improve the performance). We could stop using cfg to achieve this feature and instead rely on const propagation and a “magical” MIR constant that can be flipped without requiring a recompilation of the function. Compiling the MIR to machine code would then eliminate the branch depending on the value of the flippable constant.

[kornel]

Maybe it’s time to have two builds of stdlib anyway?

stdlib’s debug info adds 2MB of overhead to Linux binaries even in release mode, which is another reason to separate versions.

[sfackler]

The decision to include or omit debuginfo is totally orthogonal to the decision to include or omit debug assertions.

[system]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.