From 63836d8106166ebc15f365d95a6e02828dab87a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= Date: Wed, 11 Sep 2024 09:16:56 +0100 Subject: [PATCH] Remove Lazy from native and bytecode modes: not thread safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Angstrom (and as a consequence Uri.of_string) is not thread-safe. On OCaml 4 this actually leads to a SIGSEGV sometimes, on OCaml 5 it raises `CamlinternalLazy.Undefined`. This could be fixed by adding a `Lazy.force p |> ignore` so that the Lazy value is never observable from a concurrent/parallel context, but I prefer to remove Lazy completely, at least until the reason for the OCaml 4 segfault is understood. Using a ref here is safe, because the ref is only changed once before 'fix_direct' returns. And it still preserves the optimization that Lazy did: the fixpoint is only computed once. While the fixpoint is being calculated the ref is set to a closure that raises an exception. This is compatible with the previous behaviour, where `f` calling its argument in a non-delayed way would've raised `CamlinternalLazy.Undefined`. When used correctly the changing `ref` is not observable from concurrent/parallel context, and even if it would be it wouldn't lead to the program crashing. This avoids the crash noticed in https://github.com/mirage/ocaml-uri/issues/178 Use @gasche's suggestion to avoid an option by storing a function that would raise initially Signed-off-by: Edwin Török --- lib/angstrom.ml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/angstrom.ml b/lib/angstrom.ml index e546fbc..da86ecf 100644 --- a/lib/angstrom.ml +++ b/lib/angstrom.ml @@ -454,11 +454,14 @@ let take_till f = let choice ?(failure_msg="no more choices") ps = List.fold_right (<|>) ps (fail failure_msg) +let notset = { run = fun _buf _pos _more _fail _succ -> failwith "Angstrom.fix_direct not set" } + let fix_direct f = - let rec p = lazy (f r) + let rec p = ref notset and r = { run = fun buf pos more fail succ -> - (Lazy.force p).run buf pos more fail succ } + (!p).run buf pos more fail succ } in + p := f r; r let fix_lazy ~max_steps f =