rust-lexical icon indicating copy to clipboard operation
rust-lexical copied to clipboard

[OTHER] Improve internal safety comments and architecture

Open Manishearth opened this issue 11 months ago • 0 comments

I was reviewing the lexical-write-integer crate and I discovered a bunch of things that could be improved wrt safety.

The main issue was the one already filed as https://github.com/Alexhuszagh/rust-lexical/issues/95. That's actual UB, this issue is more about the safety comments and how the invariants are upheld. The safe mode doesn't disable this code that has actual UB, either.

Besides the issue that already exists, there are a whole bunch of functions (e.g. write_digits()) that unsafely index from table are documented with "This is safe as long as the buffer is large enough to hold T::MAX digits in radix N.",but they do not include the safety requirement on table`.

Furthermore, some functions (also write_digits()) take a mut index: usize argument that is used to directly index. These safety requirements are also not mentioned.

Besides safety comments, I found a couple areas where it was hard to review the unsafe code:

A bunch of code relies on the radix being from amongst a valid set (e.g. get_table()). May benefit from being an enum; it's hard to tell if this invariant is being upheld.

Other code which is calling the write_*() functions (e.g. unsigned()) declare they are safe as long as the buffer can hold FORMATTED_SIZE elements, but that invariant is not really clear from the functions used, it's kinda hidden away.

Might be cool to have a ValidBuffer<FORMAT> wrapper that allows calling these write functions in an encapsulated way.

Anyway, hope this helps. I don't have time to devote to doing this work myself but I figured I'd share my notes in case someone else does.

Manishearth avatar Jul 10 '23 14:07 Manishearth