6168 - [CRASH, BLOCK] Submitting a form crashes browser

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Stephen P. Morse]

Go to the indicated URL.  Press the "save" button at the bottom of the form.
Browser crashes with the stack trace shown below.

Note: with the debugger I determined that the value of the encoder argument is
bad.  In fact, this sequence of routines is called many times prior to the
crash, each time with the same value for encoder.  When the call is made that
crashes, the value of encoder is suddenly a different value.

This form is the interview form used by wallet.  So this crash is now blocking
wallet development.

UnicodeToNewBytes(unsigned short * 0x028d8c70, unsigned int 28,
nsIUnicodeEncoder * 0x025f1860) line 610 + 9 bytes
URLEncode(nsString & {...}, nsIUnicodeEncoder * 0x025f1860) line 640 + 29 bytes
nsFormFrame::ProcessAsURLEncoded(int 1, nsString & {...}, nsIFormControlFrame *
0x0271c940) line 876 + 30 bytes
nsFormFrame::OnSubmit(nsFormFrame * const 0x01339ed0, nsIPresContext *
0x026ad9c0, nsIFrame * 0x0271c910) line 459
nsButtonControlFrame::MouseClicked(nsIPresContext * 0x026ad9c0) line 345
nsButtonControlFrame::HandleEvent(nsButtonControlFrame * const 0x0271c910,
nsIPresContext & {...}, nsGUIEvent * 0x0012fccc, nsEventStatus &
nsEventStatus_eIgnore) line 529
PresShell::HandleEvent(PresShell * const 0x01305924, nsIView * 0x0271da20,
nsGUIEvent * 0x0012fccc, nsEventStatus & nsEventStatus_eIgnore) line 2008 + 38
bytes
nsView::HandleEvent(nsView * const 0x0271da20, nsGUIEvent * 0x0012fccc, unsigned
int 28, nsEventStatus & nsEventStatus_eIgnore) line 831
nsViewManager::DispatchEvent(nsViewManager * const 0x01304ac0, nsGUIEvent *
0x0012fccc, nsEventStatus & nsEventStatus_eIgnore) line 1726
HandleEvent(nsGUIEvent * 0x0012fccc) line 67
nsWindow::DispatchEvent(nsWindow * const 0x0271daf4, nsGUIEvent * 0x0012fccc,
nsEventStatus & nsEventStatus_eIgnore) line 410 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012fccc) line 431
nsWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 2880 +
15 bytes
nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 720911, long *
0x0012fe48) line 2242 + 24 bytes
nsWindow::WindowProc(void * 0x0001053c, unsigned int 514, unsigned int 0, long
720911) line 474 + 27 bytes
USER32! 77e71250()

Comment 1

[Stephen P. Morse]

Here's a clue.  The last field on the form that is processed correctly is
"department" under "your business address."  The crash occurs on the very next
field, namely "address line 1".  If I remove that field from the form, the crash
then occurs on the very next field.  No matter how many field I remove following
"company name", the crash always occurs on the next following field.  If all
fields after the "department" field are removed from the form, the crash does
not occur.

Well, there's my work-around.  Until this bug is fixed, I'll be forced to use an
abreviated version of this form that stops after the "department" field.

Comment 2

[karnaze (gone)]

This looks a problem with the recent unicode additions to nsString. To get this
to fail on the 5/13 build, I had to do the following (1) use version 1.68 of
layout/html/forms/nsFormControlFrame.cpp because version 1.69 breaks form
submission and (2) fix a bug in extensions/wallet which I just checked in.

Reassinging to Frank, adding [CRASH, BLOCK] to summary, marking critical and
moving to M6.

Comment 3

[Frank Tang]

It sound some code trash the value of encoder. I will take a look at it.

Comment 4

[Frank Tang]

It crash because the value "encoder" which on stack is trashed by the following
code,

862 #ifdef SingleSignon
863         if ((type == NS_FORM_INPUT_PASSWORD) || (type ==
NS_FORM_INPUT_TEXT)) {
864           if (type == NS_FORM_INPUT_PASSWORD) {
865             type_array[value_cnt] = FORM_TYPE_PASSWORD;
866           } else {
867             type_array[value_cnt] = FORM_TYPE_TEXT;
868           }
869           value_array[value_cnt] =
870             values[0].ToNewCString();
871           name_array[value_cnt] =
872             names[0].ToNewCString();
873           value_cnt++;
874         }
875 #endif

It trash the auto variable "encoder" when value_cnt is 51. Why ? The following
code-

795 #ifdef SingleSignon
796 #define MAX_ARRAY_SIZE 50
797   char* name_array[MAX_ARRAY_SIZE];
798   char* value_array[MAX_ARRAY_SIZE];
799   uint8 type_array[MAX_ARRAY_SIZE];
800   PRInt32 value_cnt = 0;
801 #endif
802
I cannot believe we have such hard code size of array withotu boundary checking
in our code. What a traditional security hole. Reassing this back to. I suggest
you review all your code and make sure there are no hard coded limitation of
array in your code without boundary checking. We are luck that it trash a object
pointer here , if it trash some data, it could be difficult to find.

Comment 5

[Stephen P. Morse]

What can I say -- I have egg all over my face.  Yes, I forgot to do an array
boundary check.  So I introduced my own blocking bug!

All this array stuff will go away anyhow once single singon is separated from
layout.  But in the meantime I have added the bounds check.  File involved is
nsFormFrame.cpp.  It's checked in now

Comment 6

[Chris Petersen]

Fixed in 5/17 Biuld.