284038 - OOM crash [@ EnumFonts]

Status: RESOLVED FIXED

(Core Graveyard :: Ports: Qt, defect)

Description

[timeless]

note that cleanup for this case needs to properly release the partially
constructed array.

Comment 1

[Bastiaan Jacques]

Attachment: fix

Not only are the Allocs not checked, but in the for loop, i is never
incremented. So, EnumFonts() would always enumerate exactly one font.

Comment 2

[Bastiaan Jacques]

Attachment: better fix

Comment 3

[Zack Rusin]

Comment on attachment 187774 [details] [diff] [review]
better fix

yeah, why not, looks good. thanks.

Comment 4

[Bastiaan Jacques]

Comment on attachment 187774 [details] [diff] [review]
better fix

+	     array = (PRUnichar**)nsMemory::Realloc(count *
sizeof(PRUnichar*));

This is wrong. nsMemory::Realloc() should take two arguments. Why does the
compiler allow us to call it with only one?

Comment 5

[Bastiaan Jacques]

Attachment: oops.

..Because I don't normally build with the qt backend, of course. Boy, do I look
silly now.

Comment 6

[Zack Rusin]

Comment on attachment 187802 [details] [diff] [review]
oops.

As long as it builds I'm fine with virtually anything. Both KDE and Qt don't
handle OOM errors cleanly anyway. The bottom line on anything beside Windows is
that due to excessive swapping user has closed the application around five
times anyway ;)

Comment 7

[Benjamin Smedberg]

Comment on attachment 187802 [details] [diff] [review]
oops.

a=bsmedberg for checkin on 6/30 only

Comment 8

[Bastiaan Jacques]

Checked in by timeless (2005-06-30 13:48).

Comment 9

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

ok, so...
+            array = (PRUnichar**)nsMemory::Realloc((void*)array, count *
sizeof(PRUnichar*));

does this actually have a chance of working in an OOM situation? given how
realloc works, I actually doubt that... (because realloc leaves the old memory
untouched if the allocation fails. thus, it must request the new memory before
freeing the old one)

so, I'll reopen.

Comment 10

[Bastiaan Jacques]

Hm, okay. So would you suggest returning the partial array (with its count
adjusted), or freeing up the partial array and returning NS_ERROR_OOM?

Comment 11

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

hm, I think just adjusting the count is better... (something like the first
patch here)

Comment 12

[Bastiaan Jacques]

Attachment: remove realloc

And you couldn't have mentioned this earlier?!	;)

Comment 13

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

it worried me all the time but only now I could explain why :-)

Comment 14

[Bastiaan Jacques]

CC'ing Zack.

Comment 15

[Bastiaan Jacques]

Comment on attachment 188681 [details] [diff] [review]
remove realloc

Zack hasn't responded to my request, would you be interested in sr and rs'ing
this change, roc?

Comment 16

[Bastiaan Jacques]

Attachment 188681 [details] [diff] was checked in by timeless (2005-10-06 14:16).