Bug 205902 – KeyedDecoderGeneric crashes when it accesses data with non-existing key

• Component: Platform
• Status: RESOLVED FIXED

[Takashi Komori]

When we try to decode encoded data with non-existing key, the decoder crashes.
This is because when KeyedDecoderGeneric decodes with non-existing key, it accesses nullptr.

[Takashi Komori]

Created attachment 387076 [details]
Patch

[Takashi Komori]

Created attachment 387078 [details]
Patch

[Fujii Hironori]

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

> 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);

> 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.

> 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.

> Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:42
> +            return false;

Let's use std::memcmp.

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

KeyedCodingGeneric → KeyedCoding

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

Use WTF_ARRAY_LENGTH.

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

Rename 'result' to 'ok' or '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);

> 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.

[Fujii Hironori]

Comment on attachment 387078 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:108
> +    encoder->encodeInt32("int32-max", INT32_MAX);

I think you can simplify these tests by add a template function.

testSimpleValue(&WebCore::KeyedEncoder::encodeInt64, &WebCore::KeyedDecoder::decodeInt64, -2);
testSimpleValue(&WebCore::KeyedEncoder::encodeFloat, &WebCore::KeyedDecoder::decodeFloat, -1.5);

[Fujii Hironori]

Comment on attachment 387078 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:432
> +    EXPECT_EQ(users.size(), decodedUsers.size());

EXPECT_EQ(users, decodedUsers);

Is this possible?
If so, EXPECT_EQ(users.size(), decodedUsers.size()) is not needed.

[Fujii Hironori]

Comment on attachment 387078 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:108
>> +    encoder->encodeInt32("int32-max", INT32_MAX);
> 
> I think you can simplify these tests by add a template function.
> 
> testSimpleValue(&WebCore::KeyedEncoder::encodeInt64, &WebCore::KeyedDecoder::decodeInt64, -2);
> testSimpleValue(&WebCore::KeyedEncoder::encodeFloat, &WebCore::KeyedDecoder::decodeFloat, -1.5);

But, I am wondering round trip tests are really needed because it transfers the encoding and decoding tasks to PersistentEncoder.

[Takashi Komori]

Created attachment 387603 [details]
Patch

[Takashi Komori]

