-
-
Notifications
You must be signed in to change notification settings - Fork 851
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added sanity check for every jpeg marker #2084
Added sanity check for every jpeg marker #2084
Conversation
// to uint to avoid sign extension | ||
if (stream.RemainingBytes < (uint)markerContentByteSize) | ||
{ | ||
JpegThrowHelper.ThrowNotEnoughBytesForMarker(fileMarker.Marker); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this error message would contain hex value of the marker. While we can map byte to ITU spec name I don't think it's worth the effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. Does it make sense to construct at least one faulty image with a hex editor for regression testing? (Ideally I would overflow the last - non SOS - marker in the file.)
Sounds reasonable, will create one today. |
@antonfirsov wow, latest release version can actually fall into really dangerous code regions for malformed jpegs.
New stacktrace:
|
Not that terrible, could be access violation 😆 |
Prerequisites
Description
Discussed at #2077. Now before even trying to parse any jpeg marker decoder would check whether input stream has enough bytes available thus there's no need to check any
stream.Read(...)
call for return value.Closes #2085.