Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: caml_fatal_error on misuse of deserialize functions #12635

Merged
merged 3 commits into from
Oct 8, 2023

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Oct 5, 2023

Mimicking #12171, this PR gets intern.c closer to extern.c. It fixes a problem with possible misuse of deserialize functions, as these should only be called from within a caml_input_* entrypoint context. Otherwise, the field intern_src of struct caml_intern_state won't have been populated, and a null pointer dereference occurs.

cc @askvortsov1 and @gasche who authored and reviewed the original PR.

Changes Outdated Show resolved Hide resolved
runtime/intern.c Outdated Show resolved Hide resolved
runtime/intern.c Outdated Show resolved Hide resolved
runtime/intern.c Outdated Show resolved Hide resolved
runtime/intern.c Outdated
if (Caml_state->intern_state == NULL)
caml_fatal_error (
"intern_state not initialized: "
"this function can only be called from a `caml_input_*` entrypoint."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with the first line of the error message "intern_state not initialised". I don't know whether the rest of the message is clarifying the error more "this function can only be called from a caml_input_* entrypoint.".

When will this error occur? Is it that caml_deserialise_* was called directly without calling one of the caml_input_* functions?

Copy link
Contributor Author

@MisterDA MisterDA Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When will this error occur? Is it that caml_deserialise_* was called directly without calling one of the caml_input_* functions?

Exactly. In that case, before this PR, the field intern_src pointing to the source of the marshaled data won't have been populated, and would be accessed, leading to the nullptr dereference. By splitting initialization of the structure (in init_intern_state) and then access (with get_intern_state), we can check whether it has been provided a marshaled data source, and error if not (admittedly nicer than a segfault).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I request that you add a testcase for this under testsuite/tests/lib-marshal?

Also, the message can be more clear. Instead of

"this function can only be called from a caml_input_* entrypoint."

How about

"It is likely that caml_deserialize_* function was called without going through caml_input_*.

Copy link
Contributor Author

@MisterDA MisterDA Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I request that you add a testcase for this under testsuite/tests/lib-marshal?

Maybe easier said than done? As the functions raise a fatal error, I can only test one of them by process. A failwith would allow to catch the exception and recover.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe easier said than done?

This is doable. See .run files in the test suite for how it is done. But it is not worth the trouble here as the bytecode and native code behaviours will diverge. Bytecode will perform an intern at

value global_data = caml_input_val(chan);

before user code is run. This will cause any test to not cause a fatal error, which is different from native code.

These should only be called from within a `caml_input_*` entrypoint
context. Otherwise, the field `intern_src` of `struct
caml_intern_state` won't have been populated, and a null pointer
dereference occurs.
Split out `init_extern_stack` into a separate function, so that the
code for initializing and resetting the intern_state stack is not
duplicated.
@kayceesrk kayceesrk merged commit 9b68959 into ocaml:trunk Oct 8, 2023
9 checks passed
@MisterDA MisterDA deleted the intern_state_refactor branch October 8, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants