Add NUL character at the end of copied error message

[schreter]

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.

[schreter]

BTW, here is the issue fixed by this (from Valgrind):

==21911== Thread 2 test_c_call_r:
==21911== Invalid read of size 1
==21911==    at 0x4E0C934: strcmp (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21911==    by 0x18FE27: cxx_run_test (tests.cc:812)
==21911==    by 0x13781C: test::test_c_call_r::cxx_run_test (test.rs:240)
==21911==    by 0x1377E1: test::test_c_call_r (test.rs:246)
==21911==    by 0x13CD56: test::test_c_call_r::{{closure}} (test.rs:235)
==21911==    by 0x127364: core::ops::function::FnOnce::call_once (function.rs:507)
==21911==    by 0x171B9E: test::__rust_begin_short_backtrace (function.rs:507)
==21911==    by 0x14326B: core::ops::function::FnOnce::call_once{{vtable.shim}} (lib.rs:648)
==21911==    by 0x170B29: test::run_test::run_test_inner::{{closure}} (boxed.rs:2000)
==21911==    by 0x13DB60: std::sys_common::backtrace::__rust_begin_short_backtrace (lib.rs:601)
==21911==    by 0x14349A: core::ops::function::FnOnce::call_once{{vtable.shim}} (mod.rs:550)
==21911==    by 0x1D0AD2: std::sys::unix::thread::Thread::new::thread_start (boxed.rs:2000)
==21911==  Address 0x527240a is 0 bytes after a block of size 10 alloc'd
==21911==    at 0x4E0713F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21911==    by 0x1A38A0: rust::cxxbridge1::errorCopy(char const*, unsigned long) (cxx.cc:452)
==21911==    by 0x1A399B: rust::cxxbridge1::Error::Error(rust::cxxbridge1::Error const&) (cxx.cc:472)
==21911==    by 0x1A4216: std::__exception_ptr::exception_ptr std::make_exception_ptr<rust::cxxbridge1::Error>(rust::cxxbridge1::Error) (exception_ptr.h:250)
==21911==    by 0x1A3AAA: cxxbridge1$default_exception (cxx.cc:555)
==21911==    by 0x19FDDD: cxx::result::CxxException::new_default (result.rs:66)
==21911==    by 0x18308F: <&T as cxx::result::ToCxxExceptionDefault>::to_cxx_exception (result.rs:214)
==21911==    by 0x1769A6: cxx_test_suite::ffi::_::__r_fail_return_primitive::__r_fail_return_primitive::{{closure}} (lib.rs:304)
==21911==    by 0x183E4A: core::result::Result<T,E>::map_err (result.rs:861)
==21911==    by 0x17B000: cxx_test_suite::ffi::_::__r_fail_return_primitive::__r_fail_return_primitive (lib.rs:304)
==21911==    by 0x1769D7: cxx_test_suite::ffi::_::__r_fail_return_primitive::{{closure}} (lib.rs:23)
==21911==    by 0x180CA8: cxx::unwind::prevent_unwind (unwind.rs:23)

@dtolnay Maybe Valgrind tests should be added in the CI? No idea how hard that is.

[dtolnay]

I think this is correct as currently implemented.

The valgrind output you pasted appears to be from code that is not in this repository. If you could share code that reproduces an out of bounds read using what is currently in the repository, I can have another look.

[schreter]

@dtolnay

I think this is correct as currently implemented.

Hm, well, OK, let's assume you get the message "test" from Rust. That is, pointer to 4 characters "test" + len = 4. You allocate 4 bytes and copy the 4 characters "test" into those 4 bytes.

Now, the exception is thrown and caught in the C++ code (via catch(rust::Error &e). The C++ code calls e.what() and receives the pointer to the 4 characters "test" (but no len - how? It's a const char*!). Now it wants to print it to an ostream for diagnosis purposes. OK, the operator<<(const char*) now starts iterating characters starting at the pointer and it receives 't', 'e', 's', 't', ... and then? Missing NUL terminator! So it happily starts printing the garbage afterwards, potentially running into an unmapped page and crashing the process.

Care to reconsider? 🙂

[dtolnay]

For "test", the len passed into this function is 5, not 4, so the memcpy already includes the original \0 byte.

[schreter]

@dtolnay

Sorry for making a fool of myself. You are right, the NUL byte is pushed in to_c_error() (well, it is inefficient as hell, but hey, we are on the error path; and yes, it is correct).

The valgrind output was from the change required for handling C++ exceptions via std::exception_ptr, which I pushed in PR #1180 (and which is in turn prerequisite for a few other things - like transparent exception passing through Rust). There, the default error is constructed from a Rust &str, which in turn is constructed by using formatting functions to format the error into a buffer, which then for most use cases saves all but 2 allocations which are strictly necessary to construct the exception (only the allocation of the string and the exception proper remains).

The original code allocated at least once extra for the to_string() (and depending on the error's Display trait implementation, likely one or two times more).

Anyway, the changes in the other PR are self-contained and valgrind-verified, so this change is indeed not needed.