42930 - Assertions when attributes are empty strings (really a String bug)

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I hit an assert in nsGenericHTMLElement::ParseValueOrPercent, calling tmp.Last() 
when tmp (the attribute value) is an empty string. But this code is only called 
when tmp.ToInteger returns an error code of NS_OK. A comment in 
nsString::ToInteger says
     //if you don't have any valid chars, return 0, but set the error;
So, I think tmp.ToInteger should have returned an error, but didn't.

Walking through the code, I can see why. The code looks like this:

    char*  endcp=cp+mLength;
...
    while((cp<endcp) && (!done)){
...
    }
...
      //if you don't have any valid chars, return 0, but set the error;
    if(cp<=endcp) {
      *anErrorCode = NS_OK;
...

Here, mLength == 0 so endcp == cp. There are no iterations of the while loop so 
cp never advances, so the invariant that "cp is the address of the first valid 
character plus one" is never established. So we enter the if statement and the 
error state is cleared, never to be set again.

I just changed the test to "if(done)" and everything works fine.

nsCString::ToInteger has the same bug. SVToInteger has a similar bug. (I'm not 
sure which of these functions are actually used or going to be used in the 
future...)

I will attach a testcase and a patch.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Patches to nsString(2); tested and working

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Proposed patch to SVToInteger (nsBufferManager.h) (not tested)

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Testcase causing assertion in Viewer (and Mozilla)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd assign this to a String person, but I don't know who's the right one.

It seems to me that if you got very unlucky and your string had zero length but 
the first (uninitialized) character happened to be a digit, ToInteger would 
return the value of the digit. That sounds Bad.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, since the buffer is null-terminated, the existing code will always return 0 
(NS_OK) for empty strings. Still, it should return an error.

Comment 6

[Warren Harris]

=> scc

Comment 7

[Peter "jag" Annema]

What about this change in nsGenericHTMLElement::ParseValueOrPercent (and 
friends)?

 tmp.CompressWhitespace(PR_TRUE, PR_TRUE);
+
+if (tmp.IsEmpty()) return PR_FALSE;
+
 PRInt32 ec, val = tmp.ToInteger(&ec);
 if (NS_OK == ec) {
   if (val < 0) val = 0; 
-  if (tmp.Length() && tmp.Last() == '%') {/* XXX not 100% compatible with 
ebina's code */
+  if (tmp.Last() == '%') {/* XXX not 100% compatible with ebina's code */
     if (val > 100) val = 100;

But as per bug 46898 I'll attach a patch for nsString.cpp which makes the emptry 
string case dealt with by ToInteger so the |if (tmp.IsEmpty()) return PR_FALSE;| 
becomes redundant. Basically the patch applied for (bug 46898 + this patch) == 
(Robert's patch(es) + an extra s/cp<=endcp/cp<endcp/)

This is only a patch for nsString.cpp currently, as soon as I get CVS set up 
somewhere I'll try to create patches for nsString2.cpp and nsBufferManager.h 
(included in nsStringx.cpp, nsString2x.cpp and nsStringImpl.h, nice forest of 
strings), but if anyone feels like beating me to it ... ;-)

Comment 8

[Peter "jag" Annema]

Attachment: [patch] nsString.cpp; set errorcode if no integer found

Comment 9

[Scott Collins]

Don't bother patching nsBufferManager, the ...Stringx, or the ...StringImpl files 
as they are unused and due to be removed soon.  They were an experiment that was 
never completed.

Comment 10

[Scott Collins]

jag's patch is checked in now (as per related bug #46898)

Comment 11

[Tracy Walker [:tracy]]

please verify

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yep, looks good. Thanks!!