Bug 146129 – CheckedArithmetic's operator bool() and operator==() is broken.

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Mark Lam]

The existing operator UnspecifiedBoolType*() in CheckedArithmetic is erroneously allowing the Checked value to be implicitly casted into any integer and pointer types.  This is because it is doing a reinterpret_cast<UnspecifiedBoolType*>(1) whereas the idiom relies on the cast of a member e.g. reinterpret_cast<UnspecifiedBoolType*>(&Checked::m_value).  As a result, ImageBufferData::putData() was getting an implicit cast of a Checked value to 1.

Also, 2 of CheckedArithmetic's operator==() will crash if used on an overflowed value, while a 3rd one does not.  The 3rd one should be consistent and also crash if used on an overflowed Checked value.

[Mark Lam]

Created attachment 255152 [details]
the patch.

[Mark Lam]

FYI, this patch has already been tested locally with the layout tests, jsc tests, and api tests, all with a debug build.  No issues were found.

[Geoffrey Garen]

Comment on attachment 255152 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=255152&action=review

r=me

> Source/WTF/ChangeLog:13
> +        reinterpret_cast<UnspecifiedBoolType*>(&Checked::m_value).  As a result,
> +        ImageBufferData::putData() was getting an implicit cast of a Checked value to 1.

I don't see any change to ImageBufferData::putData in this patch, and I don't see a regression test for ImageBuffer. What gives?

[Mark Lam]

