COMMON: Avoid Quicktime parsing crash on early EOF #5368
Conversation
|
Doing this as a PR as I'm not 100% sure of my fix - @elasota I think you've touched this code recently(ish), not sure if you have any thoughts? |
|
Couldn't you just check the err/eos functions on |
I think the loop on line 184 can fall through the first time even if there is no particular error, that's why I was trying to check something about the output - but then I wasn't fully sure if |
|
Would it make more sense to initialize the atom size to |
|
@mduggan any updates? |
|
Thanks for the ping I think @elasota's suggestion right about changing the initialization of The original reason for the change went away when I fixed my corrupted file by the way - so this is not a critical bug. |
mduggan
force-pushed
the
qt_seek_crash
branch
from
a210c24
to
5f30d9d
Compare
WalkthroughWalkthroughThe changes involve enhancing the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
common/formats/quicktime.cpp
Outdated
| _foundMOOV = false; | ||
| _disposeFileHandle = disposeFileHandle; | ||
|
|
||
| Atom atom = { 0, 0, 0xffffffff }; | ||
| int64 strmleft = stream->size() - stream->pos(); | ||
| // Atom size is uint32, but use int32 max as it can wrap in later expressions | ||
| uint32 maxsize = (uint32)MIN(strmleft, (int64)(1 << 31)); | ||
| Atom atom = { 0, 0, maxsize }; | ||
|
|
||
| if (readDefault(atom) < 0 || !_foundMOOV) { | ||
| close(); |
There was a problem hiding this comment.
The change to calculate maxsize based on the remaining stream size is a good preventive measure against integer overflow and invalid seek operations. However, it's important to ensure that the MIN macro is well-defined and behaves as expected, especially in edge cases where strmleft might be negative due to an error in stream->pos() or stream->size(). Additionally, the code assumes that stream->size() returns the total size of the stream, which may not be the case for all stream types. It would be prudent to verify that stream->size() and stream->pos() are returning expected values and that strmleft is non-negative before proceeding with the calculation.
int64 strmleft = stream->size() - stream->pos();
if (strmleft < 0) {
warning("Stream position is beyond the stream size.");
close();
return false;
}
|
Ok, I switched to using stream size, and added some simple tests which previously crashed and now pass. |
|
@sluicebox has worked on the QT code, so perhaps he could also share some feedback on this? |
|
I would rather see this fixed in If it's valid for the If we don't know these answers, I think it's better to do nothing for now than complicate the caller code. Although this issue was uncovered by a corrupt file that's been replaced, it is still worth improving the error handling. Parsers that crash (possibly with an assert?) instead of returning an error is a wide problem in the codebase; see #2486 |
In parseStream, atom.size is initailized to uint32 max. If the loop in readDefault then never executes because the stream has hit EOF, that size is passed as-is to seek. This can cause a crash eg in SeekableSubReadStream because of an int overflow. Change readDefault to return an error in a few places if we have hit eof before a valid atom has been read. It's ok not to do the seek in this case.
mduggan
force-pushed
the
qt_seek_crash
branch
from
5f30d9d
to
8687a21
Compare
|
|
||
| a.offset = atom.offset; | ||
|
|
||
| if (_fd->eos() || _fd->err() || (_fd->pos() == _fd->size())) | ||
| return -1; | ||
|
|
||
| while(((total_size + 8) < atom.size) && !_fd->eos() && _fd->pos() < _fd->size() && !err) { | ||
| a.size = atom.size; | ||
| a.type = 0; |
There was a problem hiding this comment.
The code checks for end-of-stream (EOS) and errors before entering the loop, which is good for preventing reads past the end of the file. However, the check for (_fd->pos() == _fd->size()) is redundant after checking _fd->eos(), as they both indicate the end of the stream.
- if (_fd->eos() || _fd->err() || (_fd->pos() == _fd->size()))
+ if (_fd->eos() || _fd->err())Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| a.offset = atom.offset; | |
| if (_fd->eos() || _fd->err() || (_fd->pos() == _fd->size())) | |
| return -1; | |
| while(((total_size + 8) < atom.size) && !_fd->eos() && _fd->pos() < _fd->size() && !err) { | |
| a.size = atom.size; | |
| a.type = 0; | |
| a.offset = atom.offset; | |
| if (_fd->eos() || _fd->err()) | |
| return -1; | |
| while(((total_size + 8) < atom.size) && !_fd->eos() && _fd->pos() < _fd->size() && !err) { | |
| a.size = atom.size; | |
| a.type = 0; |
| while(((total_size + 8) < atom.size) && !_fd->eos() && _fd->pos() < _fd->size() && !err) { | ||
| a.size = atom.size; | ||
| a.type = 0; |
There was a problem hiding this comment.
The assignment a.size = atom.size; inside the loop is incorrect. It should be set to the size read from the stream, not the size of the parent atom. This could lead to incorrect parsing behavior.
- a.size = atom.size;
+ // a.size should be set to the size read from the stream, not the parent atom's size.Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| while(((total_size + 8) < atom.size) && !_fd->eos() && _fd->pos() < _fd->size() && !err) { | |
| a.size = atom.size; | |
| a.type = 0; | |
| while(((total_size + 8) < atom.size) && !_fd->eos() && _fd->pos() < _fd->size() && !err) { | |
| // a.size should be set to the size read from the stream, not the parent atom's size. | |
| a.type = 0; |
| uint32 start_pos = _fd->pos(); | ||
| err = (this->*_parseTable[i].func)(a); | ||
|
|
||
| if (!err && (_fd->eos() || _fd->err())) | ||
| err = -1; | ||
|
|
||
| uint32 left = a.size - _fd->pos() + start_pos; | ||
|
|
||
| if (left > 0) // skip garbage at atom end |
There was a problem hiding this comment.
The error check after calling the parsing function is correct, but the calculation of left is potentially incorrect. It assumes that _fd->pos() will always be greater than start_pos, which might not be the case if the stream seeking behavior is incorrect. It would be safer to check if _fd->pos() is not less than start_pos before subtracting to avoid underflow.
- uint32 left = a.size - _fd->pos() + start_pos;
+ if (_fd->pos() < start_pos) return -1; // Add this check to prevent underflow
+ uint32 left = a.size - (_fd->pos() - start_pos);Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| uint32 start_pos = _fd->pos(); | |
| err = (this->*_parseTable[i].func)(a); | |
| if (!err && (_fd->eos() || _fd->err())) | |
| err = -1; | |
| uint32 left = a.size - _fd->pos() + start_pos; | |
| if (left > 0) // skip garbage at atom end | |
| uint32 start_pos = _fd->pos(); | |
| err = (this->*_parseTable[i].func)(a); | |
| if (!err && (_fd->eos() || _fd->err())) | |
| err = -1; | |
| if (_fd->pos() < start_pos) return -1; // Add this check to prevent underflow | |
| uint32 left = a.size - (_fd->pos() - start_pos); | |
| if (left > 0) // skip garbage at atom end |
|
Thanks for the suggestion @sluicebox. After some thought I've realized there should always be at least 8 bytes to read in |
|
Excellent work and research. Thank you! Merging. |
In parseStream, atom.size is initailized to uint32 max. If the loop in readDefault then never executes because the stream has hit EOF, that size is passed as-is to seek. This can cause a crash eg in SeekableSubReadStream because of an int overflow.
If nothing has been read just skip seeking at the end of readDefault. parseStream will correctly return false in this case as no MOOV will be found.