get_extern_state potential NULL dereference

[edwintorok]

Found by GCC '-fanalyzer':
output.txt

gcc -c -O2 -fno-strict-aliasing -fwrapv -pthread -g -Wall -Wint-conversion -Wstrict-prototypes -Wold-style-definition -Werror -fno-common -fexcess-precision=standard -fno-tree-vrp -ffunction-sections -ggdb -fsanitize=address -fsanitize=bool,builtin,bounds,enum,nonnull-attribute,null,object-size,pointer-overflow,returns-nonnull-attribute,shift-exponent,unreachable -fsanitize-undefined-trap-on-error -fno-omit-frame-pointer -O1 -I./runtime -D_FILE_OFFSET_BITS=64 -DCAMLDLLIMPORT= -DIN_CAML_RUNTIME -DDEBUG -O2  runtime/extern.c -fanalyzer
gcc -c -O2 -fno-strict-aliasing -fwrapv -pthread -g -Wall -Wint-conversion -Wstrict-prototypes -Wold-style-definition -Werror -fno-common -fexcess-precision=standard -fno-tree-vrp -ffunction-sections -ggdb -fsanitize=address -fsanitize=bool,builtin,bounds,enum,nonnull-attribute,null,object-size,pointer-overflow,returns-nonnull-attribute,shift-exponent,unreachable -fsanitize-undefined-trap-on-error -fno-omit-frame-pointer -O1 -I./runtime -D_FILE_OFFSET_BITS=64 -DCAMLDLLIMPORT= -DIN_CAML_RUNTIME -DDEBUG -O2 -o runtime/intern.bd.o runtime/intern.c -fanalyzer

The code at

ocaml/runtime/extern.c

Lines 127 to 131 in d71ea3d

	extern_state =
	caml_stat_alloc_noexc(sizeof(struct caml_extern_state));
	if (extern_state == NULL) {
	return NULL;
	}

uses the no exception version of the allocation function, and returns NULL on OOM. 'intern.c' is different because it uses the exception raising variant.

However in 'extern.c' the callers do not check for NULL and would crash on a NULL pointer dereference as pointed out by GCC above. There was probably a reason why the code wanted to return NULL here (is it not safe to raise out of memory exception?), but I cannot find the reason in git history (I got as far as 868265f). It would seem to me that the code should either raise out of memory here or handle the NULL return value in some way (e.g. by returning early with an error).

[gasche]

The get_extern_state is called in two different kind of places:

1. at the start of the various caml_output_* entry points; at this point we don't actually need the _noexc variant and we could fail with an out-of-memory error, from the function or at the caller
2. in the middle of the serialization functions like caml_serialize_int_*. At this point it is unsafe to fail with an exception, and we probably want to even avoid the cost of NULL-checking and a failure code path.

I may be wrong, but I believe that the current code will never fail with NULL in the case (2) above, because the function has already been called at an entry point (1) before, so we implicitly know that the extern-state has already been allocated at this point. The function could still fail at the entry point and we don't have the appropriate checks for them.

In the short term, we should thus introduce NULL-checks on calls to get_extern_state in the entry points (1), where we can fail with an exception. We would then know that the code is actually correct (the calls in (2) will never NULL out), but fanalyzer may still disagree because we would rely on a non-obvious invariant.

In the medium term, it would be nice to have two different functions for (1) and (2), an initialization function that fails on OOM for (1), and an re-access function for (2) that assumes (and possibly asserts) that the initialization function has already been called.

[gasche]

cc @xavierleroy : does that sound right to you?

[gasche]

For @edwintorok (and: thanks!): the reason why we don't want to fail with exceptions from within the extern logic is that the extern logic is not reentrant for performance reasons, there is explicit state-cleanup logic to call before we return to OCaml code (which could catch the exception and then try to serialize things again). This being said, we are talking here about the specific case where we failed to allocate the extern state, so maybe there is nothing to cleanup and we could fail with an exception at this point.

(Before the 'extern state' was a bunch of global variables, now it is in domain-local state for obvious multicore-safety reasons.)

[xavierleroy]

@gasche's explanation sounds about right. It might be clearer to go even further and have two functions:

1- get_extern_state, to be called from the caml_serialize_* functions, that does not even try to allocate the extern state, it just assumes there is one already:

Caml_inline static struct caml_extern_state* get_extern_state (void)
{
  Caml_check_caml_state();
  return Caml_state->extern_state;
}

2- prepare_extern_state, to be called from caml_output_* entry point, that allocates the extern state if necessary, and that could raise an Out_of_memory exception (I guess).

[askvortsov1]

I think I'll give it a shot!

[askvortsov1]

@gasche I've drafted a PR at #12171, but I have a few questions before I undraft it and fill it out for review.

First and foremost, I can't find what guarantees the aforementioned invariant that caml_output_* would be an entrypoint to caml_serialize_*. Looking through the code, I see that:

• Marshall.* and output_value OCaml functions are defined externally through caml_output_* functions in extern.c
• caml_serialize_* functions are exposed as custom_operations structs for bigarray, int64, int32, etc
• caml_serialize_* functions also seem to be exposed through intext.h, to be used as custom serialization functions.

This is my first time working on the OCaml compiler runtime (or any compiler or runtime for that matter), so maybe I'm missing something obvious, but I can't find why serialize calls would necessarily have to follow output calls, even though that is reasonable behavior.

Secondly, regarding tests, I re-ran the gcc -fanalyzer command listed in the issue, as well as the existing test suite, but I'm not sure how we would unit test a situation where memory can't be allocated.

[gasche]

@askvortsov1 that's a good question. The caml_serialize_* functions are exposed to the outside as custom serialization operations, but my understanding is that the API does not in fact let users call them directly as entry points for a serialization operation, instead it can only be called from within a serialization context, that is, by the extern_custom in extern.c that serializes custom blocks. They are exposed to the user to let users define their own serialization functions on top of them, but those functions would in turn also be called in a serialization context.

My current understanding is as follows:

• it would still be nice to have a prepare_extern_state function that raises a clean error if it fails, to be called in the caml_output_* entry points
• we cannot rule out the risk of misuse of caml_serialize_* functions by the user, so it would still make sense to have a dynamic check in those that caml_fatal_error if the function is misused. In fact maybe we could fail hard already if Caml_state->extern_state is NULL in this context, instead of trying to allocate a new state.

Summary:

• prepare_extern_state checks that no extern state is currently set (it fatal-errors otherwise), and raises Out_of_memory if allocating the extern state fails. It is called by caml_output_* entry points.
• get_extern_state checks that the extern state is set (it fatal-errors otherwise) and returns it directly, it is called by the caml_deserialize_* functions that should not be entry points.

[askvortsov1]

@gasche thanks, and sorry for the delay on my part. #12171 should be ready for review now!

[gasche]

This has been solved by merging #12171. Thanks everyone!