Fix: Prevent crash on OOM in error copy

[schreter]

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]

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

[dtolnay]

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]

(For example, stuff like https://github.com/dtolnay/cxx/blob/1.0.91/src/cxx_vector.rs#L164 and https://github.com/dtolnay/cxx/blob/1.0.91/include/cxx.h#L333)

[schreter]

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

[dtolnay]

I am on board with handling OOM in this function without crashing the process. However I think the implementation given in this PR is not correct.

This change makes errorCopy return nullptr if it fails to allocate memory to copy the error message. That means cxxbridge1$error will return nullptr to Rust.

cxx/src/cxx.cc

Lines 458 to 460 in 03b0c8d

	const char *cxxbridge1$error(const char *ptr, std::size_t len) noexcept {
	return errorCopy(ptr, len);
	}

That means this Rust code will be holding a NonNull<u8> which is actually null, which is immediate language-level UB.

cxx/src/result.rs

Lines 45 to 50 in 03b0c8d

	extern "C" {
	#[link_name = "cxxbridge1$error"]
	fn error(ptr: *const u8, len: usize) -> NonNull<u8>;
	}
	let copy = unsafe { error(ptr, len) };

Since this is UB we cannot say what really happens, but naively it's likely the null ptr ends up as the err field of union Result, which makes the union look like it contains an ok not err.

cxx/src/result.rs

Lines 51 to 52 in 03b0c8d

	let err = PtrLen { ptr: copy, len };
	Result { err }

cxx/src/result.rs

Lines 13 to 24 in 03b0c8d

	#[repr(C)]
	#[derive(Copy, Clone)]
	pub struct PtrLen {
	pub ptr: NonNull<u8>,
	pub len: usize,
	}
	#[repr(C)]
	pub union Result {
	err: PtrLen,
	ok: *const u8, // null
	}

C++ will use the nullness of that pointer to determine whether an error has occurred in the Rust function, so it will appear that it has not.

cxx/gen/src/write.rs

Lines 1126 to 1128 in 03b0c8d

	writeln!(out, " if (error$.ptr) {{");
	writeln!(out, " throw ::rust::impl<::rust::Error>::error(error$);");
	writeln!(out, " }}");

The C++ side will continue as if a successful result has been written into the function's output location, which it has not. In reality the output location will be uninitialized. C++ will move or copy out of it which will be UB, and anything the caller does with it will be further UB, including heap corruption or other such issues depending on what data structure the return type is.