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

caml_invalid_argument can't be used with dynamically allocated strings #7423

Closed
vicuna opened this issue Nov 29, 2016 · 4 comments
Closed

caml_invalid_argument can't be used with dynamically allocated strings #7423

vicuna opened this issue Nov 29, 2016 · 4 comments

Comments

@vicuna
Copy link

@vicuna vicuna commented Nov 29, 2016

Original bug ID: 7423
Reporter: @stijn-devriendt
Assigned to: @mshinwell
Status: closed (set by @mshinwell on 2016-12-27T09:18:35Z)
Resolution: duplicate
Priority: normal
Severity: feature
Platform: Linux
OS: Ubuntu
OS Version: 14.04
Version: 4.03.0
Category: runtime system and C interface
Monitored by: @gasche "Richard Jones"

Bug description

From https://sympa.inria.fr/sympa/arc/caml-list/2016-11/msg00106.html

caml_invalid_argument(str) is no return and does not free it's
argument. So calling it with a string constructed dynamically will mean it'll never get freed.
I could construct an ocaml string and pass that to caml_raise_with_arg,
but I don't seem to be able to get to the caml_exn_Invalid_argument from
c.

Reply from Gabriel Scherer:
It may make sense to have caml_{failwith,invalid_argument}_value
variants of the exception-raising functions that take a parameter, and
be implemented using caml_raise_with_arg(s) directly instead of
caml_raise_with_string. Could you open a mantis issue or submit a
github pull request to track the question and continue discussion?

Steps to reproduce

char* error = new char[50];
snprintf(error, 50, "error: %d", 50);
caml_invalid_argument(error)

allocated memory is lost here.

@vicuna
Copy link
Author

@vicuna vicuna commented Dec 3, 2016

Comment author: @gasche

Patch proposed in #946

@vicuna
Copy link
Author

@vicuna vicuna commented Dec 3, 2016

Comment author: @xavierleroy

I'm not convinced by the repro case (just declare "char error[50];") nor by the need in general.

@vicuna
Copy link
Author

@vicuna vicuna commented Dec 3, 2016

Comment author: @gasche

Richard W. Jones mentioned that he could use a similar feature on the mailing-list: https://sympa.inria.fr/sympa/arc/caml-list/2016-12/msg00021.html

We have the same problem in libguestfs in a few places. The solution
is to use an allocation on the C stack, either a fixed size buffer or
[although we don't currently use this] a variable sized one using
alloca. The string is freed when the stack is unwound. Examples:

https://github.com/libguestfs/libguestfs/blob/master/builder/yajl-c.c#L108-L114
https://github.com/libguestfs/libguestfs/blob/master/v2v/domainxml-c.c#L120

Be nice to have a "freeing" version of caml_raise* I suppose.

The stack-based solutions rely on guessing arbitrary size limits on the dynamic strings, which is often possibles but sometimes inconvenient/inelegant.

@vicuna
Copy link
Author

@vicuna vicuna commented Dec 27, 2016

Comment author: @mshinwell

Moving discussion to here: #946

@vicuna vicuna closed this as completed Dec 27, 2016
gadmm added a commit to gadmm/ocaml that referenced this issue Jul 9, 2020
Introduce variants of the functions that do not immediately raise the
exception, allowing for resource cleanup.

This is used in this way in an ulterior commit to avod duplicating
cleanup code.

Another motivating example is given at ocaml#7423:

> caml_invalid_argument(str) is no return and does not free it's
argument. So calling it with a string constructed dynamically will
mean it'll never get freed.

This led at the time to the introduction of
caml_invalid_argument_value and caml_failwith_value that accept an
OCaml string as argument. The present commit gives a general solution
that fits with the rest of the resource-safe API.
gadmm added a commit to gadmm/ocaml that referenced this issue Jul 9, 2020
Introduce variants of the functions that do not immediately raise the
exception, allowing for resource cleanup.

This is used in this way in an ulterior commit to avod duplicating
cleanup code.

Another motivating example is given at ocaml#7423:

> caml_invalid_argument(str) is no return and does not free it's
argument. So calling it with a string constructed dynamically will
mean it'll never get freed.

This led at the time to the introduction of
caml_invalid_argument_value and caml_failwith_value that accept an
OCaml string as argument. The present commit gives a general solution
that fits with the rest of the resource-safe API.
gadmm added a commit to gadmm/ocaml that referenced this issue Jul 12, 2020
Introduce variants of the functions that do not immediately raise the
exception, allowing for resource cleanup.

This is used in this way in an ulterior commit to avod duplicating
cleanup code.

Another motivating example is given at ocaml#7423:

> caml_invalid_argument(str) is no return and does not free it's
argument. So calling it with a string constructed dynamically will
mean it'll never get freed.

This led at the time to the introduction of
caml_invalid_argument_value and caml_failwith_value that accept an
OCaml string as argument. The present commit gives a general solution
that fits with the rest of the resource-safe API.
gadmm added a commit to gadmm/ocaml that referenced this issue Jul 13, 2020
Introduce variants of the functions that do not immediately raise the
exception, allowing for resource cleanup.

This is used in this way in an ulterior commit to avod duplicating
cleanup code.

Another motivating example is given at ocaml#7423:

> caml_invalid_argument(str) is no return and does not free it's
argument. So calling it with a string constructed dynamically will
mean it'll never get freed.

This led at the time to the introduction of
caml_invalid_argument_value and caml_failwith_value that accept an
OCaml string as argument. The present commit gives a general solution
that fits with the rest of the resource-safe API.
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