Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0007468OCamlruntime system and C interfacepublic2017-01-26 15:122017-02-16 20:01
Reporteroandrieu 
Assigned Toxleroy 
PrioritynormalSeverityminorReproducibilityhave not tried
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version4.04.0 
Target Version4.06.0 +dev/beta1/beta2/rc1Fixed in Version4.06.0 +dev/beta1/beta2/rc1 
Summary0007468: GC-safety of caml_alloc_sprintf
DescriptionThe 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 InformationI can provide a patch if you agree on the analysis.
TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0017197)
xleroy (administrator)
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 (reporter)
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 (administrator)
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 (administrator)
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.

- Issue History
Date Modified Username Field Change
2017-01-26 15:12 oandrieu New Issue
2017-01-26 18:51 xleroy Assigned To => xleroy
2017-01-26 18:51 xleroy Status new => assigned
2017-01-26 18:53 xleroy Note Added: 0017197
2017-01-26 18:53 xleroy Status assigned => feedback
2017-01-27 17:07 oandrieu Note Added: 0017199
2017-01-27 17:07 oandrieu Status feedback => assigned
2017-01-29 18:32 xleroy Note Added: 0017207
2017-02-16 13:47 xleroy Target Version => 4.06.0 +dev/beta1/beta2/rc1
2017-02-16 20:01 xleroy Note Added: 0017298
2017-02-16 20:01 xleroy Status assigned => resolved
2017-02-16 20:01 xleroy Resolution open => fixed
2017-02-16 20:01 xleroy Fixed in Version => 4.06.0 +dev/beta1/beta2/rc1
2017-02-23 16:43 doligez Category OCaml runtime system => runtime system
2017-03-03 17:45 doligez Category runtime system => runtime system and C interface


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker