irmin-pack: Fix data race in control file reads #2100

Ngoguey42 · 2022-10-03T16:45:36Z

No description provided.

Ngoguey42 · 2022-10-03T16:46:48Z

src/irmin-pack/unix/control_file.ml

-    let* len = Io.read_size io in
-    let len = Int63.to_int len in
-    let* string = Io.read_to_string io ~off:Int63.zero ~len in


If the RW instance updates the file between the read_size and the read_to_string we would have a problem

samoht · 2022-10-03T17:17:40Z

src/irmin-pack/unix/io.ml

+    let len = page_size in
+    let bytes = Bytes.create len in
+    let rec aux ~off =
+      let nread = Util.really_read t.fd off len bytes in


You still have a data race here if the RW instance updates the file between two short reads, don't you?

Two thoughts:

I don't think a loop is needed here since Util.really_read also loops until len is read. But --

Regardless of location of the loop, it does seem to me that a race can happen if you need multiple reads. What is the best (or even a good) way to protect against this? The simplest I can think of is to check last updated timestamp before each read and start over if it changes, but maybe there is some other trick?

Here is how my patch functions:

I've added a read_all_to_string function that just reads a file. It doesn't guarantee that the full content of the file is read atomically. It only guarantees that pages are read atomically (i.e. a page never spans on two iterations of the loop)

I've changed the read function in the control file to rely on this new read_all_to_string function. Since control files are always contained within a page, that read is atomic. The assertion ensures so.

Following your reviews I've changed my code in the following manner:

In io_intf.ml I've detailed the guarantees offered by read_all_to_string

In read_all_to_string I am now directly using Syscalls.pread in order to make it more obvious that the pages are read atomically. The loop in really_read only exists for situations where len is greater than OCaml's UNIX_BUFFER_SIZE (https://github.com/ocaml/ocaml/blob/trunk/otherlibs/unix/unixsupport.h#L107, https://github.com/mirage/index/blob/main/src/unix/pread.c#L18)

In control_file.ml I've documented and improved the handling of corrupted control files

Since control files are always contained within a page, that read is atomic

If you assume an underlyingext2 filesystem, that's probably mostly true (the kernel can still decide to interrupt file-system reads). If you run this under NFS (and other network-based filesystems), that's unfortunately not true.

In that case we will need to add a checksum in the control file (cc @art-w). At least this PR fixes the issue for some file systems.

yea it's great to improve the current situation - it's also nice to be clear about the limitations.

Are the RO instance re-reading the full files on every "sync"?

The only way to get atomic writes in POSIX is via rename. I don't have enough context to know if this could work in this case but we are doing this in various other places of irmin, for instance in ocaml-git when we update references: https://github.com/mirage/ocaml-git/blob/master/src/git-unix/git_unix.ml#L529-L547

Are the RO instance re-reading the full files on every "sync"?

Yes.

The only way to get atomic writes in POSIX is via rename.

I see, thanks

Could you update the comments in the code to reflect that reads might not be atomic - this can be misleading for critical pieces of code

Done in #2107

codecov-commenter · 2022-10-03T17:21:53Z

Codecov Report

Merging #2100 (2b68dc9) into main (4936dec) will increase coverage by 0.05%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main    #2100      +/-   ##
==========================================
+ Coverage   64.63%   64.69%   +0.05%     
==========================================
  Files         132      132              
  Lines       15697    15726      +29     
==========================================
+ Hits        10146    10174      +28     
- Misses       5551     5552       +1

Impacted Files	Coverage Δ
src/irmin-pack/unix/control_file.ml	`86.48% <66.66%> (+1.12%)`	⬆️
src/irmin-pack/unix/io.ml	`65.71% <72.72%> (+0.82%)`	⬆️
src/irmin-fs/irmin_fs.ml	`81.21% <0.00%> (-1.22%)`	⬇️
src/irmin-pack/unix/file_manager.ml	`84.06% <0.00%> (-0.69%)`	⬇️
src/irmin-fs/unix/irmin_fs_unix.ml	`67.74% <0.00%> (-0.65%)`	⬇️
src/irmin-pack/unix/ext.ml	`61.69% <0.00%> (-0.25%)`	⬇️
src/irmin-pack/unix/async.ml	`53.84% <0.00%> (-0.16%)`	⬇️
src/irmin-pack/unix/errors.ml	`28.88% <0.00%> (ø)`
src/irmin-graphql/server.ml	`62.03% <0.00%> (+0.35%)`	⬆️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

icristescu · 2022-10-04T12:06:58Z