(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.

[Takashi Komori]

(In reply to Fujii Hironori from comment #4)
> Comment on attachment 387078 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387078&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:108
> > +    encoder->encodeInt32("int32-max", INT32_MAX);
> 
> I think you can simplify these tests by add a template function.
> 
> testSimpleValue(&WebCore::KeyedEncoder::encodeInt64,
> &WebCore::KeyedDecoder::decodeInt64, -2);
> testSimpleValue(&WebCore::KeyedEncoder::encodeFloat,
> &WebCore::KeyedDecoder::decodeFloat, -1.5);

Added template function testSimpleValue.

[Takashi Komori]

(In reply to Fujii Hironori from comment #5)
> Comment on attachment 387078 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387078&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:432
> > +    EXPECT_EQ(users.size(), decodedUsers.size());
> 
> EXPECT_EQ(users, decodedUsers);
> 
> Is this possible?
> If so, EXPECT_EQ(users.size(), decodedUsers.size()) is not needed.

Changed to compare users and decodedUsers directly.

[Fujii Hironori]

Comment on attachment 387603 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebCore/KeyedCoding.cpp:80
> +template <typename EncodeValueType, typename DecodeValueType> typename std::enable_if<std::is_same<EncodeValueType, DecodeValueType>::value, bool>::type testSimpleValueDecode(bool decodeSuccess, EncodeValueType expected, DecodeValueType decoded)

Use is_same_v instead of is_same.
Use constexpr if instead of SFINAE.

[Fujii Hironori]

Created attachment 387617 [details]
Patch

[Takashi Komori]

Created attachment 387780 [details]
Patch

This patch removes decoding number tests by different type number decoder.
Because on mac port KeyedDecoderCF could decode numbers which is encoded by another number type encoder.

[Takashi Komori]

Created attachment 387885 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 387885 [details]
Patch

Clearing flags on attachment: 387885

Committed r254678: <https://trac.webkit.org/changeset/254678>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58643165>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/254678/webkit has broken 11 API tests

Log:
https://build.webkit.org/builders/Apple-Catalina-Debug-WK2-Tests/builds/1735/steps/run-api-tests/logs/stdio

Build:
https://build.webkit.org/builders/Apple-Catalina-Debug-WK2-Tests/builds/1735

Crash example:
    TestWebKitAPI.KeyedCoding.SetAndGetFloat
        ASSERTION FAILED: CFGetTypeID(object) == CFTypeTrait<T>::typeID()
        /Volumes/Data/slave/catalina-debug/build/WebKitBuild/Debug/usr/local/include/wtf/cf/TypeCastsCF.h(70) : T WTF::dynamic_cf_cast(CFTypeRef) [T = const __CFBoolean *]
        1   0x115b15d39 WTFCrash
        2   0x115b15d59 WTFCrashWithSecurityImplication
        3   0x12a72a107 __CFBoolean const* WTF::dynamic_cf_cast<__CFBoolean const*>(void const*)
        4   0x12a72a048 WebCore::KeyedDecoderCF::decodeBool(WTF::String const&, bool&)

[Jonathan Bedard]

(In reply to Truitt Savell from comment #18)
> It looks like the changes in https://trac.webkit.org/changeset/254678/webkit
> has broken 11 API tests
> 
> Log:
> https://build.webkit.org/builders/Apple-Catalina-Debug-WK2-Tests/builds/1735/
> steps/run-api-tests/logs/stdio
> 
> Build:
> https://build.webkit.org/builders/Apple-Catalina-Debug-WK2-Tests/builds/1735
> 
> Crash example:
>     TestWebKitAPI.KeyedCoding.SetAndGetFloat
>         ASSERTION FAILED: CFGetTypeID(object) == CFTypeTrait<T>::typeID()
>        
> /Volumes/Data/slave/catalina-debug/build/WebKitBuild/Debug/usr/local/include/
> wtf/cf/TypeCastsCF.h(70) : T WTF::dynamic_cf_cast(CFTypeRef) [T = const
> __CFBoolean *]
>         1   0x115b15d39 WTFCrash
>         2   0x115b15d59 WTFCrashWithSecurityImplication
>         3   0x12a72a107 __CFBoolean const* WTF::dynamic_cf_cast<__CFBoolean
> const*>(void const*)
>         4   0x12a72a048 WebCore::KeyedDecoderCF::decodeBool(WTF::String
> const&, bool&)

Those look like the API tests added in this patch:

https://results.webkit.org/?suite=api-tests&suite=api-tests&suite=api-tests&suite=api-tests&suite=api-tests&suite=api-tests&suite=api-tests&test=TestWebKitAPI.KeyedCoding.SetAndGetUInt32&test=TestWebKitAPI.KeyedCoding.SetAndGetUInt64&test=TestWebKitAPI.ProcessSwap.QuickLookRequestsPasswordAfterSwap&test=TestWebKitAPI.UIPasteboardTests.DataTransferGetDataWhenPastingURL&test=TestWebKitAPI.UIPasteboardTests.PasteWithCompletionHandler&test=TestWebKitAPI.WKWebView.IsOpaqueNoDecodedFromArchive&test=TestWebKitAPI.WKWebView.PrintFormatterHangsIfWebProcessCrashesBeforeWaiting

I think we should roll this out.

[Fujii Hironori]

Committed r254720: <https://trac.webkit.org/changeset/254720>

[Fujii Hironori]

Reverted. Thank you, Truitt and Jonathan.

[Takashi Komori]

Reopening to attach new patch.

[Takashi Komori]

Created attachment 388199 [details]
Patch

[Takashi Komori]

Remove all invalid type decoding tests not to crash on mac port.

[Fujii Hironori]

Comment on attachment 388199 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebCore/KeyedCoding.cpp:89
> +    if constexpr (std::is_same_v<EncodeValueType, String> && std::is_same_v<DecodeValueType, String>) {

Then, remove this "if constexpr" part

> Tools/TestWebKitAPI/Tests/WebCore/KeyedCoding.cpp:160
> +    EXPECT_TRUE(testSimpleValue(&WebCore::KeyedEncoder::encodeString, &WebCore::KeyedDecoder::decodeString, String()));

I think this test also should be removed.

[Takashi Komori]

Created attachment 388206 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #25)
> Comment on attachment 388199 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388199&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebCore/KeyedCoding.cpp:89
> > +    if constexpr (std::is_same_v<EncodeValueType, String> && std::is_same_v<DecodeValueType, String>) {
> 
> Then, remove this "if constexpr" part

Removed.

> > Tools/TestWebKitAPI/Tests/WebCore/KeyedCoding.cpp:160
> > +    EXPECT_TRUE(testSimpleValue(&WebCore::KeyedEncoder::encodeString, &WebCore::KeyedDecoder::decodeString, String()));
> 
> I think this test also should be removed.

Removed.

[WebKit Commit Bot]

Comment on attachment 388206 [details]
Patch

Clearing flags on attachment: 388206

Committed r254811: <https://trac.webkit.org/changeset/254811>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.