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

Runtime segmentation fault (toplevel and ocamlc, OCaml >= 4.06.0) #8681

Closed
monadius opened this issue May 19, 2019 · 5 comments

Comments

@monadius
Copy link

commented May 19, 2019

The following simple program successfully compiles and causes Segmentation fault: 11 at runtime (toplevel and ocamlc, ocamlopt is not affected):

let rec f =
  let rec f t = g t
  and g t = t in
  f;;

Gc.minor();;

f 0;;

Tested OCaml versions (installed via opam), OS: macOS 10.14.4:

  • OCaml 4.05.0: no runtime errors (ocamlc and ocaml)
  • OCaml 4.06.0, 4.06.1, 4.07.1, 4.10.0+trunk: Segmentation fault: 11 (ocamlc and ocaml)

Sometimes the following error message is displayed (ocamlc 4.06.0, 4.06.1, 4.07.1, 4.10.0+trunk):

ocamlrun(39593,0x1074715c0) malloc: can't allocate region
*** mach_vm_map(size=183345762954911744) failed (error code=3)
ocamlrun(39593,0x1074715c0) malloc: *** set a breakpoint in malloc_error_break to debug
Fatal error: out of memory.
@gasche

This comment has been minimized.

Copy link
Member

commented May 19, 2019

The problem comes from the interaction between the value-recursion compilation mechanism (dummy value creation and update), for the outer let rec f = ... binding, and the shared-closure representation of the inner (f,g) recursive functions: the static "size" computed for the dummy value is wrong as it does not correctly take into account the shared closure representation.

In bytecode, the following patch fixes the issue:

diff --git a/bytecomp/bytegen.ml b/bytecomp/bytegen.ml
index c7343bfc1d..3227bfcfc3 100644
--- a/bytecomp/bytegen.ml
+++ b/bytecomp/bytegen.ml
@@ -159,11 +159,29 @@ let rec size_of_lambda env = function
   | Llet(_str, _k, id, arg, body) ->
       size_of_lambda (Ident.add id (size_of_lambda env arg) env) body
   | Lletrec(bindings, body) ->
-      let env = List.fold_right
-        (fun (id, e) env -> Ident.add id (size_of_lambda env e) env)
-        bindings env
-      in
-      size_of_lambda env body
+      let env =
+        if List.for_all (function (_, Lfunction _) -> true | _ -> false) bindings
+        then
+          (* letrec of functions; special size computation for the function:
+               2 * number of functions - 1 + number of free variables
+             (see CLOSUREREC case in interp.c) *)
+          let nfuncs = List.length bindings in
+          let nvars =
+            List.map (fun (_id, e) -> free_variables e) bindings
+            |> List.fold_left Ident.Set.union Ident.Set.empty
+            |> Ident.Set.cardinal
+          in
+          let closize = RHS_function (2 * nfuncs - 1 + nvars,
+                                      1 (*FIXME arity? for js_of_ocaml*)) in
+          List.fold_right
+            (fun (id, _e) env -> Ident.add id closize env)
+            bindings env
+        else
+          List.fold_right
+            (fun (id, e) env -> Ident.add id (size_of_lambda env e) env)
+            bindings env
+        in
+        size_of_lambda env body
   | Lprim(Pmakeblock _, args, _) -> RHS_block (List.length args)
   | Lprim (Pmakearray ((Paddrarray|Pintarray), _), args, _) ->
       RHS_block (List.length args)
-- 

However, the issue can also be reproduced with the native compiler, by using a more complex example where ocamlopt cannot remove the recursion away, such as:

let rec h =
  let rec f t = g t
  and g t = h t; f t in
  f

let () = Gc.minor ()
let () = ignore (h 0)

so the size computation needs to be fixed in the native-code compiler as well.

@gasche

This comment has been minimized.

Copy link
Member

commented May 19, 2019

The following patch seems to fix the native-code compiler as well:

diff --git a/asmcomp/cmmgen.ml b/asmcomp/cmmgen.ml
index 712cbaaf9e..ec8fe8a2eb 100644
--- a/asmcomp/cmmgen.ml
+++ b/asmcomp/cmmgen.ml
@@ -940,7 +940,15 @@ let rec expr_size env = function
       expr_size env closure
   | Usequence(_exp, exp') ->
       expr_size env exp'
-  | _ -> RHS_nonrec
+  | Uoffset(u, _ofs) ->
+      (* Uoffset is generated to access an element of a shared-closure
+         representation of mutual function recursion; in this case,
+         the 'u' argument is the closure for the whole block,
+         and its size is the correct size to use.
+      *)
+      expr_size env u
+  | other ->
+      fatal_errorf "expr_size %a" Printclambda.clambda other
 
 (* Record application and currying functions *)

Before preparing a Pull Request with these fixes, I would like to see if there is also an easy way to forbid these suspicious declarations already in the frontend, when checking the validity of the recursive definitions.

gasche added a commit to gasche/ocaml that referenced this issue May 19, 2019
…iled recursive functions

The minimal reproduction case provided by @monadius

    let rec h =
      let rec f t = g t
      and g t = t in
      f

is unsafe because a single recursive block of size 3 is created for
(f,g) together (this is the shared-closure representation of
mutually-recursive functions), but the code generating a dummy value
for the value-recursion (let rec h = ...) uses the size computation
for just (f) as a normal function, which gives a smaller size. The
(f,g) closure gets only partially written in (h), and as a result the
memory ends up in an invalid state. (Running a minor GC then calling
(h) segfaults.)

This commit fixes the issue by doing a more faithful size calculation
for recursive function blocks.

However, this fix is only preliminary as the extra "arity" parameter
passed in the bytecode (only used by js_of_ocaml) doesn't really make
sense here (the recursive functions may have distinct arities); should
we take the min of all arities? the max? This should probably be
checked with js_of_ocaml people.
gasche added a commit to gasche/ocaml that referenced this issue May 19, 2019
This new fix rejects the fragile pattern a type-checking time, more
precisely when computing the size approximations for the recursive
value declarations.
stedolan added a commit to stedolan/ocaml that referenced this issue Jun 4, 2019
stedolan added a commit to stedolan/ocaml that referenced this issue Jun 4, 2019
@gasche

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@stedolan had a more complete fix in #8712 which is now merged. This bug should be fixed in the upcoming 4.09 (not 4.08) release.

@XVilka

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

So I guess this can be closed now?

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

Indeed, thanks!

@gasche gasche closed this Jun 19, 2019
ionathanch added a commit to ionathanch/coq that referenced this issue Aug 15, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Aug 16, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Aug 19, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Aug 20, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Aug 20, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Aug 22, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Aug 23, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Aug 26, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Aug 28, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Aug 30, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Sep 25, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Sep 26, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Oct 5, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
ionathanch added a commit to ionathanch/coq that referenced this issue Oct 19, 2019
… fault when running `Definition ss := (Set, Set)`; possibly caused by ocaml/ocaml#8681.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.