802459 - OOM crash if attaching too large file for filelink

Status: RESOLVED FIXED

(Thunderbird :: FileLink, defect)

Description

[Makoto Kato [:m_kato]]

I have fixed Gecko side by bug 801483 if nsIBufferedInputStream.init(<too large size) causes OOM crashes (ex. bp-f7294fd5-e5d4-4c5f-81ea-86b722121012).  So init(<too large size>) in Gecko 19 will throw NS_ERROR_OUT_OF_MEMORY instead of crash.

But, buffer length parameter of init should not use file size because this is buffer size for seeking.

the following code is incorrect usages of nsIBufferedInputStream.

/mail/components/cloudfile/nsUbuntuOne.js (View Hg log or Hg annotations)
    line 692 -- createInstance(Ci.nsIBufferedInputStream);
/mail/components/cloudfile/nsYouSendIt.js (View Hg log or Hg annotations)
    line 1074 -- .createInstance(Ci.nsIBufferedInputStream);
/mail/components/cloudfile/nsBox.js (View Hg log or Hg annotations)
    line 857 -- .createInstance(Ci.nsIBufferedInputStream);

Comment 1

[Mike Conley (:mconley) (:⚙️)]

(In reply to Makoto Kato from comment #0)
> I have fixed Gecko side by bug 801483 if nsIBufferedInputStream.init(<too
> large size) causes OOM crashes (ex.
> bp-f7294fd5-e5d4-4c5f-81ea-86b722121012).  So init(<too large size>) in
> Gecko 19 will throw NS_ERROR_OUT_OF_MEMORY instead of crash.
> 
> But, buffer length parameter of init should not use file size because this
> is buffer size for seeking.
> 
> the following code is incorrect usages of nsIBufferedInputStream.
> 
> /mail/components/cloudfile/nsUbuntuOne.js (View Hg log or Hg annotations)
>     line 692 -- createInstance(Ci.nsIBufferedInputStream);
> /mail/components/cloudfile/nsYouSendIt.js (View Hg log or Hg annotations)
>     line 1074 -- .createInstance(Ci.nsIBufferedInputStream);
> /mail/components/cloudfile/nsBox.js (View Hg log or Hg annotations)
>     line 857 -- .createInstance(Ci.nsIBufferedInputStream);

Hey Makoto,

Thanks for letting us know about this.

So what is an appropriate buffer size? I'm seeing 4096 being tossed around here: http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/test/unit/test_file_protocol.js#33

Would that be sufficient?

-Mike

Comment 2

[Makoto Kato [:m_kato]]

(In reply to Mike Conley (:mconley) from comment #1)
> (In reply to Makoto Kato from comment #0)
> > I have fixed Gecko side by bug 801483 if nsIBufferedInputStream.init(<too
> > large size) causes OOM crashes (ex.
> > bp-f7294fd5-e5d4-4c5f-81ea-86b722121012).  So init(<too large size>) in
> > Gecko 19 will throw NS_ERROR_OUT_OF_MEMORY instead of crash.
> > 
> > But, buffer length parameter of init should not use file size because this
> > is buffer size for seeking.
> > 
> > the following code is incorrect usages of nsIBufferedInputStream.
> > 
> > /mail/components/cloudfile/nsUbuntuOne.js (View Hg log or Hg annotations)
> >     line 692 -- createInstance(Ci.nsIBufferedInputStream);
> > /mail/components/cloudfile/nsYouSendIt.js (View Hg log or Hg annotations)
> >     line 1074 -- .createInstance(Ci.nsIBufferedInputStream);
> > /mail/components/cloudfile/nsBox.js (View Hg log or Hg annotations)
> >     line 857 -- .createInstance(Ci.nsIBufferedInputStream);
> 
> Hey Makoto,
> 
> Thanks for letting us know about this.
> 
> So what is an appropriate buffer size? I'm seeing 4096 being tossed around
> here:
> http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/test/unit/
> test_file_protocol.js#33
> 
> Would that be sufficient?

Yes, sufficient. nsXMLHttpRequest uses same size, too.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Does this look alright?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 675232 [details] [diff] [review]
Patch v1

This is a cheap win - do we want it for Aurora and Beta?

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 675232 [details] [diff] [review]
Patch v1

Definitely. a=me.

Comment 6

[Mark Banner (:standard8)]

This was checked in last week:

https://hg.mozilla.org/comm-central/rev/70e854549186

Comment 7

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/a71014233c1f
https://hg.mozilla.org/releases/comm-beta/rev/d0a73868cef8

Comment 8

[Wayne Mery (:wsmwk)]

what might one expect to see in crash signature for this OOM crash?