extern.c: raise OOM instead of passing null in caml_output_* entrypoints

[askvortsov1]

Fix for #12037

Summary

-fanalyzer detected a potential null dereference in extern.c, because the current implementation of get_extern_state returns NULL on OOM instead of erroring properly. There are two cases where this helper function might be called:

• caml_output_* or caml_obj_reachable_words entrypoints, where we should initialize extern_state if it doesn't yet exist. In this case, an out_of_memory should be thrown if necessary, instead of passing along NULL, which would segfault.
• caml_serialize_* helpers, which should only be callable from inside a caml_output_* context. These should throw a fatal error if extern_state is null, since that means they are being called outside the caml_output_* context.

get_extern_state has been split into prepare_extern_state and get_extern_state, which handle the two above cases respectively.

Question for Reviewers

In discussion on the issue, @gasche proposed that prepare_extern_state should throw if the state is initialized, and get_extern_state should throw if it isn't; that way, using entrypoints without cleanup, or serialization helpers out of an entrypoint, would both error.

Unfortunately, this wouldn't work, since the memory block allocated for extern_state is reused between entrypoint output calls. Instead, we could add an in_progress flag that would be toggled on prepare_extern_state init calls and extern_free_state cleanup calls. Then, we could error if output-ing while another output is in progress, or serialize-ing if no output is in progress. I took a first step towards this by factoring out common code used in init-ing/reset-ing the stack. Is this something we'd like to add?

[gasche]

I am happy with the new iteration of this PR, and I think that it does solve the issue.

You are indeed correct that failing in prepare_exn if the extern state is already allocated is not quite correct, unless we are wiling to reallocate it each time. I think that we should not bother doing that -- it is wasteful and the benefit we get from failing in prepare_exn would merely be to find reentrancy bugs that we would probably detect in other ways as well. So ignoring that part of my recommendation was the right move, I think.

[askvortsov1]

How would you feel about another PR that would are a flag to extern_state as described in this PR's description? That seems like a reasonable way to handle those bugs without incurring excessive performance cost.

[gasche]

@askvortsov1 my honest impression is that it would not be worth the effort. I think that there are better ways to spend our time (both hacking time and reviewing time). (Then I tried to list a few, purely on the runtime, and fell short: the cleanup-at-exit feature needs reimplementing, #10865, but there are already people who are planning to work on it, and the gc statistics probably need to change / be fixed in the multicore world, see discussion in #11751 (comment) , but these need to acquire more knowledge of the runtime so they are less self-contained. )