HarperFast / HarperFast/harper

Audit-entry previousVersion is written whenever truthy but detected only by a leading 0x42 byte, so an out-of-range value silently misparses every following field

Open
#2,247 0 comments 0 reactions 1 assignee Claimed by @kriszyp View on GitHub
bug
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 2h
Merged PRs (30d)
205

Description

## Summary

In the LMDB audit-entry format, the presence of the 8-byte `previousVersion` field is signalled **only** by its own leading byte being `0x42` (66). The writer emits the field whenever `previousVersion` is truthy. Those two conditions do not agree, so a `previousVersion` whose float64 does not begin with `0x42` produces an entry that is physically 8 bytes longer than the reader believes: the reader skips the field and parses `action`, `nodeId`, `tableId`, `recordId`, and `version` from the wrong offsets.

There is no corruption detector on this path. The misparse surfaces later as a nonsense `recordId`, and — when the misaligned walk lands on a record-body length byte — as `RangeError: The number cannot be converted to a BigInt` out of `ordered-binary`'s number path, which kills outgoing replication for that (peer, database) pair.

## The two sides that disagree

Writer — `resources/auditStore.ts:442-445`:

```ts
if (previousVersion) {
if (previousVersion > 1) ENTRY_DATAVIEW.setFloat64(start, previousVersion);
else ENTRY_HEADER.set(PREVIOUS_TIMESTAMP_PLACEHOLDER, start);
position = start + 9;
}
```

Reader — `resources/auditStore.ts:544-547`:

```ts
if (buffer[decoder.position] == 66) {
// 66 is the first byte in a date double.
previousVersion = decoder.readFloat64();
}
```

The same inference is made in the audit key codec at `resources/auditStore.ts:80`.

`previousVersion > 1` is not the right test, because "leading byte is `0x42`" is a much narrower predicate than "greater than 1".

## Which values are actually representable

A float64 has leading byte `0x42` exactly when its value is in `[2^33, 2^49)` — i.e. `[8589934592, 562949953421312)`. Real millisecond epoch timestamps sit at roughly `2^40.7`, comfortably inside, which is why this works in the common case.

| `previousVersion` | float64 (BE) | leading byte | reader sees |
|---|---|---|---|
| `1787229175163.2493` (ms epoch) | `427a01f28fd7b3fd` | `0x42` | field present — correct |
| `2^33` = `8589934592` | `4200000000000000` | `0x42` | field present — correct |
| `2^49` = `562949953421312` | `4300000000000000` | `0x43` | **skipped — misparse** |
| `2^33 - 1` | `41fffffffff00000` | `0x41` | **skipped — misparse** |
| `2.0` | `4000000000000000` | `0x40` | **skipped — misparse** |
| `1` | `3ff0000000000000` | `0x3f` | **skipped — misparse** |

The `previousVersion > 1` branch admits every value in `(1, 2^33)` and `[2^49, ∞)`. All of them are written and none of them are readable.

## The reachable case today

`PREVIOUS_TIMESTAMP_PLACEHOLDER` (`resources/RecordEncoder.ts:80`) is `Uint8Array([1, 1, 1, 1, 3, 0x40, 0, 0])` — an instructed-write directive the storage layer substitutes at commit time. Two ways it lands outside the representable range:

1. **Substituted with the no-previous-version sentinel.** Observed byte-level on HarperFast/harper-pro#737: the substitution puts `2.0` in the slot, leading byte `0x40`. Every reader then skips a field that is physically present.
2. **Not substituted at all.** The directive's own leading byte is `0x01`, so an unsubstituted placeholder is equally invisible.

harper-pro#737 has the hexdump of a poisoned entry taken straight out of a v4 leader's audit log, and the full chain from there to `close(1008)` and a wedged outbound subscription.

## Scope

- **LMDB audit store: exposed.** `auditStore.put` → `createAuditEntry(auditRecord)` with `start = 0`, and `readAuditEntry` sniffs byte 0. This is the affected path.
- **RocksDB transaction-log store: not exposed.** `resources/RocksTransactionLogStore.ts:105` declares presence with an explicit flag bit, `HAS_PREVIOUS_VERSION = 0x20000000`, in its uint32 header, and reads it back by that flag at `:411`. Presence is stated, not inferred, so the value is unconstrained. **This is the design the audit store should have.**

Worth noting the ambiguity already bites in the other direction elsewhere: the comment at `resources/auditStore.ts:657-659` documents suppressing the metadata-prefix heuristic precisely so "a classic record whose structure-id byte is 66 (0x42)" is not misread as a timestamp. Same root shape — a value byte doing duty as a type tag.

(For the `action` byte specifically the collision is not reachable: the single-byte form is at most `0x3F` — event type in the low nibble plus `HAS_RECORD` `0x10` and `HAS_PARTIAL_RECORD` `0x20` — and the extended form is written as a uint32 or'd with `0xc0000000`, so its leading byte is `≥ 0xC0`. Neither can be 66.)

## Suggested fix

In rough order of preference:

1. **Declare presence explicitly**, the way the RocksDB log store already does — a flag bit rather than a value sniff. Correct, and it removes the constraint on the value entirely. Needs a format-version step and a read path that accepts both shapes.
2. **Enforce the existing invariant on write.** Replace the `previousVersion > 1` guard with the predicate the reader actually implements — the field is emitted only if its float64 leads with `0x42`, i.e. the value is in `[2^33, 2^49)` — and otherwise omit the field entirely rather than writing an unreadable one. Cheap, no format change, and it converts silent misparse into a benign "no previous version". An assertion or warn on the rejected path would surface whoever is minting out-of-range values.
3. Independently: make the placeholder-substitution failure loud rather than silent, so an unsubstituted or sentinel-substituted slot is caught at mint time instead of at read time on a peer.

Option 2 closes the reachable hole and is separable from the format change in option 1.

## Relationship to the harper-pro work

harper-pro#740 ("A base copy no longer returns the requesting peer's own records while this node is being cloned from it") is a **compensating fix** — it removes the redundant reverse base copy that was minting these entries during a v4 → v5 clone. Its own review notes call this out as the underlying hole left open:

> Core's `createAuditEntry`/`readAuditEntry` still carry that second hole, so a v5+LMDB audit store is exposed — a separate hardening PR.

This is that issue. It matters independently of the migration path: any writer that supplies a `previousVersion` outside `[2^33, 2^49)` corrupts its own audit log, and the only symptom is a replication subscription that dies on an undecodable `recordId`.

Related: harper-pro#737 (byte-level root cause and repro), harper#2154 (the earlier bodyless-audit-entry wedge — different field, same "malformed entry wedges replication" failure mode).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.