Skip to content

Fix memory corruption when an exception is raised during demarshaling. - #14015

Merged
dra27 merged 3 commits into
ocaml:trunkfrom
bvaugon:trunk
May 6, 2025
Merged

dra27 merged 3 commits into
ocaml:trunkfrom
bvaugon:trunk

Conversation

@bvaugon

@bvaugon bvaugon commented May 5, 2025

Copy link
Copy Markdown
Contributor

Please refer to issue #14007 for details.

Comment thread runtime/intern.c Outdated
{
CAMLparam1 (str);
CAMLlocal1 (obj);
value obj;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I echo at @gasche in #14007 (comment) "including a code comment" 😊, but with a more concrete suggestion. Given that this is absolutely not a root (I also echo that it's a nice piece of detective work!), I think having the variable defined nowhere near the CAMLparam will reduce any temptation for a regression, regardless of the comment comment, here and also in the two other places where intern_rec is used like this. That would mean:

  • value res in caml_input_val moving down to just before the intern_rec call on L911
  • This one here moving down to just before the intern_rec call on L953
  • value obj in input_val_from_block moving down to just before the intern_rec call on L971

Each one of these instances conveniently includes a comment /* Fill it in which I think can each be expanded to Fill it in - obj must NOT be registered as a GC root */. It's important that that the result is both immediately passed to intern_end (which does register it as a root during intern_cleanup) and then returned, but I don't think that needs further comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's right. I pushed a new commit to follow these suggestions.

In fact, I think that it would be interesting to add some explanations in the file header of intern.c in which we may explicit some "invariants" of the demarshaling algorithm. Explain that no garbage collection may occur during demarshaling (except just before demarshaling on the first minor heap allocation and maybe during decompression, and just after demarshaling when an exception is allocated). Explain that when the number of demarshaled words is small, a place is reserved for them in the minor heap, but that huge demarshaled values are allocated into new major heap blocks. etc.

For whose interested in marshaling primitives, I have other issues/questions, especially:

  • Why there is a "COMPRESSED" flag declared in extern.c:920 and no COMPRESSED value declared in marshal.ml:16 ?
  • Is it normal that closures cannot be demarshaled (and seems to be lost forever) on Windows with the same program but after rebooting (or after remounting the disk on which the executable program is) ? Or is it a link option to activate (like -fPIE, knowing that this one does not solve this problem) to allow demarshaling of closures on Windows after rebooting ?

But a new thread may be opened for that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Compressed marshalling isn't exposed in the Standard Library (see the various PRs around 5.1.x) - the facility is at present only available to the compiler, or by direct use of the extern primitives; cf. utils/compression.ml

The Windows issue is not familiar to me - please can have an issue, yes!

…id non-root variables to be registered as GC roots.

@gasche gasche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @bvaugon for the deep fix and @dra27 for the excellent suggestions to make the fix more self-documenting.

@gasche

gasche commented May 6, 2025

Copy link
Copy Markdown
Member

The bug is also present in 4.14 (I think it was introduced in 63e272e), so I suppose that we want to backport the fix to all maintained OCaml releases (I guess 4.14, 5.3, 5.4).

@gasche gasche added the merge-me label May 6, 2025
@dra27
dra27 merged commit e6321cf into ocaml:trunk May 6, 2025
dra27 pushed a commit that referenced this pull request May 6, 2025
dra27 pushed a commit that referenced this pull request May 6, 2025
dra27 pushed a commit that referenced this pull request May 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants