Skip to content

Multiversion: Fix upgrade/downgrade TOCTOU#2758

Merged
sentientwaffle merged 2 commits into
mainfrom
dj-multiversion-race
Feb 20, 2025
Merged

Multiversion: Fix upgrade/downgrade TOCTOU#2758
sentientwaffle merged 2 commits into
mainfrom
dj-multiversion-race

Conversation

@sentientwaffle

Copy link
Copy Markdown
Member

Bug

Previously we asserted the invariant: if a replica is downgrading, then it must be on the newest release in the binary.
But this is only kind-of true!

Scenario:

  1. Replica starts on release A.
  2. Replica detects that its binary has been replaced by B.
    It reads the binary of B into a memfd.
  3. Replica decides to upgrade to B, so it exec()'s the memfd.
  4. (Swap B's binary on disk with C.)
  5. Replica starts up, running B's binary.
  6. During open, replica reads the binary's header from disk.
    But that's C's binary/header, so B is unexpectedly not the latest release in it.

This is a tricky bug! It manifests with the following stack trace:

info(replica): 0: release_transition: release=0.16.16..0.16.17 (reason=commit_dispatch)
info(multiversioning): executing current release 0.16.18 (target: 0.16.17) via /opt/tigerbeetle/tigerbeetle...
2025-02-14 14:48:33.574Z info(io): opening "data"...
2025-02-14 14:48:34.053Z info(main): release=0.16.18
2025-02-14 14:48:34.053Z info(main): release_client_min=0.15.3
2025-02-14 14:48:34.053Z info(main): releases_bundled={ 0.16.14, 0.16.16, 0.16.17, 0.16.18, 0.16.19 }
2025-02-14 14:48:34.053Z info(main): git_commit=ac1c58b344c3cc04fbb4211e126553a7c7db1c4f
2025-02-14 14:48:34.866Z info(replica): superblock release=0.16.17
thread 1556 panic: reached unreachable code
tigerbeetle/zig/lib/std/debug.zig:412:14: 0x12d0f7d in assert (tigerbeetle)
tigerbeetle/src/vsr/replica.zig:9335:23: 0x131930f in release_transition (tigerbeetle)
tigerbeetle/src/vsr/replica.zig:629:40: 0x1317e0d in open (tigerbeetle)
tigerbeetle/src/tigerbeetle/main.zig:362:21: 0x131dc85 in start (tigerbeetle)
tigerbeetle/src/tigerbeetle/main.zig:79:30: 0x13b20f1 in main (tigerbeetle)
tigerbeetle/zig/lib/std/start.zig:524:37: 0x12d0d45 in posixCallMainAndExit (tigerbeetle)
tigerbeetle/zig/lib/std/start.zig:266:5: 0x12d0861 in _start (tigerbeetle)
???:?:?: 0x4 in ??? (???)
Unwind information for `???:0x4` was not available, trace may be incomplete

...Which is misleading -- it looks like we have somehow transitioned directly from 0.16.16 to 0.16.18 within 0.16.19's binary, which shouldn't be possible. And it isn't! We are in 0.16.18's binary (via memfd) but when 0.16.18 reads the binary's multiversion header it gets 0.16.19's header.

Fix

Don't assert the incorrect invariant! If we detect this race, log a warning.


Also in this PR:

  • Fix multiversion logging -- there were some places we were logging releases as integers instead of triples.
  • Add a couple missing asserts.

Ensure release numbers are displayed as release triples, not just integers.
Previously we asserted the invariant: _if a replica is downgrading, then it must be on the newest release in the binary_.
But this is only kind-of true!

Scenario:
1. Replica starts on release A.
2. Replica detects that its binary has been replaced by B.
   It reads the binary of B into a memfd.
3. Replica decides to upgrade to B, so it exec()'s the memfd.
4. (Swap B's binary on disk with C.)
5. Replica starts up, running B's binary.
6. During open, replica reads the binary's header from disk.
But that's C's binary/header, so B is unexpectedly not the latest release in it.

This is a tricky bug! It manifests with the following stack trace:
```
info(replica): 0: release_transition: release=0.16.16..0.16.17 (reason=commit_dispatch)
info(multiversioning): executing current release 0.16.18 (target: 0.16.17) via /opt/tigerbeetle/tigerbeetle...
2025-02-14 14:48:33.574Z info(io): opening "data"...
2025-02-14 14:48:34.053Z info(main): release=0.16.18
2025-02-14 14:48:34.053Z info(main): release_client_min=0.15.3
2025-02-14 14:48:34.053Z info(main): releases_bundled={ 0.16.14, 0.16.16, 0.16.17, 0.16.18, 0.16.19 }
2025-02-14 14:48:34.053Z info(main): git_commit=ac1c58b344c3cc04fbb4211e126553a7c7db1c4f
2025-02-14 14:48:34.866Z info(replica): superblock release=0.16.17
thread 1556 panic: reached unreachable code
tigerbeetle/zig/lib/std/debug.zig:412:14: 0x12d0f7d in assert (tigerbeetle)
tigerbeetle/src/vsr/replica.zig:9335:23: 0x131930f in release_transition (tigerbeetle)
tigerbeetle/src/vsr/replica.zig:629:40: 0x1317e0d in open (tigerbeetle)
tigerbeetle/src/tigerbeetle/main.zig:362:21: 0x131dc85 in start (tigerbeetle)
tigerbeetle/src/tigerbeetle/main.zig:79:30: 0x13b20f1 in main (tigerbeetle)
tigerbeetle/zig/lib/std/start.zig:524:37: 0x12d0d45 in posixCallMainAndExit (tigerbeetle)
tigerbeetle/zig/lib/std/start.zig:266:5: 0x12d0861 in _start (tigerbeetle)
???:?:?: 0x4 in ??? (???)
Unwind information for `???:0x4` was not available, trace may be incomplete
```

...Which is misleading -- it _looks_ like we have somehow transitioned directly from `0.16.16` to `0.16.18` within `0.16.19`'s binary, which shouldn't be possible. And it isn't! We are in `0.16.18`'s binary (via memfd) but when `0.16.18` reads the binary's multiversion header it gets `0.16.19`'s header.
@sentientwaffle sentientwaffle marked this pull request as ready for review February 20, 2025 22:09
@sentientwaffle

Copy link
Copy Markdown
Member Author

A couple more notes:

  • This fix is not retroactive and is not backported -- upgrades up until the cluster reaches the release with this fix could still hit this bug.
  • If a replica does hit this bug, starting it manually is all it takes to fix.

@matklad matklad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @fabioarnold that's another thing that vörtex upgrade test should be able to catch

@sentientwaffle sentientwaffle added this pull request to the merge queue Feb 20, 2025
Merged via the queue into main with commit c5bd4a3 Feb 20, 2025
@sentientwaffle sentientwaffle deleted the dj-multiversion-race branch February 20, 2025 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants