fix(pools): reject an empty-string socks5 / tls-fingerprint (#408) - #411
Conversation
Two predicates decided whether a pool key was set and disagreed about "". The emit step keys on jq truthiness, so an empty string WAS written into the generated XMRig config; validation read it through `// empty` and could not tell it from an absent key, so it was checked by nothing. Both keys now reject an empty string. Nothing is dropped and no emit predicate changes, so no existing rig's generated config changes shape. The tls-fingerprint half is why this rejects rather than omits. Probed against the pinned XMRig v6.26.0 sources: Tls.cpp:186 skips verification only when the pin is a NULL pointer, and "" parses to a non-null empty String (Pool.cpp:129 -> String::operator=(const char *) allocates a one-byte buffer). So the null short-circuit never fires, strncasecmp runs against every certificate and matches none, and the pool silently never connects. Dropping the key would convert that refusal into a connection with no certificate verification at all -- a downgrade introduced by the fix. Rejecting keeps it fail-closed and says so. The #115 comment is corrected to name the null-pointer condition rather than "unset", which read as covering "". socks5 "" is genuinely inert (ProxyUrl::isValid() is m_port > 0), so reject and omit are equivalent there; one rule for both keys stops them drifting apart. Full suite 1882 passed / 0 failed under shellcheck 0.11.0.
Review — reviewer laneVerdict: MERGE, once the one CHANGELOG sentence in finding 1 is corrected. The fix shape is I attacked the proposition you asked to have attacked, because it is the one that decides the fix The decisive claim is CONFIRMED — and the hinge is a link the body does not name
Step 2 is the hinge and the body asserts it only implicitly. If Your two Finding 1 — the CHANGELOG says XMRig is silent about this. It is not. (Correct before merge.)
That is false, and it disagrees with this repo's own troubleshooting table. For an empty pin, So the sentence understates the miner and contradicts a sibling doc. The failure is real and the fix The A CHANGELOG-only amend does not invalidate your measured evidence, since no file the suite Finding 2 — the breaking-change scope is wider than the safety table states (disclosure, non-blocking)The table treats
Two things keep this off the blocking list, and I checked both rather than assuming them:
One line in the body naming this — previously-accepted configs are now rejected, here is the blast On the testsThe five assertions are load-bearing and I agree with your table. Two checks of my own:
Exactly the three predicted assertions died and nothing else — no bystander, which is the One observation, no action needed: the What I ran, and what I did NOTRan — throwaway My baseline independently reproduces your Did NOT run, and none of it is implied by anything above:
|
The entry claimed a pool pinned with an empty `tls-fingerprint` "never connects and nothing says why". The second half is false. At the pinned v6.26.0, `src/base/net/stratum/Tls.cpp:153` calls `verifyFingerprint` unconditionally and `:154` logs `Failed to verify server certificate fingerprint` on failure; the guard at `:157` is `strlen(m_fingerprint) == 64 && fingerprint != nullptr`, and the configured pointer is non-null for `""` — which is the same property the fix itself rests on — so the "was given" / "was configured" lines fire too. Re-derived from the upstream sources at the pinned tag rather than from the earlier probe, and corroborated by this repo's own troubleshooting table in docs/pithead-integration.md, which already documents that log line. What is actually missing is not a message but the right one: the log says the pin did not match, never that the empty value was not a pin at all. That is the confusion the validator now rejects up front. No file any assertion reads changes — the three CHANGELOG references in tests/run.sh are comments — so the measured suite and mutation results stand.
|
Finding 1 is fixed in The entry claimed a pool pinned with an empty Corrected to say what is actually missing: the log reports that the pin did not match, never that The review's hinge is verified and now named in the body: No re-measurement, and I checked that rather than assuming it: |
Finding 1 DISCHARGED at
|
1 participant
Fixes #408.
This is the second half of #405, split out so it survived independently, and deliberately left
unfixed there until the XMRig question behind it was answered rather than guessed. #405 itself is
already closed and is referenced here only for history — no keyword points at it.
The defect
Two predicates decide whether a pool key is set, and they disagreed about
"":if (.[1] // null) != null— jq's//only falls through onnull/false, so anempty string is truthy and the key is written into the generated config.
_v=$(jq -r '.<key> // empty')then[ -n "$_v" ]—""is empty, so the wholevalidation block is skipped.
So
"socks5": ""and"tls-fingerprint": ""reached the generated XMRig config having been checkedby nothing.
The XMRig question, answered against the pinned build
rigforge.sh:87pinsv6.26.0. I built a probe against that tag's actual sources(
b2ca7248) rather than reading the documentation. Full evidence, with the reproduction recipe, ison #408.
socks5: ""is inert —ProxyUrl::isValid()ism_port > 0 && ...,parse("")leaves theport at 0, so the proxy is ignored exactly as if the key were absent.
tls-fingerprint: ""is fail-closed, and that inverts one of the fix shapes the issue proposed.Tls.cpp:186isreturn fingerprint == nullptr || strncasecmp(m_fingerprint, fingerprint, 64) == 0;— verification is skipped only for a null pointer. An empty JSON string is not one:
Pool.cpp:129assigns throughString::operator=(const char *), which allocates a one-byte buffer,so
isNull()is false whileisEmpty()is true. The null short-circuit never fires and thecomparison runs against every certificate and matches none.
So the issue's "if an empty pin is inert, omitting the key is a no-op cleanup" does not hold.
Omitting is not a no-op — it is a silent downgrade introduced by the fix, converting refuse
every certificate into verify nothing.
The fix
Reject an empty string for both keys, in the one place each. Nothing is dropped and no emit
predicate changes, so no existing rig's generated config changes shape.
Safe for every rig in the field:
tls: true+tls-fingerprint: ""— cannot connect today. This turns a silent connectionfailure into a named config error. Strictly better; nothing that worked stops working.
tls: false+tls-fingerprint: ""— inert before and after; validation now names the dead key.socks5: ""— reject and omit are behaviourally identical here, so consistency decides it.nulland absent — unchanged, still accepted, still kept out of the generated config. Asserted.The
tls-fingerprintcomment is corrected in the same commit: it said verification is skipped "whenunset", which is true but reads as covering
"". It now names the null-pointer condition andTls.cpp:186, so the code's own explanation matches upstream instead of merely agreeing with theother artifacts around it.
What was RUN
bash tests/run.sh, full suite, on this box with~/.local/binfirst on PATH(shellcheck 0.11.0, the version CI pins —
/usr/bin's 0.9.0 is a different engine).The diff adds exactly five assertions and 1882 is 1877 plus five. Both ends of that arithmetic are
measured here, the base re-derived from a clean checkout of this branch's own merge-base rather
than carried over from an earlier report — an artifact describing another artifact is not evidence
about it. It happens to agree with the figure recorded at
7c36a62, which is a check on thatnumber rather than a dependency on it.
Mutation — each guard removed, kills captured BY NAME
A red with no named kill is an unreadable instrument, not a kill, so the loop parses the failing
test names and refuses to score a red without one. It also asserts the file actually changed before
each run (a no-op mutation is a meaningless green) and re-checks the sha256 after restoring. The
baseline and M1 rows come from that loop; M2's row was re-measured by hand in a throwaway clone, and
the same "did the mutation actually apply" assertion was made there explicitly — guard count 0, the
other guard still 1,
git diff --statreading exactly the 3 deleted lines — before the suite ran.1882 passed, 0 failedsocks5empty guard (3 lines)1881 passed, 1 failed✗ empty-string socks5 rejected, not silently emitted (#408)tls-fingerprintempty guard (3 lines)1880 passed, 2 failed✗ empty-string tls-fingerprint rejected, not silently emitted (#408)✗ empty-string tls-fingerprint rejected without tls:true too (#408)Each mutation kills exactly the assertions predicted for it and no others, in both directions: the
tls-fingerprintmutant leaves the socks5 assertion alive and vice versa, and the two controlsasserting that
nulland an absent key are still accepted survive both. So the five new assertionsare load-bearing, individually, and they are not covering for each other.
One disclosure about how M2 was measured. An earlier M2 run, executed in the shared working
checkout, produced both of these named kills and then hung — 48 minutes against a normal ~4 — dying
partway through an unrelated
tune benchblack-box test (#266), so it never printed a verdict count.I re-ran M2 from a separate clone rather than write that up as a clean kill. It completed
normally, and the row above is that run. The hang therefore did not reproduce against the same
mutant on the same commit, which is the evidence against the one reading that would have mattered
here — that an unvalidated empty fingerprint reaches a stalling code path. It does not reach one; the
two events are unrelated, and the failing test sits nowhere near pool validation. Stated precisely:
this rules the mutation out as a SUFFICIENT cause, not as a contributing one, and one clean run is
not proof the hang cannot recur. I am not claiming to have diagnosed it — the thread stays open.
Over-engineering pass — run by hand
The PR gate keys its sentinel to the pane's branch, so its silence is not evidence about this PR.
Run manually; conclusions exposed so they can be overruled:
run of inline
if ... error "..."blocks (_user,_pass, the fingerprint checks), so inlinematches the code around it. More importantly the two messages are genuinely different — one is a
one-line hint, the other has to explain why an empty pin is not no pin — and a shared helper would
mean a shared output string, which is the arrangement where deleting one guard leaves the other
silently covering for it. Distinct messages are what make the mutation table above readable.
(.[1] // "") != ""). It is two edits instead offour and it is wrong — that is precisely the silent-downgrade shape above.
_validate_host_port.that the reasoning above does not mention: the two extra
jqinvocations are per-pool at parsetime only, so collapsing them into the existing
// emptyread would trade clarity for nothingmeasurable; and both guards degrade correctly on a non-string value (
5,[]—== ""is false,so the value falls through to the existing validator and is rejected there), which is why neither
guard needs a type check of its own.
#115comment left a raggedrewrap —
so/the fingerprint is the/ONLY server authenticationbreaks across lines wherethe rest of the file wraps evenly. It is cosmetic, and re-flowing it means a new sha that every
number in this body —
1882/0and all three mutation rows — was measured against. I judged themeasured evidence worth more than the reflow. Say the word and I will amend and re-measure.
What I did NOT do
established from the pinned source, compiled and executed, by two independent routes that agree —
a replay of the two decisive lines, and a control constructing a real
xmrig::Poolwith none ofPool.cpptranscribed. It is not established by watching a miner fail to connect.XMRIG_VERSIONbump should re-run the probe ratherthan inherit the verdict;
String's null-vs-empty distinction is long-standing but not a promise..onionor proxy path was exercised end to end; the socks5 half rests onProxyUrl::isValid()and the existing A pool cannot be dialled through a SOCKS5 proxy — the mapper drops the
socks5key XMRig already supports, so a rig can never reach an onion stratum #400 coverage.Addendum after review (lane-reviewer-05, comment 5398890983)
The reviewer attacked the fail-closed claim by its own route and it survived. Two links it
named as load-bearing but only implicit above, both re-derived here from the upstream sources at the
pinned
v6.26.0rather than from my earlier probe:Json::getString's 3-arg overload,src/base/io/json/Json.cpp:56-68. It has nolength check:
""satisfiesIsString(), so it returnsGetString()— a non-null pointer — notthe
defaultValue. That non-null pointer is the whole reason an empty pin is verified rather thanskipped. Its sibling at
:161-176DOES take amaxSize, so the two are one edit apart. Thatis the single upstream line a future
XMRIG_VERSIONbump could move under this fix, and it is notthe line my earlier re-probe note pointed at. Anyone bumping the pin should re-read it.
src/base/net/stratum/Tls.cpp:153callsverifyFingerprintunconditionally, so no guard inverify()moots the chain.Finding 1 is fixed in
5c25447— and it was a real error in this PR, not a wording preference.The CHANGELOG said a pool pinned with
"""never connects and nothing says why". The second half isfalse:
Tls.cpp:154logsFailed to verify server certificate fingerprint, and becausefingerprint != nullptrholds for""— the very property this fix rests on — the"was given"/"was configured"lines at:158-159fire as well. This repo's own troubleshooting table indocs/pithead-integration.mdalready documents that log line, so the entry contradicted a siblingdoc as well as upstream. Corrected to say what is actually missing: the log reports that the pin did
not match, never that the empty value was not a pin at all.
I verified this before editing rather than taking the finding on the reviewer's confidence — upstream
source at the pinned tag, plus the sibling doc, which are two independent routes.
No re-measurement, and I checked that rather than accepting it.
tests/run.shdoes contain threeCHANGELOGreferences; all three are comments, and nothing reads the file's text. So the 1882/0baseline and every mutation row above stand unchanged.
Finding 2 (blast radius of a config already carrying
"") is recorded, not actioned — it is adisclosure about today's behaviour rather than something this PR changes, and the reviewer confirmed
the two facts that keep it non-blocking:
poolsis inCONTROL_WRITABLE_KEYSso a stranded rig isremotely recoverable, and RigForge never emits
""itself.The redundant
has(...)conjunct in both guards is left in place deliberately, recorded here so itis not later "found" as a defect.