364112 - After submitting a huge string in a textbox, profile becomes corrupted such that typing into any textbox hangs

Status: VERIFIED FIXED

(Toolkit :: Form Manager, defect)

Description

[Marcel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8.1) Gecko/20061010 Firefox/2.0

On a site, there was an inputfield. Well, in notepad i typed a lot of charachters, millions perhaps. I pasted this line in an input field and posted it to the server. It took so long that the browsers hanged. From that moment, i cannot type in any input field in my profile. On the first charachter, firefox hangs and i am forced to do a ctrl+alt+del. This problem does not happen when i use a new profile

Reproducible: Always

Steps to Reproduce:
1.Paste lots of text in an input field (so no textarea)
2.Post form
3.Restarted firefox
4.Do whatever, a search in google orso. My firefox (profile) hangs

Actual Results:  
Crash, not responding

Comment 1

[Marcel]

Did some more testing
I made a backup of my profile using mozbackup. I checked all except "saved form details". Then resored the backup, and now i dont have any problems now...

Maybe this additional information will help

Comment 3

[Daniel Veditz [:dveditz]]

I don't see how this can block a specific 1.8 release without some traction on a fix. Nominating for the trunk release and moving into the "wanted" bucket for the branch.

You hang on any input field? I was only able to reproduce hanging on input fields with the same name as my testcase (e.g. delay on Google, Yahoo still fine).

Comment 4

[Daniel Veditz [:dveditz]]

Attachment: testcase, induces delay at Google

Comment 5

[Daniel Veditz [:dveditz]]

During the hang stacks tend to look like the following. Mork is growing the buffer 512 bytes at a time until it gets big enough to read the string value, which is going to take 32K allocs until it's big enough for the 16Mb string. I think the hope is to replace Mork, so meanwhile we should put a sane upper bound on saved form data of a couple of K.

>	memset() Line 108	Asm
 	_heap_alloc_dbg() Line 448	C
 	_nh_malloc_dbg() Line 260	C
 	_nh_malloc() Line 209	C
 	operator new() Line 25	C++
 	orkinHeap::Alloc() Line 90	C++
 	morkBlob::GrowBlob() Line 90	C++
 	morkCoil::GrowCoil() Line 164	C++
 	morkSpool::SpillPutc() Line 127	C++
 	morkSink::Putc() Line 132	C++
 	morkParser::ReadValue() Line 938	C++
 	morkParser::ReadCell() Line 587	C++
 	morkParser::ReadRow() Line 678	C++
 	morkParser::ReadTable() Line 789	C++
 	morkParser::ReadContent() Line 1432	C++
 	morkParser::OnPortState() Line 1473	C++
 	morkParser::ParseLoop() Line 1531	C++
 	morkParser::ParseMore() Line 1570	C++
 	morkThumb::DoMore_OpenFileStore() Line 479	C++
 	morkThumb::DoMore() Line 399	C++
 	morkThumb::DoMore() Line 194	C++
 	nsFormHistory::UseThumb() Line 588	C++
 	nsFormHistory::OpenExistingFile() Line 441	C++
 	nsFormHistory::OpenDatabase() Line 381	C++
 	nsFormHistory::AutoCompleteSearch() Line 722	C++
 	nsFormFillController::StartSearch() Line 556	C++
 	nsAutoCompleteController::StartSearch() Line 993	C++
 	nsAutoCompleteController::Notify() Line 688	C++
 	nsTimerImpl::Fire() Line 397	C++
 	nsTimerManager::FireNextIdleTimer() Line 628	C++
 	nsAppShell::Run() Line 141	C++
 	nsAppStartup::Run() Line 151	C++
 	XRE_main() Line 2695	C++
 	main() Line 61	C++
 	mainCRTStartup() Line 398	C

Cure once affected: use clear private data to delete form history.

Comment 6

[Jesse Ruderman]

> Mork is growing the buffer 512 bytes at a time until it gets big enough to read the string value

Reminds me of bug 319004 comment 5.

Comment 7

[Daniel Veditz [:dveditz]]

Apparently you need to quit and restart to get the full effect of the hang, the current 16Mb string version might be far too malicious. (note, mork actually saves the data as escaped UTF-16 so a single 'a' is saved as 'a$00' -- quadrupling the amount it needs to re-re-re-reallocate, 512 bytes at a time.)

If you edit the testcase to set the value to 's' instead of 's16' the resulting hang is bearable. It does happen on the first input field regardless of the site, but once the form data is loaded into history there aren't further delays.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Hmm, was this not fixed by bug 319004? Either way, we use mozstorage for form history on the trunk, so this bug doesn't apply there.

