166886 - Crash while loading sourceforge.net (on a debug build)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[Shanmugavelu Shanmuganathan (gone)]

Mozilla's debug build crashed everytime, on a Tru64 UNIX, while loading the 
page sourceforge.net.
Debugging this stuff revealed a bug in 
"./modules/libpr0n/decoders/bmp/nsICODecoder.cpp" 
in the routine  nsICODecoder::ProcessDirEntry. 
This has a call    

DOCOPY(&aTarget.mImageOffset, mDirEntryArray+12);

while DOCOPY is defined as 

#define DOCOPY(dest, src) memcpy(dest, src, sizeof(dest))

The third argument in memcpy is the one that is causing the problem.
It gets the sizeof(dest) while dest is the size of the address 
(&aTarget.mImageOffset). This is 64 in 64 bit OS. We only need 32 bitsPRUint32 
because IconDirEntry::mImageOffset  is defined as PRUint32

This wipes out the adjacent memory and causes crash, while the content of the 
address which was stored in that memory is accessed later.

In order to solve this we can change the  DOCOPYs from

DOCOPY(&aTarget.mImageOffset, mDirEntryArray+12);
to 
DOCOPY(aTarget.mImageOffset, mDirEntryArray+12);

and the definition of DOCOPY from 

#define DOCOPY(dest, src) memcpy(dest, src, sizeof(dest))
to 
#define DOCOPY(dest, src) memcpy(&dest, src, sizeof(dest))

This solved the problem. 
I can give the full diffs if there is no objection to this.

Comment 1

[Matthias Versen [:Matti]]

-> Stuart

Comment 2

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

taking bug as I wrote this stuff

Comment 3

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

actually that's the ICO decoder which hyatt wrote, not I :) anyway, I'll fix this.

Comment 4

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

ok that code was completely broken. (this macro was part of the stuff I did
write :) ). I'll attach a slight variant of the suggested patch which should be
safer.

nominating for 1.0.2.

Comment 5

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

Attachment: patch

Comment 6

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

paper or stuart, could one of you review this patch?
shanmu, could you verify if the patch fixes your crash?

Comment 7

[Shanmugavelu Shanmuganathan (gone)]

This patch fixed the crash.

Comment 8

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

crash -> critical

Comment 9

[Scott Collins]

Comment on attachment 97996 [details] [diff] [review]
patch

Actually, I don't think I would use a macro for this.  It only saves you one
argument; it relies on something rather ambiguous to humans to make it function
correctly; and in source, the casual reader will be significantly more familiar
with |memcpy| than with |DOCOPY|.  Not to mention the fact that macro's are
best avoided in general, particularly where they don't do the magic of
conditional definition.

My advice: eliminate the macro, use straight |memcpy|s throughout.

Comment 10

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

Attachment: very well... using memcpy, removing DOCOPY

Comment 11

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

scc, could you review this patch?

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

If you want I'll give an r= for that patch.  However, given a choice(*) I'd
rather you cast your void/char pointer to the structure you're copying from and
just use structure copies.

(*) the case where doing that wouldn't be good is if you expected that some
platforms wouldn't like longword accesses on odd boundaries (not normally a
problem in modern times), or if you expected this to be byte-misaligned and
thought that the compiler might do better inlining a series of memcpy's than
doing a structure copy, or if only small isolated parts of the structure were
being copied into a different structure (though in that case I'd still want you
to make a structure pointer to pull them out).

IMHO.  Lots of memcpy's for 2 or 4 bytes is silly, even if the compiler _might_
optimize them away.

Comment 13

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

um, jesup, are you really sure that every compiler will byte-align the structure?

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Yeah, good point.  It would require making sure the source structure was
correctly packed on each compiler, which is a pain.  I still dislike the
memcpy()'s and would prefer something like:

foo.bar = *((PRUint32 *) (mypointer + 6));

Which you could combine with your 32->64 bit conversions, but as you mentioned
in  IRC these are done once per BMP so it's not important.

r=[e-mail]

Comment 15

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

(patch keyword does not apply here)

Comment 16

[tor]

Comment on attachment 98572 [details] [diff] [review]
very well... using memcpy, removing DOCOPY

While I agree that many memcpy()s is somewhat gross, it looks somewhat cleaner
than the same amount of pointer casting.

sr=tor

Comment 17

[Stuart Parmenter]

Comment on attachment 98572 [details] [diff] [review]
very well... using memcpy, removing DOCOPY

r=pavlov

Comment 18

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

ok, checked in. this crash should be fixed now.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 98572 [details] [diff] [review]
very well... using memcpy, removing DOCOPY

a=[e-mail] for 1.0 branch.  CHange mozilla1.0.2+ to fixed1.0.2 when
checked in

Comment 20

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

fixed on 1.0 branch.

Comment 21

[Terri Preston]

Verified win xp branch build 2002091908, linux branch build 2002091906 and Mac
os x branch build 2002091905

Comment 22

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

Attachment: sf favicon

unfortunately I'm not sure that this is the icon that triggered this bug