(In reply to comment #3)
> Comment on attachment 255152 [details]
> the patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255152&action=review
> 
> r=me
> 
> > Source/WTF/ChangeLog:13
> > +        reinterpret_cast<UnspecifiedBoolType*>(&Checked::m_value).  As a result,
> > +        ImageBufferData::putData() was getting an implicit cast of a Checked value to 1.
> 
> I don't see any change to ImageBufferData::putData in this patch, and I
> don't see a regression test for ImageBuffer. What gives?

We talked offline, but I'll document the details here in case anyone else is interested.

The case of bad implicit casting in ImageBufferData::putData() is now prevented by the use of explicit operator bool().  ImageBufferData::putData() is doing an "if (width <= 0 || height <= 0)" statement where width and height are of type Checked<int>.  We don't need to change ImageBufferData::putData() because the new Checked::operator<=() does the right thing for it automatically.

I didn't try to create a test for ImageBufferData::putData() because I don't know how to jump through all the hoops to get a width and height value into that code path in platform/graphics/cg/ImageBufferDataCG.cpp to exercise that specific comparison (that was previously doing the bad cast).  The issue was detected at compile time by the switch to Checked::operator bool().  With the new patch, the compile time error is resolved.

[Oliver Hunt]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 255152 [details]
> > the patch.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=255152&action=review
> > 
> > r=me
> > 
> > > Source/WTF/ChangeLog:13
> > > +        reinterpret_cast<UnspecifiedBoolType*>(&Checked::m_value).  As a result,
> > > +        ImageBufferData::putData() was getting an implicit cast of a Checked value to 1.
> > 
> > I don't see any change to ImageBufferData::putData in this patch, and I
> > don't see a regression test for ImageBuffer. What gives?
> 
> We talked offline, but I'll document the details here in case anyone else is
> interested.
> 
> The case of bad implicit casting in ImageBufferData::putData() is now
> prevented by the use of explicit operator bool(). 
> ImageBufferData::putData() is doing an "if (width <= 0 || height <= 0)"
> statement where width and height are of type Checked<int>.  We don't need to
> change ImageBufferData::putData() because the new Checked::operator<=() does
> the right thing for it automatically.

Sorry i don't understand this. The whole purpose of the horrifying member pointer insanity was prevent that kind of implicit conversion. Does that no longer work? If so we need to go through everything that uses that trick and fix them.

[Mark Lam]

(In reply to comment #5)
> > The case of bad implicit casting in ImageBufferData::putData() is now
> > prevented by the use of explicit operator bool(). 
> > ImageBufferData::putData() is doing an "if (width <= 0 || height <= 0)"
> > statement where width and height are of type Checked<int>.  We don't need to
> > change ImageBufferData::putData() because the new Checked::operator<=() does
> > the right thing for it automatically.
> 
> Sorry i don't understand this. The whole purpose of the horrifying member
> pointer insanity was prevent that kind of implicit conversion. Does that no
> longer work? If so we need to go through everything that uses that trick and
> fix them.

The old approach (before we have the ability to use an explicit operator bool()) was to rely on the fact that C++ does not implicitly casts addresses of class members to other int / pointer types.  Hence, the following pattern would work as expected at preventing implicit casts into int types:   

    typedef T* (RefPtr::*UnspecifiedBoolType);
    operator UnspecifiedBoolType() const { return m_ptr ? &RefPtr::m_ptr : nullptr; }

However, CheckedArithmetic was not doing this instead:

    typedef void* (Checked::*UnspecifiedBoolType);
    operator UnspecifiedBoolType*() const
    {
        ...
        return (m_value) ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0;
    }

Note that it was not using the address of a class member for the cast.  It was using the literal 1.  When I changed it to reinterpret_cast<UnspecifiedBoolType*>(&Checked::m_value), we started seeing the compile time error.  Ditto for replacing this cast operator with explicit operator bool().

[Mark Lam]

(In reply to comment #6)
> However, CheckedArithmetic was not doing this instead:

typo: "CheckedArithmetic was doing this instead:"

>         return (m_value) ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0;

[Oliver Hunt]

(In reply to comment #7)
> (In reply to comment #6)
> > However, CheckedArithmetic was not doing this instead:
> 
> typo: "CheckedArithmetic was doing this instead:"
> 
> >         return (m_value) ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0;

We use the cast (1) in many places as well - C++ behaviour is spec'd on the type of the return value, not the value that is returned from that function. A calcite is not allowed to make a semantic change on defined behaviour.

Hence we get back to the question: why was this not failing before? The boolean comparison should not have worked.

[Mark Lam]

(In reply to comment #8)
> We use the cast (1) in many places as well - C++ behaviour is spec'd on the
> type of the return value, not the value that is returned from that function.
> A calcite is not allowed to make a semantic change on defined behaviour.

You are right.  We do use it in several places.  So those should be fixed.

Here's a small test to compare the behavior of the 2 idioms:

1. casting &member:
   ===============
    template <typename T>
    class Clazz {
    public:
        Clazz(T value) : m_value(value) { }

        typedef T (Clazz::*UnspecifiedBoolType);
        operator UnspecifiedBoolType() const { return m_value ? &Clazz::m_value : 0; }

        T m_value;
    };


    int main(int argc, const char** argv) {
        Clazz<int> c1(0x1000);
        Clazz<int> c2(0);

        // int val = c1;    // This causes a compile time error with both approaches.
        if (c1 < 0)
            printf("Path taken: c1 < 0\n");
        if (c1 > 0)
            printf("Path taken: c1 > 0\n");
        if (c1 < (void*)10)
            printf("Path taken: c1 < 10\n");
        if (c1 > (void*)10)
            printf("Path taken: c1 > 10\n");

        if (c1)
            printf("Path taken: c1\n");

        if (c2)
            printf("Path taken: c2\n");
    }

    Output:
    ======
    test.cpp:25:16: error: invalid operands to binary expression ('Clazz<int>' and 'int')
            if (c1 < 0)
                ~~ ^ ~
    test.cpp:27:16: error: invalid operands to binary expression ('Clazz<int>' and 'int')
            if (c1 > 0)
                ~~ ^ ~
    test.cpp:29:16: error: invalid operands to binary expression ('Clazz<int>' and 'void *')
            if (c1 < (void*)10)
                ~~ ^ ~~~~~~~~~
    test.cpp:31:16: error: invalid operands to binary expression ('Clazz<int>' and 'void *')
            if (c1 > (void*)10)
                ~~ ^ ~~~~~~~~~
    4 errors generated.


2. casting literal 1:
   =================
    template <typename T>
    class Clazz {
    public:
        Clazz(T value) : m_value(value) { }

        typedef void* (Clazz::*UnspecifiedBoolType);
        operator UnspecifiedBoolType*() const { return m_value ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0; }

        T m_value;
    };


    int main(int argc, const char** argv) {
        Clazz<int> c1(0x1000);
        Clazz<int> c2(0);

        // int val = c1;    // This causes a compile time error with both approaches.
        if (c1 < 0)
            printf("Path taken: c1 < 0\n");
        if (c1 > 0)
            printf("Path taken: c1 > 0\n");
        if (c1 < (void*)10)
            printf("Path taken: c1 < 10\n");
        if (c1 > (void*)10)
            printf("Path taken: c1 > 10\n");

        if (c1)
            printf("Path taken: c1\n");

        if (c2)
            printf("Path taken: c2\n");
    }

    Output:
    ======
    $ ./test.o 
    Path taken: c1 > 0
    Path taken: c1 < 10
    Path taken: c1

Hence, the casting of literal 1 approach is not quite giving us the same thing.

[Mark Lam]

Oliver, if you agree with the correctness of this patch, I would like to land it.  I will file a separate bug later to fix all the other places that casts literal 1.

[Mark Lam]

Here's another example that illustrates why the reinterpret_cast<UnspecifiedBoolType*>(1) approach is bad:

        Clazz<int> c1(0x1000);
        void* ptr = c1 + 1;

Basically, we can still do pointer math with it, whereas the &Clazz::m_value approach yields a compile time error.

[Mark Lam]

Comment on attachment 255152 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=255152&action=review

>>>> Source/WTF/ChangeLog:13
>>>> +        ImageBufferData::putData() was getting an implicit cast of a Checked value to 1.
>>> 
>>> I don't see any change to ImageBufferData::putData in this patch, and I don't see a regression test for ImageBuffer. What gives?
>> 
>> We talked offline, but I'll document the details here in case anyone else is interested.
>> 
>> The case of bad implicit casting in ImageBufferData::putData() is now prevented by the use of explicit operator bool().  ImageBufferData::putData() is doing an "if (width <= 0 || height <= 0)" statement where width and height are of type Checked<int>.  We don't need to change ImageBufferData::putData() because the new Checked::operator<=() does the right thing for it automatically.
>> 
>> I didn't try to create a test for ImageBufferData::putData() because I don't know how to jump through all the hoops to get a width and height value into that code path in platform/graphics/cg/ImageBufferDataCG.cpp to exercise that specific comparison (that was previously doing the bad cast).  The issue was detected at compile time by the switch to Checked::operator bool().  With the new patch, the compile time error is resolved.
> 
> Sorry i don't understand this. The whole purpose of the horrifying member pointer insanity was prevent that kind of implicit conversion. Does that no longer work? If so we need to go through everything that uses that trick and fix them.

I'll update the comment to say "implicit cast of a Checked value to (void*)1 and doing incorrect pointer comparisons on it."

[Oliver Hunt]

Ok, you should have a follow up bug to correct the existing casts of 1, and also you should probably file a bug on the compiler as that behaviour is incorrect.

[Mark Lam]

Thanks for the reviews.  Landed in r185755: <http://trac.webkit.org/r185755>.

[Csaba Osztrogonác]

(In reply to comment #14)
> Thanks for the reviews.  Landed in r185755: <http://trac.webkit.org/r185755>.

It broke the EFL and GTK build. :-(

[Mark Lam]

(In reply to comment #15)
> (In reply to comment #14)
> > Thanks for the reviews.  Landed in r185755: <http://trac.webkit.org/r185755>.
> 
> It broke the EFL and GTK build. :-(

Too bad the EWS has not been working for so long, and was not able to warn about this build failure.  The issue is now fixed in r185764: <http://trac.webkit.org/r185764>.