-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Prevent crash on OOM in error copy #1183
base: master
Are you sure you want to change the base?
Conversation
C++ error message returned by `what` must be NUL-terminated. However, the current copy function only copied the characters, but didn't add the NUL. Allocate one more byte and set it to NUL.
When the error message is copied, the allocation may fail. This would terminate the process, since it's called from a `noexcept` function. Make the allocation `nothrow` and handle `nullptr` in `what()` method appropriately, returning a generic message saying that the message cannot be allocated. This also prevents a crash if `what()` is called on a moved object (which is technically UB, but still better not to crash).
Note: this subsumes the bugfix for invalid read #1179. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Terminate on OOM matches the behavior of all other code in the repo, on both the Rust side and C++ side. Is this PR at all useful to you without a whole bunch of other refactoring of all the other usages of operator new and liballoc?
That is our problem that currently, there is no clean interface how to prevent crashes. The issue is, we are integrating our Rust code in a C++ process which runs potentially a multi-terabyte process, which simply can't just fall down. Therefore, yes, we have have an issue that we have to carefully handle OOM situations and map OOM in general to a panic/exception. Fortunately, almost all Rust library code is already prepared for that, so it's not a big deal panicking/throwing an exception in the allocation (and yes, we know that it's not yet fully implemented, but we are here on the bleeding edge).
It's of course not optimal, it's just a small fix for this particular issue. In general, throwing in a Containers need to be fixed, too, but we currently don't use them yet. So there was no investment there yet. In fact, we use custom exception handlers anyway (I didn't push the change yet to upstream, though, since it needs some polishing), so we are not affected much by not having this in, I just pushed it as a warm-up for completeness (whenever I see a bug/potential issue I try to fix it right away or at least note it down for later). IMHO, for this particular issue, it is definitely correct way to go. A comparabe example would be |
When the error message is copied, the allocation may fail. This would terminate the process, since it's called from a
noexcept
function.Make the allocation
nothrow
and handlenullptr
inwhat()
method appropriately, returning a generic message saying that the message cannot be allocated.This also prevents a crash if
what()
is called on a moved object (which is technically UB, but still better not to crash).