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.
Created attachment 387076 [details] Patch
Created attachment 387078 [details] Patch
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.
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);
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.
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.
Created attachment 387603 [details] Patch
(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.
(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.
(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.
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.
Created attachment 387617 [details] Patch
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.
Created attachment 387885 [details] Patch
Comment on attachment 387885 [details] Patch Clearing flags on attachment: 387885 Committed r254678: <https://trac.webkit.org/changeset/254678>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58643165>
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&)
(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.
Committed r254720: <https://trac.webkit.org/changeset/254720>
Reverted. Thank you, Truitt and Jonathan.
Reopening to attach new patch.
Created attachment 388199 [details] Patch
Remove all invalid type decoding tests not to crash on mac port.
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.
Created attachment 388206 [details] Patch
(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.
Comment on attachment 388206 [details] Patch Clearing flags on attachment: 388206 Committed r254811: <https://trac.webkit.org/changeset/254811>