rust-osdev/ucs2-rs

UCS-2 conversion utilities for Rust.

ucs2-rs's Issues

0.1.1 should be a major release?

Since the Error trait changed, I think 0.1.1 should be yanked and re-released as 0.2?

Future of This Crate

From official documentation/comments it appears that ucs2 is a base for String handling in the uefi crate. However, there are exactly two usages in the output protocol implementation (output.rs).

The uefi-crate provides Char16 CString16 and CStr16 to work with UCS-2/UEFI strings, however.

I have the the following variants in mind:

• A: We should move Char16 CString16 and CStr16 from the uefi crate to this crate
• B: drop this crate

I think A would be a good solution.

PS: Am I missing something? Is there a specific reason for the status quo?

Ping @nicholasbishop @GabrielMajeri

Technically possibly reachable panic(s)

The conditions for returning Err(Error::BufferUnderflow), which are responsible to the panics getting in compiled are arguably unreachable to the point that core::hint::unreachable_unchecked should be used instead of return Err(Error::BufferUnderflow), however, under the assumption that they are somehow reachable...

https://github.com/GabrielMajeri/ucs2-rs/blob/master/src/lib.rs#L77-L79

If len == i + 1 this branch won't be taken, hence llvm inserting a panic for the i + 1 array access.

https://github.com/GabrielMajeri/ucs2-rs/blob/master/src/lib.rs#L63-L66

LLVM inserts a panic for the i + 1 array access due to the use of == instead of >=, not sure why, but it does, presumably llvm can't quite prove that that is okay. (which it definitely is)

convenient link to compiler explorer with before/after removing the first mentioned panic: https://rust.godbolt.org/z/25yb-K

I once again believe these panics to be absolutely unreachable, along with the code paths for return Err(Error::BufferUnderflow) and even return Err(Error::InvalidData), because it is UB to have a &str that isn't valid UTF-8, also see playground for a reference that Rust considers a string with an incomplete character "invalid"

Null terminated conversion

When using the decode method, I'm passing a UCS-2 encoded buffer that contains a 0x0000 (u16) indicating the end of the string. The buffer is fixed size, but whatever happens after the 0x0000 is undefined, and is normally 0xFFFF. This results in the 0xFFFF getting decoded into a 3 bytes character, which eventually leads to an overflow in the output buffer (output buffer is an u8 array of twice the length of the input. For context, this scenario happens when reading the directory entries in a FAT16 filesystem.

To mitigate this I've had to write code to check if there's a null byte in the input buffer and if so, pass to decode only the slice that doesn't contain it. I'm not entirely unhappy with this solution but it took me many many hours of debugging and eventually stepping into the library code to find out what the problem was, so I thought it'd be nice for the library to either take this into account or provide another method that does. Something like decode_to_end_of_string or something like that. The result count of bytes would be the numbers of bytes converted till the null byte. Actually, I would do this the default and have the option of go past the null bites to decode the buffer in full, but I don't know enough about UCS-2 or UTF-8 to know which one should be the default behaviour.

I'm happy to write a pull request moving the code I wrote and a test if that helps.

Thanks for your crate!

Decoding support

Currently, we only have support for UTF-8 to UCS-2 conversions. We should add the appropiate decode / decode_with functions for the reverse (UCS-2 to UTF-8) conversions.