Workaround for memory unsafety in third party DLLs #143090
Conversation
rustbot
added
S-waiting-on-review
Won't this cause extra copies and thus potentially hurt performance? |
|
Potentially, I guess. Though I'd expect filesystem APIs are almost certainly going to be much slower than a memcpy. As I mentioned in a comment, the fastest alternative would be to just ensure the empty string is null terminated. The assumption being that, however they're impersonating system APIs, they're at least being internally consistent. |
|
Shipping a workaround upstream for unsound software means that we are expecting all Rust programmers, not just Firefox, to pay for carrying the weight of unsound ecosystem software that they are not expected to interoperate with. Even if it's just a few bytes of code size we wouldn't need to pay otherwise, I don't think it makes sense. |
|
You cited us as shipping workarounds before, but the ones I remember were for Microsoft software. Where, yes, we may have done whatever the owners of the platform expected us to do, no matter how inane it may have seemed, because it doesn't matter if whatever is objectively incorrect if the local monarch will hang you(r process) for defiance. |
|
The one I'm thinking of was for alignment issues with a third party sandbox. This issue was fixed but we do still have the workaround under the assumption that other software may not do alignment properly. I guess it could be removed now but it also might break something. |
|
Ah. Yes, open source software that we can see whether it is patched or not still seems like a much different story. |
|
Can we set a deadline and make it public? |
|
Ok, I've changed it so that the impact will be minimal. The updated PR just tries to ensure null termination for our own strings and leave it up to the OS to use null termination for strings it owns (where "the OS" in this case includes people emulating OS APIs). |
|
This seems more tolerable. I have sometimes wondered if maybe |
| /// It is preferable to use `from_slice_with_nul` for our own strings as a | ||
| /// mitigation for #143078. | ||
| #[derive(Copy, Clone)] | ||
| pub struct UnicodeStrRef<'a> { |
There was a problem hiding this comment.
Could you mention in the docs something about this optionally being nul-terminated but that not being a guarantee? To help explain why the functions aren't unsafe.
| let mut path_str = c::UNICODE_STRING::from_ref(path); | ||
| let path_str = UnicodeStrRef::from_slice(path); | ||
| let mut object = c::OBJECT_ATTRIBUTES { | ||
| ObjectName: &mut path_str, | ||
| ObjectName: path_str.as_ptr(), |
There was a problem hiding this comment.
If I am understanding things correctly, this change only ensures nul-termination if path is empty but otherwise passes path unchanged? What is the benefit here?
There was a problem hiding this comment.
In the non-empty case the path is always given to us by the OS via directory iteration. So that falls under assuming OS returned strings are OK to use.
There was a problem hiding this comment.
Shouldn't it use from_slice_with_nul then? Or something like from_slice_with_nul_if_nonempty that still redirects the empty slice nul pointer but makes it clear that \0 is otherwise expected.
There was a problem hiding this comment.
Ah I see what you mean. The slice itself doesn't contain the nul (if any). There may or may not be a nul one beyond the end of the slice but we don't know. The point is we just assume there is if some third party is hooking OS functions and requires strings be null-terminated but otherwise we assume nothing outside of the slice.
We can't really do better than that without being more invasive. The APIs themselves say nothing about null termination.
|
I'm thinking maybe we should leave #143078 open once this merges, or make a new issue for eventually stripping this out. The software that will be relying on this seems objectively incorrect, and we have no way of knowing what calls actually need this; or really noticing if the I'm on board with @the8472's suggestion that we should document in that issue that we will be reverting this PR in ~6 months or so, so we don't wind up carrying a workaround forever. |
|
Based on the feedback I decided to simplify this down to a single I think this will make it easy for people to do the right thing by default. |
Resolves #143078
Note that we can't make any guarantees if third parties intercept OS functions and don't implement them according to the documentation. However, I think it's practical to attempt mitigations when issues are encountered in the wild and the mitigation itself isn't too invasive.