46898 - potential segfault in nsString.cpp, function ToInteger

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P3)

Description

[Peter "jag" Annema]

In nsString.cpp, function ToInteger, there's a potential for a segfault when 
ToInteger is called on an empty string. Attaching patch which fixes that and 
does some related code clean-up.

Comment 1

[Peter "jag" Annema]

Attachment: [patch] fix for potential segfault on "" in ToInteger

Comment 2

[Peter "jag" Annema]

And accepting it!

r=scc, need approval for check-in.

Comment 3

[Chris Waterson]

a=waterson

Comment 4

[Scott Collins]

OK, I'll take it; checkin jag's patch; and close the bug when I'm done.

Comment 5

[Scott Collins]

fix checked in with version 3.127 of nsString.cpp

Comment 6

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

Please, let nsString::ToInteger return an error when called on an empty string.
Just returning zero is sometimes OK, but I think it should be up to client code
to interpret empty strings. BTW, I filed bug 42930 on this some time ago...

Comment 7

[Peter "jag" Annema]

Hmmm, if client code should interpret empty strings, shouldn't it be doing so 
through str.IsEmpty() or str.Length()?

Aside from that, I think you're right that an error should be returned. As a 
matter of fact, I missed another case in my patch, for example "zyx" in base 10. 
In that case *anErrorCode gets set to NS_OK too, which really doesn't seem 
right, though that is what happened in the original code. Your patches touched 
those cases too.

Moving |*anErrorCode = NS_OK;| to inside the |if (done)| makes the code behave a 
bit more logical. Since this bug was fixed, I'll attach a patch to bug 42930.

Comment 8

[Scott Collins]

need to apply this fix to |nsString| as well

Comment 9

[Scott Collins]

jag also supplies a fix for problems relating to correctly interpreting the radix

Comment 10

[Scott Collins]

Attachment: more fixes for |ToInteger| from jag

Comment 11

[Scott Collins]

your new patches are checked in, jag

Comment 12

[Doron Rosenberg (IBM)]

jag - could you verify this?

Comment 13

[Peter "jag" Annema]

Doh. Fixed!