Open
Bug 778103
Opened 12 years ago
Updated 2 years ago
nsJPEGDecoder::WriteInternal treats setjmp as returning nsresult
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
NEW
People
(Reporter: ayg, Unassigned)
References
Details
http://dxr.lanedo.com/mozilla-central/image/decoders/nsJPEGDecoder.cpp.html?string=nsJPEGDecoder.cpp#l190
/* Return here if there is a fatal error within libjpeg. */
nsresult error_code;
if ((error_code = setjmp(mErr.setjmp_buffer)) != 0) {
if (error_code == NS_ERROR_FAILURE) {
PostDataError();
/* Error due to corrupt stream - return NS_OK and consume silently
so that libpr0n doesn't throw away a partial image load */
mState = JPEG_SINK_NON_JPEG_TRAILER;
PR_LOG(gJPEGDecoderAccountingLog, PR_LOG_DEBUG,
("} (setjmp returned NS_ERROR_FAILURE)"));
return;
} else {
/* Error due to reasons external to the stream (probably out of
memory) - let libpr0n attempt to clean up, even though
mozilla is seconds away from falling flat on its face. */
PostDecoderError(error_code);
mState = JPEG_ERROR;
PR_LOG(gJPEGDecoderAccountingLog, PR_LOG_DEBUG,
("} (setjmp returned an error)"));
return;
}
}
This is, to put it mildly, unlikely to produce expected results. The NS_ERROR_FAILURE path is presumably impossible. The other path will call PostDecoderError, which will either call NS_ABORT_IF_FALSE if the high bit of error_code happens to be 0, or will store it and report gibberish.
I don't understand this code does well enough to actually fix it, so I'm just reporting it. In bug 777292 I wrote a patch that adds a cast to nsresult so that it will actually compile once nsresult type-safety is enforced, plus a comment pointing out that the code is wrong.
Reporter | ||
Comment 1•12 years ago
|
||
Actually, I just didn't understand what setjmp()/longjmp() do. setjmp() returns whatever longjmp() was passed, which in this case is actually NS_ERROR_FAILURE. This mixing of nsresult and int is still confusing, however, and IMO it would be better to use some other integer value like -1 so that we don't have to cast things. See also another patch related to this in bug 782252.
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•