mem::uninitialized backport

[Noratrieb]

Before 1.0.0, itoa used std::mem::uninitialized to use uninitialized u8s. This is now regarded as being undefined behaviour, and itoa correctly switched to MaybeUninit instead some time ago. But this switch came with a new major version, meaning crates that still depend on old itoa (like the current published version of csv) do not get that soundness fix.

See: rust-lang/rust#66151 for more info around mem::uninitialized.

Therefore, I would suggest backporting a soundness fix for 0.4.x. We can't use MaybeUninit, since that would increase the MSRV which is probably not desired. I would suggest to zero-initialize it instead. While this can come at a minor perf impact, it's for an old and outdated version, and most people will likely have upgrade their dependencies already. This would also be a really simple change.

What's your opinion on this? Is this something we should do right now? Maybe at a later point in the future?

[5225225]

Also, for testing purposes, there's the RUSTFLAGS="-Zstrict-init-checks" flag, which makes mem::uninitialized and mem::zeroed check through arrays, and mem::uninitialized of uninit primitives panic.

[Noratrieb]

I don't have benchmarks, but if you're interested I can run some.

[5225225]

I scanned through the list of crates that depend on itoa and there's only a few that have a non-0.4 version number, so there's no real need to backport to 0.3 or lower.

(Those crates being: https://crates.io/crates/pretty_toa, https://crates.io/crates/rust-version . There's also https://crates.io/crates/parallel but that's entirely yanked so can just be ignored)

[5225225]

Running the benches that exist in the repo on commit de247d6 after making Buffer::new use [0_u8; I128_MAX_LEN]:

(I deleted the std fmt bench lines because they aren't important here)

test bench_itoa_fmt::bench_i16_0      ... bench:           6 ns/iter (+/- 0)
test bench_itoa_fmt::bench_i16_min    ... bench:          10 ns/iter (+/- 0)
test bench_itoa_fmt::bench_u64_0      ... bench:           4 ns/iter (+/- 0)
test bench_itoa_fmt::bench_u64_half   ... bench:          11 ns/iter (+/- 1)
test bench_itoa_fmt::bench_u64_max    ... bench:          17 ns/iter (+/- 1)
test bench_itoa_write::bench_i16_0    ... bench:           5 ns/iter (+/- 0)
test bench_itoa_write::bench_i16_min  ... bench:          12 ns/iter (+/- 0)
test bench_itoa_write::bench_u64_0    ... bench:           4 ns/iter (+/- 0)
test bench_itoa_write::bench_u64_half ... bench:          11 ns/iter (+/- 0)
test bench_itoa_write::bench_u64_max  ... bench:          16 ns/iter (+/- 0)

And with mem::uninit, how it is now.

test bench_itoa_fmt::bench_i16_0      ... bench:           4 ns/iter (+/- 0)
test bench_itoa_fmt::bench_i16_min    ... bench:           9 ns/iter (+/- 0)
test bench_itoa_fmt::bench_u64_0      ... bench:           4 ns/iter (+/- 0)
test bench_itoa_fmt::bench_u64_half   ... bench:          10 ns/iter (+/- 0)
test bench_itoa_fmt::bench_u64_max    ... bench:          16 ns/iter (+/- 0)
test bench_itoa_write::bench_i16_0    ... bench:           5 ns/iter (+/- 0)
test bench_itoa_write::bench_i16_min  ... bench:          10 ns/iter (+/- 0)
test bench_itoa_write::bench_u64_0    ... bench:           5 ns/iter (+/- 0)
test bench_itoa_write::bench_u64_half ... bench:          10 ns/iter (+/- 0)
test bench_itoa_write::bench_u64_max  ... bench:          16 ns/iter (+/- 0)

Looks marginal. Some increase, but it didn't utterly kill performance or anything. Though this is being ran on my desktop just running cargo bench, so not exactly the most scientific measurements.

[5225225]

Also, to be clear, this isn't urgent or anything, making mem::uninit panic for uninit u8s is likely to happen eventually, but is probably a fair way off because so many crates did it.

It's entirely possible that by the time we are able to do that, all the crates would have updated by then, but I think that's unlikely.

[saethlin]

If history is anything to go by, code like this will be made UB by the addition of LLVM attributes before a PR which prevents that with panics is approved. So that is what I would be more concerned with.

[Noratrieb]

(rust-lang/rust#99182)[https://github.com/rust-lang/rust/pull/99182] has now landed, removing all possible future LLVM UB from the way itoa uses mem::uninit. This greatly reduces the risk of using old itoa in practice, and therefore makes a backport less important. A backport would still improve the experience of people using crates with old itoa under Miri on MSAN, but I don't think it's worth it, therefore I'll close the issue.

[Noratrieb]

Reopening this since soon there will be FCWs emitted for mem::uninitialized.

[dtolnay]

People getting a FCW for using an ancient version of the library sounds like a good outcome to me.

[RalfJung]

There is some reluctance to adding an FCW that would show up for a large fraction of the ecosystem, with not even cargo update enough to fix the warning. So a semver-compatible update for itoa would still be very useful. itoa 0.x is still used a lot.

[dtolnay]

I will not be doing a backport. The code in 0.4 exactly follows best practice at the time that 0.4 was released. ManuallyDrop did not exist and the standard library documentation literally showed mem::uninitialized::<[Vec<u32>; 1000]>() as an example of correct use.

If modern-day compiler optimization requires it, you can make the function 0x1-init and then remove it in a future edition. Neither of those things warrant a FCW for users of itoa 0.4 as it will not affect them.

[RalfJung]

The code in 0.4 exactly follows best practice at the time that 0.4 was released. ManuallyDrop did not exist and the standard library documentation literally showed mem::uninitialized::<[Vec; 1000]>() as an example of correct use.

Yes, nobody is denying that.

But we're trying to fix that mess, and there are some people (including me) advocating for a strategy that aims to remove the function in future editions entirely, and there are other people that say that path only makes sense if we do an FCW in all editions.

you can make the function 0x1-init

Latest nightly already does that.

[dtolnay]

there are some people (including me) advocating for a strategy that aims to remove the function in future editions entirely

I am on board with this.

and there are other people that say that path only makes sense if we do an FCW in all editions.

My not backporting is a vote saying my strong belief that those people are misguided.

[RalfJung]

My not backporting is a vote saying my strong belief that those people are misguided.

Yeah, I can tell. ;) It's more of a veto though, in effect. So looks like either we convince t-libs to do an old-edition-only removal, or we're just stuck with having mem::uninitialized around forever (or at least for a long time)... :/