fix random failure in Marshal.from_{string,bytes} (PR#12056)#12064
fix random failure in Marshal.from_{string,bytes} (PR#12056)#12064xavierleroy merged 1 commit intoocaml:trunkfrom
Conversation
I read the comment as not being about memory safety, but rather about concerns on the maximal memory usage of the process (so more like OOM conditions, etc.). If I take a large compressed payload that decompresses into an even larger uncompressed payload, I don't want to have to hold the two alive at the same time in my memory (irrespectively of whether they are GG- or malloc-owned). My proposal for the fix:
Note: the bug that you are fixing was introduced by the decompression PR. The test was already flaky before that, but it failed much less often. There are probably other bugs still lurking. |
That's exactly how I read it too, and it's still misleading because
Yes, I forgot to mention that. I tried a bit, but failed to reproduce it. |
|
OK, so it was my fault after all, sorry for having introduced this bug in #12006. For |
|
(We get to blame the flaky test. It's its fault if we didn't trust it when it failed on the PR. Maybe.) |
xavierleroy
left a comment
There was a problem hiding this comment.
Good fix, thanks. I still like the comment you removed, but won't fight for it.
Changes
Outdated
| - #12056, #12057, #??: Random failure in Marshal.from_{string,bytes} | ||
| (Damien Doligez, review by ??) |
There was a problem hiding this comment.
I'll let the Changelog police tell you if you need a separate entry for these or if they should be merged with #12006 .
There was a problem hiding this comment.
police heree: it's as Damien prefers.
| intern_alloc_storage(s, h.whsize, h.num_objects); | ||
| s->intern_src = &Byte_u(str, ofs + h.header_len); /* If a GC occurred */ | ||
| /* Decompress if needed */ | ||
| intern_decompress_input(s, "input_val_from_string", &h); |
There was a problem hiding this comment.
Yes, this is obviously more correct :-) and there's no regrets w.r.t. memory usage, since str is in the OCaml heap (it's not malloc-ed) and live throughout the function, so intern_decompress_input has no chance to free it.
gasche
left a comment
There was a problem hiding this comment.
(I also believe that the change is correct.)
Oh that's right, I missed the shared object table. I'll restore the comment and update Changes, then I'll merge. |
0b912cb to
27aa297
Compare
|
Looks great, thanks! Now merging... |
The problem is pretty basic after all: a minor GC occurs while
caml_input_val_from_bytesis holding an infix pointer into a string located in the minor heap.The fix involves moving the call to
intern_decompress_inputafterintern_alloc_storageand removing the comment that says not to do that. The comment was misleading becauseintern_alloc_storageonly allocates in the minor heap whileintern_decompress_inputfrees memory inmallocland, so they can't interact anyway.Fixes (one part of) #12056