Comment 9

[Daniel Veditz [:dveditz]]

No, it was not fixed by bug 319004. That hacked around the mork problem by limiting title strings specifically; it did not fix the underlying mork issue for other uses of mork. The branch fix I'm proposing here is a similar hack-around in the form-data code. The trunk "fix" is to stop using mork.

Not sure what Thunderbird/mailnews needs to worry about since they're still using mork on the trunk. A malicious vcard with arbitrary sized values? locally-saved IMAP mail?

Comment 10

[Daniel Veditz [:dveditz]]

Attachment: don't save form data that's too large

Prevent the branch DOS by limiting saved form data to 200 characters for the field ID and 2000 for the <input> text. Anything bigger and we simply don't save it. The limits are totally arbitrary, but it's hard to imagine a legit ID being 200 characters, or a 2000-char data string that you'd want to replay in future forms.

For the trunk I've kept the limits even though we now use mozstorage which doesn't have the performance suckage of mork. Large chunks of data still seem useless/malicious and not worth storing.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

I don't think we should arbitrarily limit what we store on the trunk unless we have a good reason to.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 262627 [details] [diff] [review]
don't save form data that's too large

What gavin said, plus  I would rather cut aValue at its maximum length rather than just throwing (for "invalid" name we should probably keep throwing, I guess).

Comment 13

[Daniel Veditz [:dveditz]]

The "good reason to" is because sites can store arbitrarily large amounts of data on disk. Although I guess even with a size limit they can just add more fields, so we'd really need a per-site quota of some sort (much like the cookie and globalStorage quotas). And there's no way to manage form data or even know how much you have other than periodically trashing it all. But OK, I'll leave the trunk alone and let that be a different issue (I'd still like to fix the potential uninitialized "exists" variable).

On the branch I don't see the value of saving truncated data. It's chopped off but you're not going to be able to tell that -- if users see the value as a choice they will naturally assume it's what they entered and could blame us for corrupted data. Much better to simply drop it (this is also the approach we take with overlarge cookies).

If you want larger limits I don't mind. 4K, 8K, 16K, whatever. Uselessly large but not yet close to causing the perf problem.

Comment 14

[Daniel Veditz [:dveditz]]

Attachment: fix mork-based history only

in nsFormHistory.cpp the return value doesn't matter since nothing checks it. Should someone want this in nsStorageFormHistory.cpp I'd probably want to return NS_OK so they just silently drop without bothering the caller.

Comment 15

[Asaf Romano (gone)]

Comment on attachment 262683 [details] [diff] [review]
fix mork-based history only

>Index: toolkit/components/satchel/src/nsFormHistory.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/satchel/src/nsFormHistory.cpp,v
>retrieving revision 1.37
>diff -u -p -8 -r1.37 nsFormHistory.cpp
>--- toolkit/components/satchel/src/nsFormHistory.cpp	18 Jul 2006 00:18:32 -0000	1.37
>+++ toolkit/components/satchel/src/nsFormHistory.cpp	24 Apr 2007 20:54:18 -0000
>@@ -68,16 +68,20 @@ static void SwapBytes(PRUnichar* aDest, 
>     PRUnichar aChar = *aSrc++;
>     *aDest++ = (0xff & (aChar >> 8)) | (aChar << 8);
>   }
> }
> 
> #define PREF_FORMFILL_BRANCH "browser.formfill."
> #define PREF_FORMFILL_ENABLE "enable"
> 
>+// upper bounds on saved form data, more isn't useful.
>+#define FORMFILL_NAME_MAX_LEN  200
>+#define FORMFILL_VALUE_MAX_LEN 2000
>+

Sold, just increase those a bit (esp. the former); r=mano.

Comment 16

[Daniel Veditz [:dveditz]]

Attachment: 1.8/1.8.0 branch patch (lengths changed per review comments)

Comment 17

[Daniel Veditz [:dveditz]]

Attachment: ignore -- double submit

Comment 18

[Daniel Veditz [:dveditz]]

Checked into trunk

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 263092 [details] [diff] [review]
1.8/1.8.0 branch patch (lengths changed per review comments)

approved for 1.8.0.12 and 1.8.1.4, a=release-drivers

Comment 20

[Daniel Veditz [:dveditz]]

fixed on 1.8 and 1.8.0 branches

Comment 22

[Hasham]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1pre) Gecko/2008062306 GranParadiso/3.0.1pre

Verified.