Bugfix: stratum: Keep the dupe index valid across a table cleanup

[paulscode]

datum_stratum_add_new_dupe returns a pointer to the entry it has just written, and when that write fills the table it calls datum_stratum_dupes_cleanup before returning. Both things the cleanup can do invalidate the pointer it is about to hand back:

• the prune path qsorts the array, so every entry moves;
• the expand path reallocs the array, so the old one is freed.

datum_stratum_check_for_dupe stores that returned pointer straight into dupes->index[nonce_index] at line 301. So from the first cleanup onwards the bucket index holds pointers to entries that have moved, or into a freed array, and the next share on that nonce follows one.

Reproducing

Build with -fsanitize=address and run datum_gateway --test with the tests in this PR:

ERROR: AddressSanitizer: heap-use-after-free
READ of size 8 ... datum_stratum_check_for_dupe      src/datum_stratum_dupes.c:309
freed by thread T0 here:
  realloc
  datum_stratum_dupes_expand                         src/datum_stratum_dupes.c:138
  datum_stratum_dupes_cleanup                        src/datum_stratum_dupes.c:231
  datum_stratum_add_new_dupe                         src/datum_stratum_dupes.c:274
  datum_stratum_check_for_dupe                       src/datum_stratum_dupes.c:301

Line 309 is if (i->nonce > nonce), reading the entry the bucket points at.

That is the expand path. The prune path frees nothing, so there is no use-after-free to trap on, and it is reached whenever anything in the table can be aged out. It is not harmless: it leaves buckets pointing at slots past current_items, which are handed out again to later entries. In a 256 share test against a small table, 235 of them left the bucket index pointing outside the live entries.

Once a bucket points at a slot that is then reused for a new entry, that entry can be linked to itself, and the walk in datum_stratum_check_for_dupe follows next until it is NULL. The tests check for that and it did not occur in these runs; what is demonstrated is the stale pointer and the reuse, not a hang.

Why it is worth fixing rather than rare

The table is allocated once per stratum thread in datum_stratum_dupes_init and never re-initialised, so nothing clears this short of restarting the gateway. Reaching it is a function of accumulated shares rather than of anything unusual: with the shipped defaults (max_clients_per_thread 128, vardiff_target_shares_min 8, share_stale_seconds 120) the table is 32768 entries per thread. After the first cleanup the corruption is not occasional.

We found this while looking into an operator reporting that local share rejections went from near zero to total after several hours of mining, recurring, and cleared each time only by restarting the gateway. I want to be straight that we have not proven that symptom is this bug: past the corruption the behaviour is undefined, and a spurious duplicate, a missed duplicate and a crash are all reachable from here. The defect itself is deterministic and does not depend on that diagnosis being right.

The fix

Move the capacity check to the top of datum_stratum_check_for_dupe. That is the last point in the call where nothing is holding a pointer into the array or an insertion point in a bucket. Each call inserts at most once — the four datum_stratum_add_new_dupe sites are mutually exclusive and each returns immediately — so checking there is enough to guarantee the insert has room, and the insert now refuses rather than writing past the end if it ever does not.

Two further things in the same file, both small:

datum_stratum_dupes_cleanup's full_wipe branch zeroes the entries and leaves the index pointing at them, which is the reuse case above set up deliberately. Nothing calls it with full_wipe today, which is the only reason it has not bitten.

stratum.max_clients_per_thread is range checked for an upper bound and not a lower one, and the table size is the product of three configured values, so a zero or a negative sizes the table at nothing. Nothing is not a table that merely overflows quickly: the expand grows it by 25%, 25% of zero is zero, and the gateway takes a share it then has nowhere to put. Size it once, floor it, then allocate, so the array and max_items cannot disagree.

Tests

Added to the existing src/datum_stratum_dupes_tests.c. They cover both cleanup paths, because they fail differently, and assert that every pointer reachable from the index names a live entry and that no chain loops. They fail on current master and pass with the fix.

This function predates the 64-bit nonce work and the same defect is present in OCEAN's tree, so the equivalent fix is going to OCEAN-xyz/datum_gateway against 0.3.x.

[jasonsopko]

Tested ACK 6ccfbe5.

What I ran. Three AddressSanitizer builds, all through datum_gateway --test: master b9ea7dc with its own tests passes; master's datum_stratum_dupes.c with only the PR's test file dropped in trips the heap-use-after-free at the read in datum_stratum_check_for_dupe, freed by the realloc in datum_stratum_dupes_expand, the trace in the description; the PR head passes with no sanitizer report. Logs: https://gist.github.com/jasonsopko/55a0b55cc28a88beb369a75aa31e7b4c.

What I read. datum_stratum_add_new_dupe takes &dupes->ptr[current_items], links it, and on the fill runs the cleanup before returning that pointer; the sort moves it, the expand frees it, and the two paths in datum_stratum_check_for_dupe that write the return value into dupes->index[] store it after that. The other two insert sites discard the return value, and datum_stratum_dupes_reorganize rebuilds the buckets from the array, so the damage is the stale head pointer the caller writes back on top of the rebuilt index. The four insert sites are mutually exclusive and each returns, so the one check at the top of check_for_dupe is enough; after a prune current_items is below 95 percent of max_items, after an expand it is the old max_items against the new, so the insert has room either way. The index clear in the full_wipe branch and the floor on the table size read right.

Exposure. With the shipped defaults the table is 32768 entries per stratum thread, and at the vardiff target of eight shares a minute one client reaches the first cleanup in about 68 hours of uptime. A gateway with one machine on it gets there in three days; a gateway with a rack gets there the first day. Every gateway on this chain runs this code.

The description's separation of what is demonstrated from what is suspected is the part I would point other people at.

[luke-jr]

What do you think of OCEAN-xyz#195 ? Does it address this?

[jasonsopko]

No, OCEAN-xyz#195 fixes the sizing and types but not this. add_new_dupe still returns a pointer that the cleanup's realloc/qsort has invalidated.

Master (ac9b70c) with OCEAN-xyz#195's dupes changes fails this PR's tests under ASan (use-after-free at dupes.c:317). Master with this PR merged passes clean. It still merges cleanly, and the two fit together.