From a26aabd17bf389e52077b65010ce735656d15241 Mon Sep 17 00:00:00 2001 From: Bernard Ladenthin Date: Fri, 7 Aug 2026 22:44:39 +0200 Subject: [PATCH] docs(VeraCrypt): add TODO.md and surface the open defect in the index crossrepostatus.md listed VeraCrypt but carried no status, so an open, deliberately unfixed defect (the RNG check-then-lock race) was invisible to anyone reading only the index. VeraCrypt/TODO.md is kept in the workspace rather than in the repo, because the fork is used to prepare upstream PRs and these items must not reach upstream. The reason is stated in both files. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P5zDjB2RVZPB7rGAgHBsEn --- VeraCrypt/README.md | 1 + VeraCrypt/TODO.md | 92 +++++++++++++++++++++++++++++++++++++++++++++ crossrepostatus.md | 5 ++- 3 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 VeraCrypt/TODO.md diff --git a/VeraCrypt/README.md b/VeraCrypt/README.md index 47ff029..c0f8de1 100644 --- a/VeraCrypt/README.md +++ b/VeraCrypt/README.md @@ -14,6 +14,7 @@ checked out at [`../VeraCrypt`](../VeraCrypt). | File | Topic | |---|---| +| [`TODO.md`](TODO.md) | **Start here** — what is open, what is blocked on a decision, what is waiting on upstream | | [`upstream-sync.md`](upstream-sync.md) | The fork drifts silently — verify before analysing anything | | [`build-verification.md`](build-verification.md) | Building and compile-checking on Linux (Docker) and Windows (native MSVC) | | [`coverage-measurement.md`](coverage-measurement.md) | Measuring coverage, why `lcov`/`gcovr` lie here, real figures, and which test attempts paid off | diff --git a/VeraCrypt/TODO.md b/VeraCrypt/TODO.md new file mode 100644 index 0000000..12350c6 --- /dev/null +++ b/VeraCrypt/TODO.md @@ -0,0 +1,92 @@ +# VeraCrypt — open items + +Status as of **2026-08-07**, verified against `upstream/master` @ `b48e31f5` (1.26.29). + +> **Why this file lives here and not in the repo.** The other tracked repos keep their +> open work in their own `TODO.md`. VeraCrypt cannot: the fork is used to prepare +> upstream pull requests, and the findings below are a **local record that must not reach +> upstream**. A `TODO.md` inside the working copy would sooner or later be swept into a +> branch. So it stays in the workspace. + +## Open — waiting on upstream maintainers + +Nothing to do on our side; these are submitted and out of our hands. + +| PR | Subject | Since | +|---|---|---| +| [#1842](https://github.com/veracrypt/VeraCrypt/pull/1842) | Document that `CRYPTOPP_ALIGN_DATA` can expand to nothing | 2026-08-01 | +| [#1843](https://github.com/veracrypt/VeraCrypt/pull/1843) | Explicit derived-key alignment in `CreateVolumeHeaderInMemory` | 2026-08-01 | +| [#1844](https://github.com/veracrypt/VeraCrypt/pull/1844) | Verify `CRYPTOPP_ALIGN_DATA` delivers the requested alignment | 2026-08-01 | +| [#1850](https://github.com/veracrypt/VeraCrypt/pull/1850) | 14 test blocks covering the crypto and platform layers | 2026-08-07 | + +**Merge order matters:** #1850 asks for **#1844 to land first** — both touch +`src/Volume/EncryptionTest.{cpp,h}` at different points. `git apply` fails, `git apply -3` +succeeds. The note is already in the #1850 description; no follow-up needed unless a +maintainer merges them the other way round. + +## Open — decided against reporting, fix designed but not applied + +### 1. Check-then-lock race in `RandomNumberGenerator` — the one real defect + +Reproduced on unmodified source; a two-thread driver segfaults. Full analysis, the +standalone reproducer and the three-step fix are in +[`concurrency-findings.md`](concurrency-findings.md#suggested-fix). + +Fix in short — smallest step first: + +1. move the `Running` guard **inside** `ScopeLock` in `AddToPool` and `GetData` +2. make `Running` a `std::atomic` (`volatile` does not help) +3. join the creation thread — this is the root cause; 1 and 2 only make it unreachable + +**Blocked by a decision, not by missing knowledge.** Deliberately not reported upstream. +Applying it would mean opening a production-code PR, which is out of scope for now. + +**If it is ever picked up:** verify with the reproducer (unpatched segfaults, patched must +print `survived N rounds`) *and* re-run `veracrypt --test` — `TestRandomNumberGenerator` +catches a "fix" that deletes the guard instead of moving it. + +### 2. `VolumeCreator` thread is neither joined nor detached + +`Core/VolumeCreator.cpp:436` — the only `Thread::Start` in the tree with no matching +`Join()` or `Detach()`. `Abort()` only sets a flag, `~VolumeCreator` is empty. This is what +makes item 1 constructible. Same decision applies. + +## Open — robustness, no live defect + +| Item | Where | Status | +|---|---|---| +| `VolumeLayout` dereferences `Header` unguarded | `Volume/VolumeLayout.cpp:131, 136, 180, 185` | Not reachable in production (`Volume.cpp:197` sets the header first). Inconsistent with the `NotInitialized` style used elsewhere. | +| `EncryptionMode::ValidateParameters` has no callers | `Volume/EncryptionMode.cpp:59, 65` | Production never validates these parameters. Coverage stuck at 38.71 % and **cannot be raised honestly** — calling it from a test would paint the line green and hide the point. | +| `ValidateState()` compiled out of Release | 13 sites, all `if_debug(...)` | Presumably deliberate. Do not treat as a safety net; do not write tests for it. | + +## Open — coverage gaps that need fixtures + +Both were predicted before writing anything and are not reachable without new +infrastructure: + +- **`Volume/Keyfile.cpp` stops at 47 %** — the security-token and directory-enumeration + branches need a PKCS#11 token or a populated directory. +- **`Volume/VolumeHeader.cpp` barely moves (82 %)** — the remaining part is the + decrypt-attempt loop, which needs a real encrypted volume. + +A fixture volume created once and checked in would unlock both, at the cost of a binary +test asset. Not attempted. + +## Known weakness in the tests we submitted + +Recorded so it is not rediscovered as a surprise — details in +[`test-coverage-work.md`](test-coverage-work.md#weaknesses-that-remain): + +- Breaking the `Running` check in `AddToPool` goes **undetected** by the suite. It is + covered by line coverage but asserted by nothing. Coverage is not detection. +- `TestVolumeHeaderRejection` relies on `catch (PasswordEmpty&)`; a change of exception + type still fails the suite, but the attribution becomes unclear. + +## Done + +- Fork brought up to date — it had drifted 16 months (394 commits). See + [`upstream-sync.md`](upstream-sync.md); **check this before analysing anything.** +- Coverage measurement method established, including why `lcov`/`gcovr` report wrong + numbers here — [`coverage-measurement.md`](coverage-measurement.md). +- 14 test blocks written, each verified by injecting a defect — 16 of 17 injected defects + attributed to the new test itself. diff --git a/crossrepostatus.md b/crossrepostatus.md index 4976fb7..bc6e3d8 100644 --- a/crossrepostatus.md +++ b/crossrepostatus.md @@ -12,6 +12,8 @@ Single-repo open work lives in each repo's own `TODO.md`: - [`../java-llama.cpp/TODO.md`](../java-llama.cpp/TODO.md) - [`../srcmorph/TODO.md`](../srcmorph/TODO.md) - [`../streambuffer/TODO.md`](../streambuffer/TODO.md) +- [`VeraCrypt/TODO.md`](VeraCrypt/TODO.md) — **kept here, not in the repo**: the fork is used to + prepare upstream PRs, and these items are a local record that must not reach upstream. Recurring per-repo audits (mostly cross-repo by nature but living per-repo today) are documented in [`policies/code-quality-todos.md`](policies/code-quality-todos.md). @@ -23,7 +25,8 @@ Repos — **Java tier** (shared Maven/JUnit toolchain, governed by the guides an **Other tracked repos** — no shared Java toolchain, each with its own conventions and its own knowledge folder here: -- **VeraCrypt** (C/C++) = `/home/user/VeraCrypt` — see [`VeraCrypt/`](VeraCrypt/): build and coverage tooling, submitted PRs, local-only findings +- **VeraCrypt** (C/C++) = `/home/user/VeraCrypt` — see [`VeraCrypt/`](VeraCrypt/): build and coverage tooling, submitted PRs, local-only findings. + Status: 4 PRs pending upstream, and **one reproduced defect (RNG check-then-lock race) that is deliberately unfixed and unreported** — fix designed, see [`VeraCrypt/TODO.md`](VeraCrypt/TODO.md) - **llama.cpp** (C/C++) = `/home/user/llama.cpp` — see [`llama.cpp/`](llama.cpp/) - **subprocess.h** (C) = `/home/user/subprocess.h` — see [`subprocess.h/`](subprocess.h/)