NonZero constructor in debug-builds should assert input is actually nonzero.

[pnkfelix]

NonZero constructor in debug-builds should assert input is actually nonzero.

In particular, I am a bit concerned by the somewhat free-wheeling way that we are adding uses of the unsafe NonZero::new constructor function without any checks that the given input actually is non-zero.

I freely admit that I do not have many examples of current bugs that such a check would have prevented. However, I can point to some questionable code in a previous iteration of the libstd btree code that such a check would have at least caught: prior to PR #30426, the btree node.rs code had this line:

rust/src/libcollections/btree/node.rs

Line 305 in 26a2f85

self.keys = unsafe { Unique::new(ptr::null_mut()) };

Was this a bug? Well, ... according to the invariants of Unique and NonZero, the keys.is_null() check should in principle be optimizable to be "always false", right?

• The assignment occurs at the end of a drop method implementation, but the struct in question was marked #[unsafe_no_drop_flag], and so that drop code can (and will) be run repeatedly, so that keys.is_null() call can well occur in a context where the data is in a "bad state"
• (indeed, the whole point of the keys.is_null() check is to catch the null being used as a sentinel value, even though it is in fact illegal in that context.)

[pnkfelix]

To actually put in this check, the language would need to be extended to some degree, since NonZero::new is marked as a const fn, and thus does not have an easy way to embed a debug_assert! statement before the trailing expression.

(I have discussion here rust-lang/rfcs#1383 of potentially trying to change/extend const evaluation to support debug_assert! so that it could be used with const-evaluatable expressions, and cause compilation failure when the assert fails.)

[pnkfelix]

another necessary extension to the language implementation would probably be #28809 (see why in #28809 (comment) )

[pnkfelix]

cc #27730

[pnkfelix]

In case its not clear, I strongly encourage the libs team to not stabilize NonZero until this is addressed in some way.

[pnkfelix]

(and see also #27573 )

[eddyb]

NonZero::new should return Option<Self> and we could maybe provide a new_unchecked method, if really needed.

[Gankra]

new_unchecked is literally the only thing a consumer would ever use. You're asserting the value is statically NonZero in low-level unsafe code.

Note that adding the debug assert is an exception-safety concern -- though it will only trigger if you were about to do UB, so it's ok I guess?

[eddyb]

@gankro How do you know the value is non-zero to begin with?

[apasel422]

@eddyb One place this could be used where the value is known to be non-zero is with HashMap's SafeHash.

[Gankra]

@eddyb usually because we're already checking -- e.g. it came out of an allocator and we didn't abort. Unique is pretty universally used for "we're Box but not".

The problem code exists because someone was using null as a sentinel instead of heap::EMPTY for whatever reason. Probably because of the old zeroing drop?

[eddyb]

@gankro There's your answer. If it's known to be not NULL, then the allocator should return NonZero<*mut u8> or even Unique.
The allocator is what uses NonZero::new and matches on the result.

[Gankra]

Yep, that'd be great. 's in the allocator API RFC to some extent already.

[steveklabnik]

Triage: NonZero has been replaced with many types, like NonZeroU8. These have both checked and unsafe unchecked constructors. Closing!