400295 - set reasonable upper bounds on string buffer sizes

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: proposed patch v1 - set an upper bound

Mozilla core string code should just fail, rather than seriously attempt
to honor, requests to allocate string buffers that are tens or hundreds 
of megabytes in size.  Better the silly allocation should fail, than to 
render the entire process unusable.  Patch attached.

There's a bug seen in trunk Mozilla mailnews clients on startup.
They allocate and reallocate string buffers whose sizes are tens or hundreds 
of megabytes in length, which is way beyond reason for what they're doing.  
These allocations cause the VM space for the process to grow to ridiculous 
sizes (over 1 GB), and for huge amounts of TIME to be spent copying memory 
(in realloc), all because the VM system tolerates these absurd allocations.  
They render the whole process unusable.  IMO, it is better for the part of
the process that is trying to allocate silly amounts of memory to fail,
than for the entire process to become unusable.

The relevant mozilla allocation functions are:
nsStringBuffer::Alloc
nsStringBuffer::Realloc

IMO, these functions should set upper bounds on the amount they will 
allocate, and simply fail if the caller asks for more. This will at least
keep the entire process running in a reasonable amount of VM space. 
It will cause the negative effects of the bad code to be somewhat more
localized, to just the bad code itself, and not to the entire process.

The proposed patch won't fix the bad code that does the silly allocations,
but it is good for the process (and the user) overall.

Comment 1

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

Comment on attachment 285385 [details] [diff] [review]
proposed patch v1 - set an upper bound

+      return (nsStringBuffer *)NULL;          // Just say No!

should be return nsnull; (in particular, no cast)


This is probably reasonable, however I don't think most callers check whether the allocation succeeded, so I somewhat fear that this might increase the risk that callers write beyond the end of the buffer...

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Seems to me like a bad idea to enforce arbitrary limits in the string code; better to fix callers that shouldn't be using strings not to do so.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

David,  How often do legitimate 200MB strings occur?

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Somebody could potentially write something that reads a file into a string and then does something with it, all using O(N) algorithms.  Should we really prevent them from doing that?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

In browser code?  
A browser would legitimately need to read a 50MB file into memory as a string?
What's more important here?  Absurdly improbably what-ifs? 
or mitigating the effects of absurd code bugs on running processes, bugs like
the real bug that motivated this patch? (bug 398961)

Comment 6

[Jesse Ruderman]

50MB files are not absurdly improbable.