The FTP STOR path in addon/ftpserver/ftpworker.cpp can destroy or truncate a file and can report a transfer as complete when it was not. This came out of the multi BIN work in #241, where two track files of a 17 file rip had uploaded as zero bytes. They opened without error, contributed nothing to the image, and produced an invalid TOC. Multi BIN loading now refuses an image with an empty data file, so the symptom is visible, but the upload path that produced those files is unchanged.
Four separate problems, all in CFTPWorker::Store().
The destination is truncated before the data connection exists. The file is opened with FA_CREATE_ALWAYS | FA_WRITE before OpenDataConnection() is called. If the data connection fails to come up, the function returns with the destination already truncated to zero bytes. An upload that never transferred a byte can therefore replace a good file with an empty one. The open should happen after the data connection is established, or the transfer should be written to a temporary name and renamed on success.
Short writes are ignored. Both f_write() calls check only the FRESULT and never compare nWritten against the requested length. FatFs reports a full volume as FR_OK with a short count, so filling the card mid upload is not detected at all. The byte count needs to be checked on every write.
f_sync() failures are ignored. The return value is discarded in both places it is called. A sync failure at the end of a transfer is exactly the case where the data did not reach the card.
Completion is reported even when the writes did not all land. Because of the two problems above, bSuccess can still be true after a short write, and the client is told "Transfer complete." A client that trusts that reply has no way to know the file on the card is wrong.
There is also a resource leak worth fixing at the same time. The two early returns after f_open(), one when the initial SendStatus() fails and one when OpenDataConnection() returns null, both return without calling f_close(). The only f_close() in the function is on the normal path.
Expected behaviour: do not truncate the destination until the transfer can actually start, treat a short write or a failed sync as a failed transfer, and report failure to the client when the file on the card is not what was sent.
This was deliberately left out of #242 to keep that change scoped to multi BIN support.
The FTP STOR path in
addon/ftpserver/ftpworker.cppcan destroy or truncate a file and can report a transfer as complete when it was not. This came out of the multi BIN work in #241, where two track files of a 17 file rip had uploaded as zero bytes. They opened without error, contributed nothing to the image, and produced an invalid TOC. Multi BIN loading now refuses an image with an empty data file, so the symptom is visible, but the upload path that produced those files is unchanged.Four separate problems, all in
CFTPWorker::Store().The destination is truncated before the data connection exists. The file is opened with
FA_CREATE_ALWAYS | FA_WRITEbeforeOpenDataConnection()is called. If the data connection fails to come up, the function returns with the destination already truncated to zero bytes. An upload that never transferred a byte can therefore replace a good file with an empty one. The open should happen after the data connection is established, or the transfer should be written to a temporary name and renamed on success.Short writes are ignored. Both
f_write()calls check only theFRESULTand never comparenWrittenagainst the requested length. FatFs reports a full volume asFR_OKwith a short count, so filling the card mid upload is not detected at all. The byte count needs to be checked on every write.f_sync()failures are ignored. The return value is discarded in both places it is called. A sync failure at the end of a transfer is exactly the case where the data did not reach the card.Completion is reported even when the writes did not all land. Because of the two problems above,
bSuccesscan still be true after a short write, and the client is told "Transfer complete." A client that trusts that reply has no way to know the file on the card is wrong.There is also a resource leak worth fixing at the same time. The two early returns after
f_open(), one when the initialSendStatus()fails and one whenOpenDataConnection()returns null, both return without callingf_close(). The onlyf_close()in the function is on the normal path.Expected behaviour: do not truncate the destination until the transfer can actually start, treat a short write or a failed sync as a failed transfer, and report failure to the client when the file on the card is not what was sent.
This was deliberately left out of #242 to keep that change scoped to multi BIN support.