[Webkit-unassigned] [Bug 205902] KeyedDecoderGeneric crashes when it accesses data with non-existing key

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=205902

--- Comment #8 from Takashi Komori <Takashi.Komori at sony.com> ---
(In reply to Fujii Hironori from comment #3)
> Comment on attachment 387078 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387078&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        KeyedDecoderGeneric craches when it acesses data with non-existing key.
> 
> acesses → accesses

Fixed.

> > Source/WebCore/platform/generic/KeyedDecoderGeneric.cpp:46
> > +    bool exists(const String& key) { return !!m_map.get(key); }
> 
> It's inefficient to call 'm_map.get(key)' twice.
> Make 'get' method return Node*.
> 
> Node* get(const String& key) { return m_map.get(key); }
> 
> > Source/WebCore/platform/generic/KeyedDecoderGeneric.cpp:193
> > +    return WTF::get_if<T>(dictionary->get(key));
> 
> auto node = dictionary->get(key)
> if (!node)
>   return nullptr;
> return WTF::get_if<T>(node);

Fixed.

> > Source/WebCore/platform/generic/KeyedDecoderGeneric.cpp:197
> > +bool KeyedDecoderGeneric::getValueFromDictionaryStack(const String& key, T& result)
> 
> I think decodeSimpleValue is a better name. KeyedDecoderGlib is using the
> name.

Changed.

> > Tools/TestWebKitAPI/PlatformWin.cmake:65
> > +        Tests/WebCore/KeyedCodingGeneric.cpp
> 
> I think this tests should be run on all ports.
> Rename KeyedCodingGeneric.cpp KeyedCoding.cpp.
> Add KeyedCoding.cpp for all ports.

Moved to Tools/TestWebKitAPI/CMakeLists.txt but I can't test other ports...

> > Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:42
> > +            return false;
> 
> Let's use std::memcmp.

Fixed.

> > Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:48
> > +TEST(KeyedCodingGeneric, SetAndGetBytes)
> 
> KeyedCodingGeneric → KeyedCoding

Moved.

> > Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:51
> > +    const size_t dataLength = sizeof(data) / sizeof(uint8_t);
> 
> Use WTF_ARRAY_LENGTH.

Fixed.

> > Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:67
> > +    result = decoder->decodeBytes("data", buffer, size);
> 
> Rename 'result' to 'ok' or 'success'.

Changed to 'success'.

> > Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:133
> > +    EXPECT_EQ(INT32_MAX, value);
> 
> I think you should add one more test case checking unavailable to decode
> int32 as other type, for example, bool.
>     ok = decoder->decodeBool("zero", value);
>     EXPECT_FALSE(ok);

Added tests which try to decode invalid type. 

> > Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:263
> > +    encoder->encodeDouble("double-max", DBL_MAX);
> 
> I think std::numeric_limits<double>::min() and
> std::numeric_limits<double>::max() are better in C++ code.

Fixed.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200114/a1cf3bfd/attachment.htm>