otherlibs/systhreads/st_stubs.c: Avoid free of uninitialized pointer

[rwmjones]

Calling caml_c_thread_unregister() will (amongst many other things) free the caml_thread_t signal_stack entry of the current thread. The OCaml runtime initializes the main thread using special code in caml_thread_domain_initialize_hook, but this leaves the signal_stack entry uninitialized.

If you use glibc tunables to enable malloc checking, and especially use the malloc.perturb feature[1], then uninitialized areas of memory are filled with a repeating pattern. This causes the process to crash if you free an uninitialized pointer.

Thus any program which calls caml_c_thread_unregister on the main thread, and is using glibc malloc checking, will crash.

Fix this by initializing the signal_stack to NULL (since free(NULL) is permitted and does nothing).

This was found when debugging a problem in the nbdkit OCaml plugin[2].

[1] https://www.gnu.org/software/libc/manual/html_node/Memory-Allocation-Tunables.html
[2] https://discuss.ocaml.org/t/free-uninitalized-data-when-calling-caml-c-thread-unregister-on-the-main-thread/15198

Fixes: #13400

[gadmm]

I think you are not supposed to call caml_c_thread_unregister on the main thread. Nevertheless it's good to initialise the signal_stack field.

[gasche]

This also looks fine to me. Thanks!

The PR as-is targets 5.2, but I would prefer to merge in trunk directly. If you can change the branch, please do, otherwise I can close and push manually. I would then cherry-pick in the 5.3 branch anyway.

I think that this would deserve a Changes entry as it fixes a user-facing issue, and to credit your external contribution.

[rwmjones]

Hang on, I'll try to retarget it ...

[gadmm]

Please add a Changes entry but

as it fixes a user-facing issue

while it is good to initialize memory, is it technically a bug to free a block whose memory was (partly) left uninitialized?

[gasche]

while it is good to initialize memory, is it technically a bug to free a block whose memory was (partly) left uninitialized?

My understanding is that the bug reported is not that we free a caml_thread_t that is partially uninitialized, but that we call free on the signal_stack field (through the call to caml_free_signal_stack in thread_detach_from_runtime) that is unitialized.

If I read the code correctly, the code will still fail now that signal_stack is a proper NULL, so arguably we are turning an obscure UB bug in a clean failure (rather than fixing a bug per se). Anyway.

[rwmjones]

My understanding is that the bug reported is not that we free a caml_thread_t that is partially uninitialized, but that we call free on the signal_stack field (through the call to caml_free_signal_stack in thread_detach_from_runtime) that is unitialized.

This is correct, you call free on an uninitialized pointer (when glibc tunables are used to pollute uninitialized memory). From the stack trace notice the pointer is 0xd5d5... where 0xd5 is the complement of malloc.perturb=42:

#7  free_check (mem=mem@entry=0xd5d5d5d5d5d5d5d5)
    at /usr/src/debug/glibc-2.40-3.fc41.x86_64/malloc/malloc-check.c:228
#8  0x00007ffff7fa0111 in free_check (mem=0xd5d5d5d5d5d5d5d5)
    at /usr/src/debug/glibc-2.40-3.fc41.x86_64/malloc/malloc-check.c:215
#9  __debug_free (mem=0xd5d5d5d5d5d5d5d5) at malloc-debug.c:208

Note free(NULL) is a no-op, according to POSIX.

review by Guillaume Munch-Maccagnoni and Gabriel Scherer

Will modify in a moment.

[gasche]

Note free(NULL) is a no-op, according to POSIX.

My impression is that the code of caml_free_signal_stack is not designed to support this and will do weird things in this case -- in particular there is an assertion that the signal stack is not NULL, so this use-case will abort on assert failure in debug mode. If you wanted to ensure that this use-case is supported, I guess you should add an early exit in caml_free_signal_stack on NULL inputs, or check this in the caller.

[rwmjones]

Note free(NULL) is a no-op, according to POSIX.

My impression is that the code of caml_free_signal_stack is not designed to support this and will do weird things in this case -- in particular there is an assertion that the signal stack is not NULL, so this use-case will abort on assert failure in debug mode.

The plan to fix this in nbdkit is to not call caml_c_thread_* at all on the main thread. We didn't know how to do this before as there's no "this is the main thread" or "this thread is already registered" API in OCaml, but @gadmm pointed out that it could be done by keeping our own thread-local flag.

If you wanted to ensure that this use-case is supported, I guess you should add an early exit in caml_free_signal_stack on NULL inputs, or check this in the caller.

Shouldn't caml_c_thread_* functions ignore the case when you're trying to register a thread that is already known [actually it does this already] or unregister one of these "main threads"? IDK, anyway the patch fixes the crash and the fix above should resolve it properly.

[gadmm]

while it is good to initialize memory, is it technically a bug to free a block whose memory was (partly) left uninitialized?

My understanding is that the bug reported is not that we free a caml_thread_t that is partially uninitialized, but that we call free on the signal_stack field (through the call to caml_free_signal_stack in thread_detach_from_runtime) that is unitialized.

This does not sound like a user-facing issue in this sense either; per the documentation caml_c_thread_unregister should only be called after a succesful caml_c_thread_register, and it is out of reach to make the C FFI entirely memory-safe in the face of all possible incorrect use (unfortunately).

[gasche]

Anyway, I think the current change is good -- even if indeed the users should maybe rethink their implementation.

(I see that over at #13400 @gadmm has provided useful feedback on how to write the code differently. If you do happen to come up with additions to the current API to improve @rwmjones' use-case, I would be happy to consider implementing them or reviewing their implementation. For now it look like a purely user-side solution could be reasonable, so maybe there is no need.)