Open Bug 778103 Opened 12 years ago Updated 2 years ago

nsJPEGDecoder::WriteInternal treats setjmp as returning nsresult

Categories

(Core :: Graphics: ImageLib, defect)

defect

Tracking

()

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.
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.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.