GC-safety of caml_alloc_sprintf

[vicuna]

Original bug ID: 7468
Reporter: @oandrieu
Assigned to: @xavierleroy
Status: resolved (set by @xavierleroy on 2017-02-16T19:01:55Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.04.0
Target version: 4.06.0 +dev/beta1/beta2/rc1
Fixed in version: 4.06.0 +dev/beta1/beta2/rc1
Category: runtime system and C interface

Bug description

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.

Additional information

I can provide a patch if you agree on the analysis.

[vicuna]

Comment author: @xavierleroy

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.

[vicuna]

Comment author: @oandrieu

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:
oandrieu@6b56448
(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.

[vicuna]

Comment author: @xavierleroy

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.

[vicuna]

Comment author: @xavierleroy

Fix committed in [trunk 9e2177e], will be in 4.06. Still no repro case to put in the test suite.