Act on the code review: canary guard, VERIFY_UPLOAD, download size, log tail - #7
Merged
Conversation
The guard printed "Refusing to overwrite it" and then the very next check
overwrote the file and the cleanup check deleted it. It recorded a FAIL in
the report and nothing acted on it, so on a box holding a leftover
ZZCHECK.NC - or a real program that happened to use that name - the script
destroyed the file it had just announced it would protect.
It failed open in a second way too: state.get("entries", []) made an
UNAVAILABLE directory listing indistinguishable from an empty box, so when
the listing check failed the guard passed without having checked anything
and the writes went ahead blind. That is the exact firmware behaviour this
script exists to detect, so the two failures would have arrived together.
Both are now one gate: no listing or a taken name means the write phase is
skipped and recorded as such.
Evidence: two tests, one asserting the pre-existing file still holds its
original bytes after a full run, one asserting nothing at all is written
when the listing failed. The first of those fails against the previous
code, which is what makes it worth having.
build() called factory() with no arguments, so DncBoxTransport always took its verify=True default and config.VERIFY_UPLOAD reached nothing but dnc_webdav.py, which passes it explicitly. That is the setting the README and issue #3 name as the escape hatch if the acceptance run comes back saying the firmware normalises line endings. In that situation every good program fails verification, the operator sets DNCKIT_VERIFY_UPLOAD=false exactly as documented, restarts the panel, and nothing changes - with no error to explain why, because the value was read and then discarded. Transports now build through a from_config classmethod, so a transport with settings can say so in one place instead of every caller remembering. Evidence: parametrised test asserting build("dnc-box").verify follows config.VERIFY_UPLOAD in both directions.
End of file was inferred only from a short block, so a program whose size is an exact multiple of 512 always cost one request BEYOND the end. If the firmware answers that with silence rather than an empty block, a complete and correct file - already fully assembled in memory - is thrown away as a DncTimeout. The previous implementation returned it, because it treated silence as end of file; tightening that into an error is what created the exposure. Silence cannot distinguish "the file ended" from "the packet was lost", so the sound fix is not to guess: download() now takes the size when the caller knows it. list_dir() reports it, download_all() passes it through, and verify_remote() passes the length of the data it just sent - which is the case that matters most, since the read-back is exactly where a 512-byte program gets exercised. Worth recording: the first version of this replaced the short-block rule instead of adding to it, and a device holding FEWER bytes than expected then looped for ever asking for blocks that do not exist. That is precisely what a normalising firmware looks like, and the existing acceptance-script test caught it as a hang. Both rules are needed - the size stops us early, the short block still ends the file. Evidence: 5 tests, including one that pins the hazardous no-size path so the reason for the parameter stays documented, and one for the fewer-bytes case that hung. 124 tests pass in 3.2s. Not validated: whether the box is actually silent past the end is Q3 of the acceptance run - this makes the answer stop mattering for any caller that knows the size.
tail() parsed the entire CSV to return twelve rows, and the panel renders it on every load while reloading itself every 20 seconds. A Pi sitting next to a machine for a year at ~50 transfers a day reaches roughly 20k rows, and every one of them was read and parsed several times a minute, under the lock, to show the last twelve. Nothing rotates the file, so that only ever grows. It now seeks to the last 64 KB. Safe to slice by lines because record() already flattens newlines out of every field, so no entry spans more than one line; the first line of the window is dropped when the file is larger than the window, since it is probably cut in half. Evidence: test writing 400 entries against a 2 KB window and asserting the five most recent come back correct and in order, plus one for the small-file path where the header is inside the window and must not be returned as a row.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A review of everything merged in #1 and #2 turned up four real defects. Two of
them are in the safety machinery added by those PRs, which is the worst place
for them.
The canary guard did not guard anything
hardware_check.pyprinted "Refusing to overwrite it" and then the nextcheck overwrote the file and the cleanup check deleted it. The guard recorded a
FAIL in the report and nothing acted on it.
It failed open a second way:
state.get("entries", [])made an unavailabledirectory listing look identical to an empty box, so when the listing check
failed the guard passed without checking anything. That is the exact firmware
behaviour the script exists to detect, so both failures would have arrived
together — on a script that runs standing next to a machine.
VERIFY_UPLOAD was ignored everywhere except dnc_webdav
build()calledfactory()with no arguments, soDncBoxTransportalways usedverify=True. If the acceptance run answers Q1 with "the firmware normalisesline endings", every good program fails verification, the operator sets
DNCKIT_VERIFY_UPLOAD=falseexactly as #3 documents, restarts — and nothingchanges, with no error to explain why.
A 512-multiple download asked for a block past the end
End of file was inferred only from a short block, so a program that is an exact
multiple of 512 always cost one request beyond it. A firmware that answers that
with silence turns a complete, correct file into a
DncTimeout. The old codereturned it; tightening silence into an error in #1 is what created the exposure.
Silence cannot distinguish "ended" from "lost", so
download()now takes thesize when the caller knows it —
list_dir()reports it, andverify_remote()passes the length it just sent, which is where a 512-byte program actually gets
exercised.
Worth recording: the first attempt at this replaced the short-block rule
instead of adding to it, and a device holding fewer bytes than expected then
looped for ever. That is precisely what a normalising firmware looks like, and
the existing acceptance-script test caught it as a hang.
The panel re-parsed the whole log every 20 seconds
A year at ~50 transfers a day is ~20k rows, all read and parsed several times a
minute, under the lock, to show twelve. Now it seeks the last 64 KB.
Verification
124 tests, up from 114. The two that matter most fail against the previous code:
the pre-existing canary keeps its original bytes through a full run, and nothing
is written when the listing failed.
Relates to
mattering for any caller that knows the size.