search

ocaml / ocaml-re

Pure OCaml regular expressions, with support for Perl and POSIX-style strings
Other
232 stars 60 forks source link

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

Open kit-ty-kate opened 5 months ago

kit-ty-kate commented 5 months ago

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: https://github.com/ocaml/ocaml-re/blob/2dd38515c76c40299596d39f18d9b9a20f00d788/lib/core.ml#L162 https://github.com/ocaml/ocaml-re/blob/2dd38515c76c40299596d39f18d9b9a20f00d788/lib/core.ml#L166

OlivierNicole commented 5 months ago

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 commented 5 months ago

ocaml-re is not thread-safe at all.

OlivierNicole commented 5 months ago

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 commented 5 months ago

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 commented 2 months ago

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

edwintorok commented 2 months ago

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: https://github.com/ocaml/ocaml-re/issues/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 https://github.com/mirage/ocaml-uri/issues/178#issuecomment-2341462349 and https://github.com/mirage/ocaml-uri/issues/178#issuecomment-2353292718 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 commented 2 months ago

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.