Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GC-safety of caml_alloc_sprintf #7468

Closed
vicuna opened this issue Jan 26, 2017 · 4 comments
Closed

GC-safety of caml_alloc_sprintf #7468

vicuna opened this issue Jan 26, 2017 · 4 comments
Assignees
Milestone

Comments

@vicuna
Copy link

vicuna commented Jan 26, 2017

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
Copy link
Author

vicuna commented Jan 26, 2017

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
Copy link
Author

vicuna commented Jan 27, 2017

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
Copy link
Author

vicuna commented Jan 29, 2017

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
Copy link
Author

vicuna commented Feb 16, 2017

Comment author: @xavierleroy

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

@vicuna vicuna closed this as completed Feb 16, 2017
@vicuna vicuna added the stdlib label Mar 14, 2019
@vicuna vicuna added this to the 4.06.0 milestone Mar 14, 2019
@vicuna vicuna added the bug label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants