Invalid utf8 str could still be created by `from_utf8_unchecked`

[shinmao]

Bypass debug_assert!

inlinable_string/src/inline_string.rs

Lines 275 to 287 in f7c0a33

	impl InlineString {
	#[cfg_attr(feature = "nightly", allow(inline_always))]
	#[inline(always)]
	fn assert_sanity(&self) {
	debug_assert!(
	self.length as usize <= INLINE_STRING_CAPACITY,
	"inlinable_string: internal error: length greater than capacity"
	);
	debug_assert!(
	str::from_utf8(&self.bytes[0..self.length as usize]).is_ok(),
	"inlinable_string: internal error: contents are not valid UTF-8!"
	);
	}

We consider that the protection of assert_sanity is not sufficient to check the validity of string due to removed debug_assert! in release mode.

PoC

fn main() {
    let mut s = InlineString::from("A");

    {
        let bytes: &mut [u8] = s.as_mut();
        bytes[0] = 0xFF; // invalid UTF-8 byte
    }

    // attack vector 1: AsRef<str>::as_ref -> from_utf8_unchecked
    let _a: &str = s.as_ref();

    // attack vector 2: Index<RangeFull> -> from_utf8_unchecked
    let _b: &str = &s[..];

    // attack vector 3: Deref -> from_utf8_unchecked
    let _c: &str = &*s;
}

With cargo run in nightly version

thread 'main' panicked at /home/usr/.cargo/registry/src/index.crates.io-6f17d22bba15001f/inlinable_string-0.1.15/src/inline_string.rs:283:9:
inlinable_string: internal error: contents are not valid UTF-8!

However, if run with cargo run --release, no error message will show. Maybe consider changing it to assert! instead? Or change it to str::from_utf8 to do error handling could be more straightforward.

[ArtemGr]

Hello there and Merry Christmas!
TBH, I am not comfortable with InlinableString checking my strings on every access. AFAIK, this has no precedence in standard Rust String, and InlinableString is supposed to make certain string operations faster, not slower.
We could make a feature to enable such checks (and thus remove them from default no-feature code path, woot!), but I don't think it should be a default. In some from_utf8 method - sure, but not on every access! The latter is insane!
So.. in expecting a UTF-8 check on every access, where are you coming from? Tell me more, if you would.

p.s. The way I read the "inlinable_string: internal error: contents are not valid UTF-8!" error is that it is a smoke check on whether the InlinableString library itself is okay: if we messed up, we might discover this from the non-UTF-8.
But in production the effect might be opposite: we might surprise a user by failing where we should not fail, resulting, among other things, in DoS and security bugs.
Conversely, given that InlineString is now stable, we should remove the costly check.

[shinmao]

@ArtemGr merry christmas!! Thanks for the detailed explanation! I completely agree with your perspective on performance. Since the primary goal of inlinable_string is to provide a fast, stack-allocated alternative, adding heavy runtime UTF-8 checks on every access would indeed be counterproductive and inconsistent with Rust's "zero-cost abstraction" philosophy.

However, my concern remains rooted in Rust's safety guarantees. I believe we can address this bug without sacrificing performance by shifting the focus from runtime checks to compile-time API design.

Specifically, I’d like to propose restricting the as_mut() implementation:

• Follow the Standard Library Pattern: In standard Rust, String does not implement AsMut<[u8]>. This is intentional because providing a direct mutable byte slice allows users to accidentally bypass UTF-8 integrity. To modify bytes directly, String users must use explicit methods like as_mut_vec(), which signals that the user is taking responsibility for safety.
• Remove or Deprecate AsMut<[u8]>: By removing this trait or replacing it with a more explicit method, we can prevent the "attack vectors" I mentioned in the PoC at the type level. This ensures that the internal buffer remains valid UTF-8 without needing a single runtime check during string access.

What do you think about aligning the API more closely with std::string::String by removing the blanket AsMut<[u8]>? This would solve the safety issue at compile-time while keeping the production performance as lean as possible.

I'd love to hear your thoughts on this approach! Hope you enjoy your day!

[ArtemGr]

Sounds good. When time allows, I plan to experiment with making as_mut optional.

[shinmao]

@ArtemGr Sure. Let me know anytime if you need discussion! Do you agree with me to file the rustsec advisory for this issue? It won't break the normal functionality of the usage, instead, it can remind any users in the future to avoid the using function in this (PoC) way (if running cargo audit).

[ArtemGr]

I've added #38.
@fitzgen 's approval is required in order to merge.