MantisBT - OCaml
View Issue Details
0007468OCamlruntime system and C interfacepublic2017-01-26 15:122017-02-16 20:01
oandrieu 
xleroy 
normalminorhave not tried
resolvedfixed 
4.04.0 
4.06.0 +dev/beta1/beta2/rc14.06.0 +dev/beta1/beta2/rc1 
0007468: GC-safety of caml_alloc_sprintf
The helper function caml_alloc_sprintf works by calling vsnprintf, then caml_alloc_string, then it may call vsnprintf again if the result does not fit in the 64 bytes static buffer.

This means that it's unsafe to use an OCaml string via String_val as the format string or as an argument since the caml_alloc_string call may trigger a GC and move the string around and leave caml_alloc_sprintf with a stale pointer.

In floats.c, caml_format_float calls caml_alloc_sprintf in such a way:
    res = caml_alloc_sprintf(String_val(fmt), d);
and in CamlInternalFormat, format_float is called with a constructed OCaml string which may be moved or reclaimed by the GC.

I can provide a patch if you agree on the analysis.
No tags attached.
Issue History
2017-01-26 15:12oandrieuNew Issue
2017-01-26 18:51xleroyAssigned To => xleroy
2017-01-26 18:51xleroyStatusnew => assigned
2017-01-26 18:53xleroyNote Added: 0017197
2017-01-26 18:53xleroyStatusassigned => feedback
2017-01-27 17:07oandrieuNote Added: 0017199
2017-01-27 17:07oandrieuStatusfeedback => assigned
2017-01-29 18:32xleroyNote Added: 0017207
2017-02-16 13:47xleroyTarget Version => 4.06.0 +dev/beta1/beta2/rc1
2017-02-16 20:01xleroyNote Added: 0017298
2017-02-16 20:01xleroyStatusassigned => resolved
2017-02-16 20:01xleroyResolutionopen => fixed
2017-02-16 20:01xleroyFixed in Version => 4.06.0 +dev/beta1/beta2/rc1
2017-02-23 16:43doligezCategoryOCaml runtime system => runtime system
2017-03-03 17:45doligezCategoryruntime system => runtime system and C interface

Notes
(0017197)
xleroy   
2017-01-26 18:53   
Your analysis sounds right, we probably have a GC issue here, thanks for pointing it out.

I can think of a couple ways to address it, but I'm interested in seeing yours.
(0017199)
oandrieu   
2017-01-27 17:07   
I was thinking of adding an caml_alloc_sprintf variant with an extra indirection for the format string, and calling it in caml_format_float with a local C root:
https://github.com/oandrieu/ocaml/commit/6b56448fcf1d41886e09137a29a7dcc029abe1cb [^]
(untested)
It's a bit complicated because of the variadic function, and now I realize that va_copy is C99, which means it's not available for MSVC < 2013 ...

Another option is to simply strdup() the OCaml string in caml_format_float.
(0017207)
xleroy   
2017-01-29 18:32   
Thanks for the sample code. The alternative I was considering is to caml_strdup() the format in caml_alloc_sprintf() before caml_alloc_string() is called, but only in the infrequent case we need to redo the snprintf.

What makes me grumpy is that I can't write a repro case for the bug, even by declaring caml_format_float as an external and calling it directly in the test with a format argument that resides in the minor heap.
(0017298)
xleroy   
2017-02-16 20:01   
Fix committed in [trunk 9e2177e], will be in 4.06. Still no repro case to put in the test suite.