229652 - Browser crashes accessing broken GIF

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[noririty]

Since checkin of bug 205761.
Mozilla, Mozilla Firebird on Windows XP and Windows 2000 crash loading certain URL.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1935&action=view
Those pages contain bad GIF image file.
This situation may easily occur by interruption while FTP transfer.

Comment 1

[Hermann Schwab]

wfm Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20031226
Bug 205761 was checked in 2003-10-22 14:12

no crash, instead of image I get an icon of a broken image with text, the URL
isn´t fully displayed, it stops at the border of the box:

The image 
“http://bugzilla.mozilla.gr.j
cannot be displayed, 
because it contains 
errors.

Comment 2

[Hermann Schwab]

crashes Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20031226
didn´t crash using Mozilla 1.5, so no Talkback.

Couldn´t get it to crash when loading in same tab, but multiple times crashed
loading in a new tab loading in the background.
I was seeing an animated gif just cycling once, then sometimes crashing.
Right-clicking on the gif produced the same broken gif icon.
I saved it locally as attachment.cgi.html and I renamed a copy to
attachment.cgi.html.gif
I couldn´t load the copy in Mozilla, but could see it looping in irfanview.

After some unsuccessful attempts to crash, I started writing this bug, then,
when opening a menu, Mozilla crashed. 

Comment 3

[tor]

Attachment: fix nsRecyclingAllocator, propogate errors to imglib, stop memory scribbling

Comment 4

[tor]

*** Bug 229520 has been marked as a duplicate of this bug. ***

Comment 5

[Darin Fisher]

Comment on attachment 139062 [details] [diff] [review]
fix nsRecyclingAllocator, propogate errors to imglib, stop memory scribbling

>Index: xpcom/ds/nsRecyclingAllocator.cpp

>+        if (zeroit)
>+            memset(DATA(freeBlock), 0, bytes);
>+
>         return DATA(freeBlock);

store result of DATA to avoid duplicating pointer arithmetic.  i'm
not sure if the compiler will anyways optimize away the duplicate
arithmetic, so maybe this is a ridiculous point.  but, fwiw maybe
this would be better:

   void* data = DATA(freeBlock);
   if (zeroit)
       memset(data, 0, bytes);
   return data;


>Index: modules/libpr0n/decoders/gif/GIF2.cpp

>+  /* Protect against too much image data */
>+  if ((PRUintn)drow_start >= gs->height)
>+    return;
>+

nit: would a debug warning or assertion be of use here?

  NS_WARNING/NS_ERROR("too much image data");


>Index: modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp

>+  nsresult rv = inStr->ReadSegments(ReadDataOut, this,  count, _retval);
>+
>+  /* necko doesn't propogate the errors from ReadDataOut - take matters
>+     into our own hands */
>+  if (NS_SUCCEEDED(rv) && mGIFStruct && mGIFStruct->state == gif_error)
>+    return NS_ERROR_FAILURE;
>+  else
>+    return rv;
> }

nit: "else" after "return" is unnecessary.  how about this instead:

  if (...)
    rv = NS_ERROR_FAILURE;
  return rv;


r=darin

Comment 6

[tor]

Attachment: fix style nits

Comment 7

[tor]

Comment on attachment 139167 [details] [diff] [review]
fix style nits

Transfering r=darin

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 139167 [details] [diff] [review]
fix style nits

>Index: modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v
>retrieving revision 1.58
>diff -u -r1.58 nsGIFDecoder2.cpp
>--- modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp	14 Jan 2004 17:08:26 -0000	1.58
>+++ modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp	16 Jan 2004 01:39:13 -0000
>@@ -214,7 +214,14 @@
> /* unsigned long writeFrom (in nsIInputStream inStr, in unsigned long count); */
> NS_IMETHODIMP nsGIFDecoder2::WriteFrom(nsIInputStream *inStr, PRUint32 count, PRUint32 *_retval)
> {
>-  return inStr->ReadSegments(ReadDataOut, this,  count, _retval);
>+  nsresult rv = inStr->ReadSegments(ReadDataOut, this,  count, _retval);
>+
>+  /* necko doesn't propogate the errors from ReadDataOut - take matters

"propagate"

sr=bryner

Comment 9

[tor]

Checked in with spelling fixed.

Comment 10

[Olivier Vit (just a reporter)]

breaks now on some jpegs ?? (large ones?)

http://elections.lesverts.fr/kit-fevrier2004/En-tetes/En-tete-NB.jpg
http://elections.lesverts.fr/kit-fevrier2004/Logos/Images-demo/tournesol_champ.jpg

winxp build 2004020808

Comment 11

[tor]

Not sure why you're mentioning a JPEG decoder problem in a GIF bug, but
the problem with those files is that they're in YCCK colorspace (bug 44781).

Comment 12

[Olivier Vit (just a reporter)]

was because of the error message being displayed

maybe it could be customized to say that this type of jpeg file format isn't
supported ??

anyway, really helpful feeback, regards

Comment 13

[Charles Fenwick]

*** Bug 234043 has been marked as a duplicate of this bug. ***

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 235098 has been marked as a duplicate of this bug. ***

Comment 15

[Robin Monks]

*** Bug 272955 has been marked as a duplicate of this bug. ***