551685 - left and right sub-expressions are identical in stringobject.cpp(1267)

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Trevor Baker]

stringobject.cpp(1267) 

else if ((len > 0xffffff) || (len > 0xffffff)) // might overflow - use doubles

Comment 1

[William Maddox]

Attachment: Fix integer overflow tests in String::substr and String::_substr

The length of a String object is maintained as an int32_t, which agrees with ecmascript semantics.  Elsewhere in the code, I see checks which presume that the length of a string is <= 0x7fffffff, which is the largest (positive) int32 value. This patch is my best guess at what the orginal author was trying to do, guarding against overflow during calculations on string indices by computing as integral-valued doubles instead.  The original code did not make sense to me, and the duplicated test that is the subject of this bug report makes it clear that it was not well inspected or tested.  The patch builds and passes the acceptance test suite, but I am certain that there is no coverage of the interesting paths there.  In an attempt to write a directed test, I found that
the largest string I could create was (2^30)-1 characters in length, as larger allocations failed with an allocation error (MacOS X 64-bit, 12GB RAM).
Calls via String::AS3_substr() clamp the arguments at the string length, so it
is not possible to pass in a larger index.

Comment 2

[William Maddox]

The effect of the erroneous line in Trevor's description above is twofold:

1) Double-precision may be used unnecessarily because the limit value
   is needlessly small, but correctness should not be affected.

2) Because the value of the other argument, start, is not tested, it is
   possible that an overflow might occur if start were sufficiently large.
   In fact, start may never take on such a value as long as the allocator
   limit that I observed continues to hold, as it is clamped to the
   string length.

For reference, here is the code under discussion:

    Stringp String::substr(int32_t start, int32_t len)
    {
        if (start < 0)
            start = 0;
        else if (start > m_length)
            start = m_length;

        int32_t end;
        if (len >= 0x7ffffff)
            end = 0x7fffffff; 
        else if ((len > 0xffffff) || (len > 0xffffff))
            // might overflow - use doubles
            end = int32_t(double(len) + double(start)); 
        else
            end = start + len;

        end = (int32_t) NativeObjectHelpers::ClampIndexInt(end, m_length); 

        return substring(start, end);
    }

Comment 3

[William Maddox]

Attachment: A test case to illustrate another related issue with String::substr()

The previous lines are also buggy, i.e:

        if (len >= 0x7ffffff)
            end = 0x7fffffff; 

If the len argument is greater than (2^28)-1, it is arbitrarily increased to the effective end of the string.  See the attached test case.

Comment 4

[Edwin Smith]

If you need someone to push this, I nominate Krzysztof who should have minted commiter rights, and has also dabbled in the String code.

Comment 5

[William Maddox]

Pushed 2010-03-17 by Rob Winchell <[e-mail]>
changeset 4064	d82019521f3e
http://hg.mozilla.org/tamarin-redux/rev/d82019521f3e