Should String::from_utf8_unchecked really be used?
CodesInChaos opened this issue · 4 comments
Can you really trust the data you're parsing? And is the performance impact big enough to go with this over the safer String::from_utf8?
@CodesInChaos it would make sense to switch to String::from_utf8, I'm a new maintainer of the library and currently in the way of learning rust, I would greatly appreciate some help to add tests and improve code for a pull request.
Your new code panics when it encounters an invalid string, which is at worst a denial-of-service vulnerability. This is certainly a big step up from the undefined behaviour exhibited by the old code, which in theory could result in a remote-code-execution vulnerability (though that's rather unlikely here). Since I come from a security point of view, your change fixes the issue I really care about.
In the long run, you should consider what level of error handling to expect when talking to a buggy or malicious device. Choosing to safely crash because you generally only talk to reasonably trusted devices on your own network is an acceptable choice (for example I'm often making such a choice when my web-application talks to the database server). Alternatively you could decide that such errors need to be handled gracefully, in which case you'd propagate it upwards using a Result<T, LifxError> where LifxError is an enum which could have an entry for ProtocolViolation.
(I only came across this project while looking for potentially unsafe use of from_utf8_unchecked, so I know very little about your project)
Yes, at this moment it's a "quick and dirty" change to avoid "unsafe" code. And as you said it's better to fail rather than to introduce a security issue. I will give a try to the LifxError enum.