Possible file corruption

[Big Muscle]

This bug happens due to following source code:

FilteredFile.h, line 141: flushed = true;

Flushed flag is set to true although file is not flushed at this time. It causes that flushing in DownloadManager::endData() fails, because it thinks that file has already been flushed. If some system error (crash, hang etc.) occurs, file content will be lost, because it hasn't been flushed to disk.

Another problem with same source code is there. When user's HashData is not accessible (e.g. no access to file, file is corrupted etc.) and client can't get tree from such user, it uses only simple kind of verification - by TTH root. It's ok, but because DownloadManager/FilteredFile thinks that the file has already been flushed, it doesn't verify TTH root on file finish at all. So even though downloaded file's TTH root is different from original TTH root (i.e. file is corrupted), the file is normally finished and DC++ doesn't say any error (but it should say "TTH inconsistency" or something).

Possible solution for both problem would be to remove that "flushed = true" line and reset download position when flush fails on download finish (DownloadManager.cpp, line 328), so such segment wouldn't be marked as downloaded.

[eMTee (realprogger)]

If its true then this way any potentially dangerous file can be downloaded from a rogue client which responds so it shares eg. a file which has a TTH that is equal to some orginal harmless file. I set this report private until further investigation...

[eMTee (realprogger)]

Looks like the attached patch (by Big Muscle) solves the problem so the corrupted file can't go into the finished downloads folder anymore. However,
1. the wrong source kept in the queue resulting infinite redownload of the corrupted file.
2. a successful exploitation of this vulnerability needs that the source should not provide the full tiger tree so it may worth to rethink what to do when the full tree isn't available (there's also a resume problem in this case, explained at https://bugs.launchpad.net/dcplusplus/+bug/288756).?field.comment=Looks like the attached patch (by Big Muscle) solves the problem so the corrupted file can't go into the finished downloads folder anymore. However,
1. the wrong source kept in the queue resulting infinite redownload of the corrupted file.
2. a successful exploitation of this vulnerability needs that the source should not provide the full tiger tree so it may worth to rethink what to do when the full tree isn't available (there's also a resume problem in this case, explained at https://bugs.launchpad.net/dcplusplus/+bug/288756).

[Big Muscle (bigmuscle)]

The another possible solution would be flushing in FilteredFile. But upper patch has been tested and it works and doesn't bring any other problems. And just for an order, copyright is given to arne to use in DC++.

[Big Muscle (bigmuscle)]

I found another possible source of file corruption. When file is being transferred with ZLIB compression, it can happen that decompression error or ""Garbage data after end of stream" exceptions occur after the last bytes of segment has been transferred. These exceptions will be caught in DownloadManager::on(Data)'s handler and therefore endData won't be executed. But the whole segment can be marked as downloaded and file will be seen as finished in queue. It can cause file corruption and crash when removing such file from queue.

[Steven Sheehy (steven-sheehy)]

Please open a separate bug for each unique type of file corruption.

[Big Muscle (bigmuscle)]

I didn't open new bug, because both have similar cause - handled exception after last segment's bytes are transferred.

[eMTee (realprogger)]

The second possible source of file corruption does not seem to be valid, actually I can't find a way in the code that downloaded segments can be altered in any way if endData isn't called. Tested with a simulated decompression error and the downloaded file neither saved nor shown finished or caused crash when/after removed from the queue.

[Big Muscle (bigmuscle)]

True,

addPos in following line won't be processed when exception in write occurs:
d->addPos(d->getFile()->write(aData, aLen), aLen);

That's why I said you that this place is only my guess and I'm not sure where the problem exactly is. The only guarantee is that there's still the problem somewhere (all segments are marked as finished but endData wasn't processed) and bug is NOT fixed.

[eMTee (realprogger)]

OK, but,
1. In my understanding this second reported corruption cannot be achieved/exploited the (same) way as it described in this bug report's description
2. The commited fix (which fixes the way of the problem originally filed in this report) will be released soon
so if there's another way (or suspicion) to get files corrupted (or crashes can occour) I'd agree with Steven so we'd better to file a separate report for that problem.

[Big Muscle (bigmuscle)]

Attached patch is not complete:

a) flush() in DownloadManager can throw also Exception class, but only FileException is handled

b) looking at FilteredFile.h (line 105, function flush()), the "flushed" flag is set to true before flushing really occurs. When exception is thrown after that (it can happen in functions called at lines 111, 113 or 118), flushing won't happen but flushed flag will be set.

[poy (poy)]

does this patch solve exception catching issues? note that regarding a), exceptions were always being caught anyway, but resetPos was only caught on FileException types of exceptions; i have generalized it to calling resetPos after any flush exception, is that correct?

i think b) is on purpose, in order not to try flushing a file again if a previous flush operation failed. in any case, had such a previous flush attempt failed, it would have been caught before.

[Big Muscle (bigmuscle)]

The patch looks ok, I just can't test it now. And what about putting flush() function as a member of Download class?

[Big Muscle (bigmuscle)]

I finally found where the another problem was. It was caused by incorrect patch, because "getStartPos()" returns offset from the beginning of the file, but "pos" is offset form the beginning of the current segment.

So there should be:
void resetPos() { pos = 0; }

I hope this is correct now and problem is fixed.

[eMTee (realprogger)]

[19:35] <BigMuscle> so in the current DC++ (0.762) and StrongDC++ (2.41) releases the possibility to crash seems to be only by doing following steps:
1. start downloading file with tree available
2. shutdown client when some segments are finished (but not whole file)
3. restart client but tree can't be available (e.g. nonaccessible hashdata)
4. resume file downloading from source(s) without tree (so no source with tree is available)
5. get corruption

This sound hard to exploit and the correction above fixes this problem as well so I think we can make this report public the time when DC++ 0.762 goes stable.

[poy (poy)]

here is an updated patch, test please.

i have a few questions:
- DC++ 0.762 has been released with the wrong variant of resetPos, can it cause corruption when flushing fails? in that case, better not make it stable.
- this bug isn't fully fixed until some patch like this one i'm submitting is applied, right? or are compression / other kind of failures not a problem when flushing?

[Big Muscle (bigmuscle)]

Corruption can't happen with invalid resetPos. Only a crash by doing 5 steps that were described by eMTee. It seems that it is really difficult to reproduce, but this crash has already been reported 140× in StrongDC++ 2.41 (unless there is another bug which has nothing to do with this bug report, but I verified code and everything seems ok - except that resetPos).

The only problem is with flush in endData, so only this one needs resetPos in exception handler but it must be valid for all types of exceptions (not only FileException). Another calls of flush doesn't need to me modified.

[poy (poy)]

good, this makes it easier to fix. exception handler changed in rev 2154.

[poy (poy)]

Fixed in DC++ 0.770.