Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

schreter
Copy link
Contributor

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).

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).
@schreter
Copy link
Contributor Author

Note: this subsumes the bugfix for invalid read #1179.

Copy link
Owner

@dtolnay dtolnay left a 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?

@dtolnay
Copy link
Owner

dtolnay commented Feb 26, 2023

@schreter
Copy link
Contributor Author

@dtolnay

Terminate on OOM matches the behavior of all other code in the repo, on both the Rust side and C++ side.

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).

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?

It's of course not optimal, it's just a small fix for this particular issue. In general, throwing in a noexcept method is a no-go in our project.

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 std::make_exception_ptr(). It is noexcept and also won't crash - it will rather give you an instance of std::bad_alloc inside of std::exception_ptr before crashing the program unnecessarily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants