Fix UTF-8 unsoundness in string::merge

[mzabaluev]

Fixes #193

[mzabaluev]

Note that the merge implementation does not clear the existing value, as the protobuf spec suggests it should. If it did, the guard code would be simpler and marginally more efficient.

[nrc]

This looks good, thanks! I would be tempted to add a safe wrapper function which takes a closure to operate on the Vec<u8>, then commits so that the unsafe code is encapsulated.

[nrc]

I think you need to check the whole string, not just the new bytes here, since when the user has mutable access to the internal string, they can modify as well as append.

[mzabaluev]

In a general purpose StringGuard I would do, but here we can use the knowledge that bytes::merge would not change the already filled part of the vector, and nothing else can modify it while it is borrowed.

But again, clearing the string value before the new wire data are merged, and also on StringGuard drop without commit, would be simpler and not rely on the other function's behavior to be safe. I just don't understand why this implementation concatenates non-repeating fields of types string and bytes.

[mzabaluev]

clearing the string value before the new wire data are merged... would be simpler

Also in the BytesString (or StrChunk) rework in #190, I would just suck the data into Bytes and convert that into the new string value with a safe UTF-8 content check rather than fiddle with unsafe mutable representations.

[danburkert]

I just don't understand why this implementation concatenates non-repeating fields of types string and bytes.

It's entirely possible that this is done by accident, and should be fixed. Do you have a reference for where the spec / PB docs mention that merging will overwrite strings?

[mzabaluev]

In the developer guide on encoding:

Normally, an encoded message would never have more than one instance of a non-repeated field. However, parsers are expected to handle the case in which they do. For numeric types and strings, if the same field appears multiple times, the parser accepts the last value it sees.

[danburkert]

From my (admittedly weak) understanding of the unsafe rules, it's instant UB to create a str/String containing invalid UTF-8. Given that, I think this pattern is still unsafe, since it's appending bytes to a String, then checking validity. Given my understanding, I think the only safe way to do what we are trying to do here is accumulate into a Vec<u8>, check if it's valid utf-8, and then and only then convert to String.

[mzabaluev]

As far as I understand, it's not UB until the invalid data are accessed as str/String; but, theoretically, .as_bytes() or even .as_mut_vec() might be in violation of this.

[danburkert]

I think using some clever buffer swaps we can simplify this and remove the need for unsafe entirely, here's what I'm thinking:

    pub fn merge<B>(wire_type: WireType, value: &mut String, buf: &mut B) -> Result<(), DecodeError>
    where
        B: Buf,
    {
        let mut value_bytes = mem::replace(value, String::new()).into_bytes();
        bytes::merge(wire_type, &mut value_bytes, buf)?;
        *value = String::from_utf8(value_bytes)
            .map_err(|_| DecodeError::new("invalid string value: data is not UTF-8 encoded"))?;
        Ok(())
    }

That can even be simplified a bit more if we decide we need to overwrite on merge instead of append. WDYT?

[mzabaluev]

@danburkert I think your implementation is good, unless somebody cares about the existing string data being lost on merge failure.

[danburkert]

Thanks again @mzabaluev, this is a great catch!

[Shnatsel]

This RP seems to fix unsoundness that can be triggered in practice. Please file a security advisory at https://github.com/RustSec/advisory-db so that dependents can check if their crate is affected and upgrade.