Data race in concurrent uses of `Re.exec` on one pre-compiled Re.re

[kit-ty-kate]

Using TSan with OCaml 5.2 on a code that use re.1.11.0, I'm getting the following error:

WARNING: ThreadSanitizer: data race (pid=76587)
  Read of size 8 at 0xffff60737008 by thread T16 (mutexes: write M0):
    #0 camlRe__Core.loop_1012 lib/core.ml:166 (opamMain.exe+0xbe42e4) (BuildId: ae1ab72786b3638d51b987620a76af5acea9e7b6)
    #1 camlRe__Core.match_str_1142 lib/core.ml:304 (opamMain.exe+0xbe57ec) (BuildId: ae1ab72786b3638d51b987620a76af5acea9e7b6)
    #2 camlRe__Core.exec_2101 lib/core.ml:941 (opamMain.exe+0xbec9f8) (BuildId: ae1ab72786b3638d51b987620a76af5acea9e7b6)
    #3 camlOpamUrl.fun_1957 src/core/opamUrl.ml:70 (opamMain.exe+0xb83a88) (BuildId: ae1ab72786b3638d51b987620a76af5acea9e7b6)
    ...

  Previous write of size 8 at 0xffff60737008 by thread T1 (mutexes: write M1):
    #0 caml_modify runtime/memory.c:220 (opamMain.exe+0xe287fc) (BuildId: ae1ab72786b3638d51b987620a76af5acea9e7b6)
    #1 camlRe__Core.validate_991 lib/core.ml:162 (opamMain.exe+0xbe4108) (BuildId: ae1ab72786b3638d51b987620a76af5acea9e7b6)
    #2 camlRe__Core.loop_1012 lib/core.ml:175 (opamMain.exe+0xbe4454) (BuildId: ae1ab72786b3638d51b987620a76af5acea9e7b6)
    #3 camlRe__Core.match_str_1142 lib/core.ml:304 (opamMain.exe+0xbe57ec) (BuildId: ae1ab72786b3638d51b987620a76af5acea9e7b6)
    #4 camlRe__Core.exec_2101 lib/core.ml:941 (opamMain.exe+0xbec9f8) (BuildId: ae1ab72786b3638d51b987620a76af5acea9e7b6)
    #5 camlOpamUrl.fun_1957 src/core/opamUrl.ml:70 (opamMain.exe+0xb83a88) (BuildId: ae1ab72786b3638d51b987620a76af5acea9e7b6)
    ...

The code in opam that uses Re is the following: https://github.com/ocaml/opam/blob/391333d35bcdc8b55df709b876b8bafcf75f3452/src/core/opamUrl.ml#L46-L74 and can be summarized into:

let f =
  let re = Re.compile ... in
  fun str -> Re.exec re str

which then trigger the data race on this piece of mutable state:

ocaml-re/lib/core.ml

Line 162 in 2dd3851

st.next.(color) <- st'

ocaml-re/lib/core.ml

Line 166 in 2dd3851

let st' = st.next.(Char.code info.colors.[Char.code s.[pos]]) in

[OlivierNicole]

Cc @vouillon in case you didn’t get notified and you find this of interest. Looking at this from 20,000 feet, it looks like Re has been designed to be safe for concurrent use with systhreads, but is not currently domain-safe.

[vouillon]

ocaml-re is not thread-safe at all.

[OlivierNicole]

I see. So making it thread-safe would at best take some hours of work.

A possible temporary workaround, obviously unsatisfactory on many levels, would be to sequence all calls into the library by protecting them with a global mutex.

[kit-ty-kate]

ocaml-re is not thread-safe at all.

Is the whole library thread-unsafe or is it only specific functions like exec here? My assumption from what I'm seeing so far seems to be that any function processing Re.re is thread-unsafe but once you have Re.t it should be fine to use.

[rgrinberg]

Constructing Re.t should be thread-safe. The resulting Re.re should only be used by one thread at a time.

[edwintorok]

ocaml-re is not thread-safe at all.

This is news to me, I thought only Re.Str was not thread safe, and the rest of Re was.
I attempted to document that here: #194
(that is the reason we moved from the compiler's stdlib Str, to Re.Posix, because Re.Posix was believed to be thread-safe).

If the entire 're' is not thread safe currently, it would be good to document this so that users of the library can add appropriate mutexes. (Although we've been using re in production for years and haven't noticed issues with it on OCaml 4).

Although it looks like ocaml-re is definitely not domain-safe on OCaml 5.
See mirage/ocaml-uri#178 (comment) and mirage/ocaml-uri#178 (comment) which reproduces corruption in the match positions. Although Re.exec is called from 2 different threads and the resulting value is only used by a single thread, the match positions still somehow end up sharing state across threads.

[vouillon]

Maybe we could add a way to fully compile a regular expression immediately, rather than lazily. Then, it would be thread-safe to match against this regular expression.

[rgrinberg]

@kit-ty-kate could you confirm that this is fixed in the master branch? I think #574 should do it.

[kit-ty-kate]

I'm not sure this is entirely fixed. The following test-case still lights-up like a christmas tree with TSan enabled:

let split_url =
  let re =
    Re.(compile @@ whole_string @@ seq [
        (* Parse the scheme, which is either backend+protocol or just a protocol *)
        opt @@ seq [
          (* Backend *)
          opt @@ seq [ group @@ rep @@ diff any (set "+:");
                       alt [ char '+'; str "://"] ];
          (* Protocol *)
          group @@ rep @@ diff any (char ':');
          (* Separator *)
          str "://"
        ];
        (* Parse the path, with is either path or path.suffix (suffix contains no .) *)
        group @@ seq [
          non_greedy @@ rep @@ diff any (char '#');
          (* If there's a .suffix, group it separately (used for backend guessing) *)
          opt @@ seq [ char '.'; group @@ rep1 @@ diff any (set "\\/.#")]
        ];
        (* Parse the fragment (git branch, etc.) *)
        opt @@ seq [ char '#'; group @@ rep any ];
      ])
  in
  fun u ->
    match Re.Group.all (Re.exec re u) with
    | [| _; vc; transport; path; suffix; hash |] ->
      vc, transport, path, suffix, hash
    | _ -> assert false

let () =
  let argv = Sys.argv in
  let rec loop acc i =
    if i < Array.length argv then
      loop (
        Domain.spawn (fun () ->
          let vc, transp, path, suffix, hash = split_url argv.(i) in
          print_endline vc;
          print_endline transp;
          print_endline path;
          print_endline suffix;
          print_endline hash;
        ) :: acc) (i + 1)
    else
      acc
  in
  let ths = loop [] 1 in
  List.iter Domain.join ths

Can anyone see something i've missed in this test-case?

[vouillon]

For performance reasons, I'm using double-checked locking. This is safe with the OCaml memory model, but results in benign data races which are reported by TSan.

[kit-ty-kate]

@OlivierNicole is there a known way to drive TSan to ignore such safe data-races?

[OlivierNicole]

@kit-ty-kate Reports on a particular race can be silenced using an environment variable.

Ideally in the future, we will have dedicated types or functions in the Stdlib for this kind of intentional data races.

[kit-ty-kate]

Thanks! I'm able to run the above example without any TSan warnings with the following suppression file (Linux/arm64/OCaml 5.4.0~beta1):

race_top:^camlRe__Compile.loop_1389$
race_top:^camlRe__Compile.final_1410$
race_top:^camlRe__Mark_infos.iteri_479$

If anyone is able to confirm these functions are indeed non-benign data race free, then i'm happy to close this as fixed