Duet: verify RepRapFirmware uploads with the rr_upload crc32 parameter - #15540
Open
LZZZZ wants to merge 5 commits into
Open
Duet: verify RepRapFirmware uploads with the rr_upload crc32 parameter#15540LZZZZ wants to merge 5 commits into
LZZZZ wants to merge 5 commits into
Conversation
Two of the print host protocols PrusaSlicer supports let the client send a checksum of the file it is uploading so the receiving end can verify the transfer, but no backend computes one. Add the missing primitive. crc32_file() streams the file in 64 KiB chunks and returns the CRC-32 variant used by zlib and by RepRapFirmware (polynomial 0x04C11DB7 reflected, initial value 0xFFFFFFFF, final XOR 0xFFFFFFFF), via the header only boost::crc_32_type. The file is read in binary mode so the checksum describes the bytes on disk. It returns std::nullopt rather than a plausible looking value when the file cannot be read, so callers cannot accidentally send a checksum for data they never managed to read. to_hex() formats the result zero padded to 8 digits without an "0x" prefix, which is the form rr_upload expects. Tests cover the standard CRC-32 check vector, the empty file, sizes on both sides of the read buffer boundary including exact multiples of it, and that CR LF is not translated away. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
set_post_body(const fs::path &) opened the file with the default open mode. On Windows that translates CRLF to LF, so the bytes sent differ from the bytes on disk. This silently corrupts binary G-code uploaded through a POST body and makes any checksum computed over the file disagree with what the server receives. The other file body helper, set_put_body(), already opens in binary mode. The callers affected are the Duet rr_upload path and MKS. On Linux and macOS the two modes are identical, so this only changes behaviour on Windows, where the previous behaviour was wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An upload to a Duet 2 WiFi running RRF 3.6.1 arrived with a 3063 byte window
corrupted 94.75% of the way through a 123.7 MiB file: 479 altered bytes, 369 of
them single bit flips spread evenly across all 8 bit positions, 15 destroyed
line terminators, and 3 NUL bytes introduced. The upload reported success, the
file size on disk matched exactly, and two 30+ hour prints failed at the same
layer before the cause was found. Re-uploading produced a byte exact copy, so
the corruption was transient in transit rather than a bad SD card. TCP's
checksum is 16 bits and weak, and in this case the corruption happened past it,
on the SPI link between the WiFi module and the main MCU.
rr_upload accepts an optional crc32 parameter for exactly this. RepRapFirmware
compares it against the CRC32 of the data it received, and on a mismatch it
fails the upload, deletes the partial file and replies {"err":1}. It has been
supported since 2.04RC3 and the Duet3D documentation states "Usage of this
parameter is encouraged". PrusaSlicer never sent it, so nothing checked that an
uploaded file arrived intact.
Send it, and report a rejected upload as a probable transfer corruption rather
than as an unspecified error. Older firmware ignores the parameter, so this is
safe for existing setups.
The DuetSoftwareFramework endpoint uses a different request and has no
equivalent parameter, so it is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
get_err_code_from_body() called pt::read_json() unguarded. A reply that is not JSON, for example an HTML error page from an interposed proxy or a wedged firmware, makes it throw pt::json_parser_error. The exception escapes the on_complete handler and Duet::upload() entirely, and is only caught at the top of PrintHostJobQueue::bg_thread_main(), which tears down the whole upload queue thread for the rest of the session. Verified by replying to rr_upload with an HTML body: before this change the exception propagates out of upload(); after it, the upload reports a normal error and the queue survives. Return -1 in that case. Both callers already treat any non-zero value as a failure, and the switch in connect() falls through to its default branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The X-Session-Key header was only sent when connect_msg was empty, that is, only when there was no session key to send. connect() stores the sessionKey returned by machine/connect in that variable, so the intent was clearly the opposite. NOT TESTED: this path only runs against DuetSoftwareFramework on an SBC based Duet, which I do not have. Reported as an evident inversion rather than as a verified fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Thank you for this contribution. We imported the validated portion of this change into the community-maintained AGPL branch at opensensor/PrusaSlicer.
Our intent is to keep the existing PrusaSlicer code line maintained while this upstream repository is read-only. We are reviewing the backlog, preserving original authorship, and landing useful changes directly on our We use local review and targeted tests for fast intake, followed by fix-forward or an individual revert if a regression is found. If you know of additional constraints, tests, or follow-up work for this change, please reply here or open an issue in the community fork. |
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
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.
Problem
rr_uploadaccepts an optionalcrc32parameter that lets RepRapFirmware verify an uploaded fileagainst the CRC32 computed by the client. PrusaSlicer does not send it, so a Duet upload is never
checked. This adds it.
A 129,682,213 byte upload to a Duet 2 WiFi (RRF 3.6.1) arrived with one corrupted 3,063-byte
window, 94.75% of the way through the file.
rr_uploadreturned{"err":0}and the size on diskwas exact, so nothing detected it; two prints then failed at that layer while the firmware
reported the job completed normally. Re-uploading produced a byte-exact copy, so the corruption
was transient in transit rather than a bad SD card.
RepRapFirmware has compared this parameter against the data it received since 2.04RC3, failing
the upload and deleting the partial file on mismatch. The Duet3D documentation states "Usage of
this parameter is encouraged."
Detail of the corrupted window, if useful
Bytes 122,867,719–122,870,781; the rest of the file was byte-identical to a fresh export.
0x1Abytes introducedTCP's checksum is 16 bits and weak, and here the corruption occurred past it, on the SPI link
between the WiFi module and the main MCU. Only a client-computed checksum verified by the
firmware spans that boundary.
Changes
libslic3r/Checksum.{hpp,cpp}(new) —crc32_file()streams a file in 64 KiB chunks andreturns the CRC-32 variant used by zlib and RRF, via header-only
boost::crc_32_type, so nonew dependency. Returns
std::nulloptwhen the file cannot be read, so a checksum is neversent for data that was not read. Placed in
libslic3rand kept generic so Moonraker canfollow separately.
Duet.cpp— sends&crc32=<hex>on therr_uploadURL, and reports a rejected upload asprobable transfer corruption rather than an unspecified error. Older firmware ignores the
parameter. The DSF endpoint has no equivalent and is unchanged.
Http.cpp—set_post_body(const fs::path &)now opens in binary mode. Prerequisite: intext mode a Windows build translates CRLF to LF, so the bytes sent differ from the bytes on
disk and the checksum would not match what the server receives. Independently, this is why
binary G-code sent through a POST body is mangled on Windows today.
set_put_body()wasalready binary. The other affected caller is MKS.
Two adjacent fixes in the same file, as separate commits so they can be dropped independently:
Duet.cpp—get_err_code_from_body()calledpt::read_json()unguarded. A non-JSONreply throws, the exception escapes
Duet::upload(), and it is caught only at the top ofPrintHostJobQueue::bg_thread_main()— which kills the upload queue thread for the rest of thesession. Now guarded, returning -1, which both callers already treat as a failure.
Duet.cpp—X-Session-Keyis sent only when the session key is empty(
if (connect_msg.empty())), so it is never sent when there is a key. Introduced in2a4e09a, which added the header, so this path has not worked since.
Verification
Against the same hardware and the same 129,682,213 byte file:
rr_filelistconfirmed the file was absent afterwards with no partial leftovers.rr_uploadservercrc32=<hex>; the value equals an independentzlib.crc32of the received body; a payload containing\r\n,0x1Aand NUL arrives at full length.upload()without change 4; handled cleanly with it.tests/libslic3r/test_checksum.cppcovers the standard CRC-32 check vector ("123456789"→0xCBF43926, which pins the implementation to the variant the firmware expects rather thanCRC-32C), the empty file, sizes either side of the read buffer boundary including exact multiples
of it, and that CRLF is not translated away.
libslic3r_testsandslic3rutils_testspass.Change 5 is not tested — it only runs against DuetSoftwareFramework on an SBC-based Duet,
which I do not have. If someone with that setup can try it I would appreciate it; otherwise I am
happy to drop that commit and submit it separately. The Windows behaviour of change 3 is reasoned
from
ifstreamsemantics rather than observed; on Linux and macOS the two modes are identical.Not included
The same gap exists elsewhere and I have kept this to the backend I can verify on hardware.
Moonraker.cppquotes the API documentation for itschecksumfield verbatim in a comment, andthe code four lines below adds
root,path,printandfilebut notchecksum. That needs aSHA256 source and a Klipper host to test against.
PrusaConnect.cppnever reads thehashfieldin the server's reply.