src/irmin-pack/unix/io.ml

+    in
+    try
+      aux ~off:Int63.zero;
+      Ok (Buffer.contents buf)


maybe, just to be very clear, add a comment about really_read and the unix_buffer_size (the function only loops for reading buffers with length greater than the page size, hence atomicity guaranteed for one page).

icristescu · 2022-10-04T13:18:00Z

Thanks!

@patricoferris

…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.4.2) CHANGES: ### Added - **irmin** - Add `test_set_and_get*` functions to retrieve commit associated with an update to the store (mirage/irmin#2075, @patricoferris) - **irmin-graphql** - Expose `test_set_and_get` function as a new mutation (mirage/irmin#2075, @patricoferris) - Add `contents_hash` function to get a value's hash (mirage/irmin#2099, @patricoferris) - **irmin-pack** - Expose `Gc.cancel` to abort a running GC (mirage/irmin#2101, @art-w) - **irmin-tezos-utils** - Add package `irmin-tezos-utils` containing a graphical tool for manual pack files analysis. (mirage/irmin#1939, @clecat) ### Changed - **irmin-pack** - `irmin_pack_mem` no longer exposes disk specifics functions (mirage/irmin#2081, @icristescu) - Move unix specific details for `Pack_key` and `Pack_value` from `irmin-pack` to `irmin-pack.unix` (mirage/irmin#2084, @metanivek) - Remove unnecessary files at `open_rw` and after a failed GC (mirage/irmin#2095, @art-w) ### Fixed - **irmin-pack** - Fix data race in RO instances when reading control file (mirage/irmin#2100, @Ngoguey42) - Fix bugs in gc related to commits that share the same tree. (mirage/irmin#2106, @icristescu) ### Fixed - **irmin-pack** - Fix the traverse pack files commands in the `irmin-tezos` CLI to work with gced stores. (mirage/irmin#1919, @icristescu)

@patricoferris

…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.4.2) CHANGES: ### Added - **irmin** - Add `test_set_and_get*` functions to retrieve commit associated with an update to the store (mirage/irmin#2075, @patricoferris) - **irmin-graphql** - Expose `test_set_and_get` function as a new mutation (mirage/irmin#2075, @patricoferris) - Add `contents_hash` function to get a value's hash (mirage/irmin#2099, @patricoferris) - **irmin-pack** - Expose `Gc.cancel` to abort a running GC (mirage/irmin#2101, @art-w) - **irmin-tezos-utils** - Add package `irmin-tezos-utils` containing a graphical tool for manual pack files analysis. (mirage/irmin#1939, @clecat) ### Changed - **irmin-pack** - `irmin_pack_mem` no longer exposes disk specifics functions (mirage/irmin#2081, @icristescu) - Move unix specific details for `Pack_key` and `Pack_value` from `irmin-pack` to `irmin-pack.unix` (mirage/irmin#2084, @metanivek) - Remove unnecessary files at `open_rw` and after a failed GC (mirage/irmin#2095, @art-w) ### Fixed - **irmin-pack** - Fix data race in RO instances when reading control file (mirage/irmin#2100, @Ngoguey42) - Fix bugs in gc related to commits that share the same tree. (mirage/irmin#2106, @icristescu) - Fix the traverse pack files commands in the `irmin-tezos` CLI to work with gced stores. (mirage/irmin#1919, @icristescu)

@patricoferris

…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.4.2) CHANGES: ### Added - **irmin** - Add `test_set_and_get*` functions to retrieve commit associated with an update to the store (mirage/irmin#2075, @patricoferris) - **irmin-graphql** - Expose `test_set_and_get` function as a new mutation (mirage/irmin#2075, @patricoferris) - Add `contents_hash` function to get a value's hash (mirage/irmin#2099, @patricoferris) - **irmin-pack** - Expose `Gc.cancel` to abort a running GC (mirage/irmin#2101, @art-w) - **irmin-tezos-utils** - Add package `irmin-tezos-utils` containing a graphical tool for manual pack files analysis. (mirage/irmin#1939, @clecat) ### Changed - **irmin-pack** - `irmin_pack_mem` no longer exposes disk specifics functions (mirage/irmin#2081, @icristescu) - Move unix specific details for `Pack_key` and `Pack_value` from `irmin-pack` to `irmin-pack.unix` (mirage/irmin#2084, @metanivek) - Remove unnecessary files at `open_rw` and after a failed GC (mirage/irmin#2095, @art-w) ### Fixed - **irmin-pack** - Fix data race in RO instances when reading control file (mirage/irmin#2100, @Ngoguey42) - Fix bugs in gc related to commits that share the same tree. (mirage/irmin#2106, @icristescu) - Fix the traverse pack files commands in the `irmin-tezos` CLI to work with gced stores. (mirage/irmin#1919, @icristescu)

