{"action":"created","issue":{"url":"https://api.github.com/repos/ocaml/ocaml/issues/283","labels_url":"https://api.github.com/repos/ocaml/ocaml/issues/283/labels{/name}","comments_url":"https://api.github.com/repos/ocaml/ocaml/issues/283/comments","events_url":"https://api.github.com/repos/ocaml/ocaml/issues/283/events","html_url":"https://github.com/ocaml/ocaml/pull/283","id":115539936,"number":283,"title":"Fix memory leaks in intern.c when OOM is raised","user":{"login":"mlasson","id":5814258,"avatar_url":"https://avatars.githubusercontent.com/u/5814258?v=3","gravatar_id":"","url":"https://api.github.com/users/mlasson","html_url":"https://github.com/mlasson","followers_url":"https://api.github.com/users/mlasson/followers","following_url":"https://api.github.com/users/mlasson/following{/other_user}","gists_url":"https://api.github.com/users/mlasson/gists{/gist_id}","starred_url":"https://api.github.com/users/mlasson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mlasson/subscriptions","organizations_url":"https://api.github.com/users/mlasson/orgs","repos_url":"https://api.github.com/users/mlasson/repos","events_url":"https://api.github.com/users/mlasson/events{/privacy}","received_events_url":"https://api.github.com/users/mlasson/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":6,"created_at":"2015-11-06T16:30:59Z","updated_at":"2015-11-06T21:27:09Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ocaml/ocaml/pulls/283","html_url":"https://github.com/ocaml/ocaml/pull/283","diff_url":"https://github.com/ocaml/ocaml/pull/283.diff","patch_url":"https://github.com/ocaml/ocaml/pull/283.patch"},"body":"In the function, `intern_alloc` a call to caml_alloc_for_heap is very likely to\r\nreturn NULL when reading a big marshaled value. If that happens, before raising\r\nout_of_memory, it should call the `intern_cleanup` function to free the stack\r\nas well as `intern_input` that may have been malloced by `caml_input_val`.\r\nSimilarly, `intern_cleanup` should also be called when we are not able to\r\nallocate `intern_obj_table`. To do that, I added a function\r\n`caml_stat_alloc_no_raise` which, like its brother, `caml_stat_alloc` wraps\r\nsome debugging information around a call to malloc. I could have used directly\r\nmalloc instead of adding a new function to memory.c, as it is done in other\r\nplaces of the code (it has the drawback of not adding the debug tag).\r\n\r\nNote that this fix is not perfect. The function `intern_alloc` could also raise\r\nout_of_memory through its call to `caml_alloc_shr`. It is less likely to happen\r\nsince caml_alloc_shr is only called when the input is smaller than Max_wosize\r\nbut it could happen. In that case, there will be leak (but a smaller one)."},"comment":{"url":"https://api.github.com/repos/ocaml/ocaml/issues/comments/154546731","html_url":"https://github.com/ocaml/ocaml/pull/283#issuecomment-154546731","issue_url":"https://api.github.com/repos/ocaml/ocaml/issues/283","id":154546731,"user":{"login":"gasche","id":426238,"avatar_url":"https://avatars.githubusercontent.com/u/426238?v=3","gravatar_id":"","url":"https://api.github.com/users/gasche","html_url":"https://github.com/gasche","followers_url":"https://api.github.com/users/gasche/followers","following_url":"https://api.github.com/users/gasche/following{/other_user}","gists_url":"https://api.github.com/users/gasche/gists{/gist_id}","starred_url":"https://api.github.com/users/gasche/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gasche/subscriptions","organizations_url":"https://api.github.com/users/gasche/orgs","repos_url":"https://api.github.com/users/gasche/repos","events_url":"https://api.github.com/users/gasche/events{/privacy}","received_events_url":"https://api.github.com/users/gasche/received_events","type":"User","site_admin":false},"created_at":"2015-11-06T21:27:09Z","updated_at":"2015-11-06T21:27:09Z","body":"I haven't looked at the details yet (nor would I trust myself to have a correct intuition here), but I find the idea of a version of `caml_alloc_shr` that does not raise rather suspect: it's a complex beast, there is no easy to know before hand whether it will fail or not, and it would be easy to introduce a bug while trying to fix this issue.\r\n\r\nAfter looking at `intern_cleanup`: I wonder if a different approach wouldn't be possible and safer. Leaking one `caml_alloc_shr` is not a strong issue (although \"zero leak\" is a much more comfortable place to be in for reasoning), the problem is with code that catches the `Out_of_memory` exception and keeps running into this leak repeatedly. So instead of trying to be very careful to cleanup before leaving `intern_alloc` with an out_of_memory exception, couldn't you try to detect, on the *next* entry in `intern`, that a cleanup is missing and should be performed first? This could possibly lead to a less tricky patch (than your proposed `caml_alloc_shr_noraise`), as you would just set some `cleanup_if_I'm_still_set` global flag before the dangerous operation (and make sure that the global state is cleanable at each failure point)."}}