ft: refuse a size==0 FileRequest() when the hash is not the empty file

[SchlauFuchs]

Revised per review

Thanks @jolavillette and @csoler for tracing through the actual behavior - you're right, the original "fall through to a real transfer" approach was wrong: there's no code path that ever corrects a transfer's stored size later (the "already downloading" branch only merges peer sources), and there's no legitimate source of a size==0 request with a real hash in normal operation. Falling through would have created a permanent zombie transfer instead of the original harmless empty file.

Revised fix

Keep the empty-hash gate (da39a3ee... is correct, as noted), but for the mismatched case, refuse the request outright instead of falling through - matching the existing oversize-request precedent a few lines below (RsErr() + return false).

Testing

Clean rebuild of libretroshare.a, no new warnings.

🤖 Generated with Claude Code

https://claude.ai/code/session_0137V8RpdwiKisg9SSrMEiBt

[jolavillette]

Thanks for looking at this corner. The empty-hash gate itself is right (da39a3ee… is SHA1 of the empty string), but I traced the fall-through path and it does not do what the description says, so I'd rather flag it before this gets merged.

There is no size resolution in the engine. A transfer's size is copied once at construction (ftFileControl, ftFileProvider, ChunkMap::_file_size, ftTransferModule::mSize at fttransfermodule.cc:87) and never written again. A later FileRequest() for the same hash with the correct size takes the "Already Downloading File" branch (ftcontroller.cc:1049-1105), which only merges sources and never reads size. So "once the size becomes known" never happens.

What a size-0 request with a real hash does on the normal path (all lines on current master):

• ChunkMap(0) has zero chunks, so getDataChunk() never allocates anything (ftchunkmap.cc:350-357): no data is ever requested. It still sends a chunk-map request to every source every 60 s and rejects every reply for size mismatch (:483-486), and turtle keeps digging tunnels for the hash. The entry sits at 0/0 in the transfer list until the user cancels it, and it is saved to the config.
• After a restart (or as soon as the user clicks "Force check" on the stuck row), setAvailabilityMap() / forceCheck() on an empty map leave _file_is_complete = true (ftchunkmap.cc:92-119, :673-682). The transfer then goes to CHECKING, checkFile() spawns a hash thread, the partial file does not exist so getFileHash fails and the hash is null, mismatch → forceCheck() → complete again → new hash thread. One hashing thread every 2 s, forever, per such transfer, with no diagnostic. completeFile() is never reached, and it already treats mSize == 0 as ERROR_COMPLETION anyway (:742-743).

Where would a size-0 request come from? I could not find a "not synced yet" state. The remote directory sync reads name, size and hash as mandatory fields of one entry (directory_storage.cc:1061-1064) and the sender never ships unhashed files; every in-tree caller (links, search, collections, mail, channels, remote dirs, JSON clients) passes the size from the same record as the hash. The remaining sources are malformed input: a hand-made retroshare://file?…&size=0 link, a collection XML, a JSON call that sends or omits size, or a remote peer publishing a 0-size attachment — and two of those are zero-click (channel auto-download at p3gxschannels.cc:1181, mail push at p3msgservice.cc:265). With this PR each of them would create a permanent zombie plus the hashing loop above, remotely triggerable. Today they create a harmless empty file.

So as it stands I don't think this should go in. If you have an actual reproduction of size == 0 with a real hash from a normal sync, that would be the thing to fix at the source and I'd be glad to look at it. If not, I'd suggest closing this one. Should you want to keep the hash gate as hardening against malformed input, the safe shape is to refuse the request instead of falling through, i.e. return false with an RsErr() for size == 0 && hash != emptyFileHash — with the caveat that the GUI and webui already read a false return as "you already have this file" (MessageWidget.cpp:426-428, SearchDialog.cpp:442-445, mail_util.js:275), exactly like the existing oversize refusal at ftcontroller.cc:973-982.

[csoler]

Let's watch let the IAs fight...

[csoler]

I agree with jolavilette here: this case should not (meaning there's technically no way it can) happen. Of course you can drop an error message in the if(size==0) {} branch when the hash is not the hash of an empty string, in order to make it visible if by any chance this inconsistency happens in the future.

[SchlauFuchs]

You're right, thanks for tracing it through - there's no path that ever corrects a stored size, and no legitimate source of size==0 with a real hash. Revised to refuse the mismatched case outright (RsErr() + return false) instead of falling through, matching the oversize-request precedent.

[jolavillette]

Pre-review done with an AI under my supervision.

The revised version addresses everything raised above: the mismatched size == 0 case is now refused with a log line, in line with the oversize check a few lines below, and the two directory-creation fixes from #391 (create the folder before writing the empty stub, non-throwing create_directories in moveFile()) are what RetroShare/RetroShare#3312 relies on once it drops the GUI-side mkpath() calls. RetroShare/RetroShare#3313 carries the first commit of #3312, so it depends on this one too. Merge order: this PR first, then #3312 / #3313. CI is green on the three platforms.

One note: the third commit (skipping the GitLab mirror job on forks) is unrelated to file transfer and appears identically in #3312 and #3313. Harmless, but it may be cleaner as its own small PR, or squashed out at merge time, your call.

This now needs a human review.