@patricoferris

…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.4.2) CHANGES: ### Added - **irmin** - Add `test_set_and_get*` functions to retrieve commit associated with an update to the store (mirage/irmin#2075, @patricoferris) - **irmin-graphql** - Expose `test_set_and_get` function as a new mutation (mirage/irmin#2075, @patricoferris) - Add `contents_hash` function to get a value's hash (mirage/irmin#2099, @patricoferris) - **irmin-pack** - Expose `Gc.cancel` to abort a running GC (mirage/irmin#2101, @art-w) - **irmin-tezos-utils** - Add package `irmin-tezos-utils` containing a graphical tool for manual pack files analysis. (mirage/irmin#1939, @clecat) ### Changed - **irmin-pack** - `irmin_pack_mem` no longer exposes disk specifics functions (mirage/irmin#2081, @icristescu) - Move unix specific details for `Pack_key` and `Pack_value` from `irmin-pack` to `irmin-pack.unix` (mirage/irmin#2084, @metanivek) - Remove unnecessary files at `open_rw` and after a failed GC (mirage/irmin#2095, @art-w) ### Fixed - **irmin-pack** - Fix data race in RO instances when reading control file (mirage/irmin#2100, @Ngoguey42) - Fix bugs in gc related to commits that share the same tree. (mirage/irmin#2106, @icristescu) - Fix the traverse pack files commands in the `irmin-tezos` CLI to work with gced stores. (mirage/irmin#1919, @icristescu)

Ngoguey42 force-pushed the fix-cf-reads branch from b0afd4e to dd7aa5c Compare October 3, 2022 16:46

Ngoguey42 commented Oct 3, 2022

View reviewed changes

irmin-pack: Fix data race in control file reads

fb3c328

Ngoguey42 force-pushed the fix-cf-reads branch from dd7aa5c to fb3c328 Compare October 3, 2022 16:47

samoht reviewed Oct 3, 2022

View reviewed changes

fixup

0fe4fd7

icristescu reviewed Oct 4, 2022

View reviewed changes

fixup

2b68dc9

icristescu merged commit 10d4703 into mirage:main Oct 4, 2022

metanivek mentioned this pull request Oct 6, 2022

[new release] irmin project (3.4.2) ocaml/opam-repository#22219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

irmin-pack: Fix data race in control file reads #2100

irmin-pack: Fix data race in control file reads #2100

Ngoguey42 commented Oct 3, 2022

Ngoguey42 Oct 3, 2022

samoht Oct 3, 2022 •

edited

Loading

metanivek Oct 3, 2022

Ngoguey42 Oct 4, 2022

samoht Oct 4, 2022 •

edited

Loading

Ngoguey42 Oct 4, 2022

samoht Oct 4, 2022 •

edited

Loading

Ngoguey42 Oct 4, 2022

samoht Oct 5, 2022

Ngoguey42 Oct 5, 2022

codecov-commenter commented Oct 3, 2022 •

edited

Loading

icristescu Oct 4, 2022 •

edited

Loading

icristescu commented Oct 4, 2022

irmin-pack: Fix data race in control file reads #2100

irmin-pack: Fix data race in control file reads #2100

Conversation

Ngoguey42 commented Oct 3, 2022

Ngoguey42 Oct 3, 2022

Choose a reason for hiding this comment

samoht Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

metanivek Oct 3, 2022

Choose a reason for hiding this comment

Ngoguey42 Oct 4, 2022

Choose a reason for hiding this comment

samoht Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Ngoguey42 Oct 4, 2022

Choose a reason for hiding this comment

samoht Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Ngoguey42 Oct 4, 2022

Choose a reason for hiding this comment

samoht Oct 5, 2022

Choose a reason for hiding this comment

Ngoguey42 Oct 5, 2022

Choose a reason for hiding this comment

codecov-commenter commented Oct 3, 2022 • edited Loading

Codecov Report

icristescu Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

icristescu commented Oct 4, 2022

samoht Oct 3, 2022 •

edited

Loading

samoht Oct 4, 2022 •

edited

Loading

samoht Oct 4, 2022 •

edited

Loading

codecov-commenter commented Oct 3, 2022 •

edited

Loading

icristescu Oct 4, 2022 •

edited

Loading