- Revision
- 276510
- Author
- da...@apple.com
- Date
- 2021-04-23 12:06:25 -0700 (Fri, 23 Apr 2021)
Log Message
Remove decoder memory allocations based on untrusted data (sizes) in the stream; related changes
https://bugs.webkit.org/show_bug.cgi?id=224984
Reviewed by Sam Weinig.
Source/WebCore:
* platform/network/cf/CertificateInfoCFNet.cpp:
(WTF::Persistence::decodeCFData): Removed unneeded check for zero size. Removed code that
locally allocates a vector before bufferIsLargeEnoughToContain is called. Instead use
bufferPointerForDirectRead, which makes does the buffer size check, and pass the pointer
directly to CFDataCreate.
Source/WebKit:
* Platform/IPC/ArgumentCoders.h: Remove the calls to
HashMap::reserveInitialCapacity and HashSet::reserveInitialCapacity, based
on number read in from the decoder. This means there will be more wasted
memory in these HashMap and HashSet objects, so we have to test to make
sure this does not create a performance problem. But without this check,
we are trying to allocate memory based on an unstrusted size.
* Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:
(IPC::ArgumentCoder<RefPtr<ApplePayError>>::encode): Removed the coder
for a Vector of these RefPtr, replaced it with a coder for an individual one,
allowing the Vector ArgumentCoder template to handle vector size and construction.
One benefit is that this adds in a shrinkToFit and prevents us from making any
separate mistake about pre-sizing the Vector here since we use shared code.
(IPC::ArgumentCoder<RefPtr<ApplePayError>>::decode): Ditto.
* Shared/WebCoreArgumentCoders.cpp:
(IPC::ArgumentCoder<RefPtr<SecurityOrigin>>::encode): Ditto.
(IPC::ArgumentCoder<RefPtr<SecurityOrigin>>::decode): Ditto.
(IPC::ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::encode):
(IPC::ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::decode):
Removed unnecessary specialization for the KeyStatusVector. There is already
an ArgumentCoder for Vector, for std::pair, for Ref<SharedBuffer>, and for
enumerations like CDMKeyStatus, so there's no need to have a specialized
coder for this. This function that we are removing had a call to
reserveInitialCapacity, but the Vector ArgumentCoder template does not.
* Shared/WebCoreArgumentCoders.h: Replaced the
ArgumentCoder<Vector<RefPtr<WebCore::ApplePayError>>> specialization with
ArgumentCoder<RefPtr<WebCore::ApplePayError>>. Removed the
ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector> specialization.
Source/WTF:
* wtf/persistence/PersistentDecoder.cpp:
(WTF::Persistence::Decoder::bufferPointerForDirectRead): Added.
(WTF::Persistence::Decoder::decodeFixedLengthData): Refactor to use bufferPointerForDirectRead.
* wtf/persistence/PersistentDecoder.h: Added bufferPointerForDirectRead function for use in the
rare cases where we want to read directly out of the decoder buffer, rather than writing to a
passed-in pointer. Also did a small refactoring of bufferIsLargeEnoughToContain to use &&
rather than an if statement.
Modified Paths
Diff
Modified: trunk/Source/WTF/ChangeLog (276509 => 276510)
--- trunk/Source/WTF/ChangeLog 2021-04-23 18:57:30 UTC (rev 276509)
+++ trunk/Source/WTF/ChangeLog 2021-04-23 19:06:25 UTC (rev 276510)
@@ -1,3 +1,19 @@
+2021-04-23 Darin Adler <da...@apple.com>
+
+ Remove decoder memory allocations based on untrusted data (sizes) in the stream; related changes
+ https://bugs.webkit.org/show_bug.cgi?id=224984
+
+ Reviewed by Sam Weinig.
+
+ * wtf/persistence/PersistentDecoder.cpp:
+ (WTF::Persistence::Decoder::bufferPointerForDirectRead): Added.
+ (WTF::Persistence::Decoder::decodeFixedLengthData): Refactor to use bufferPointerForDirectRead.
+
+ * wtf/persistence/PersistentDecoder.h: Added bufferPointerForDirectRead function for use in the
+ rare cases where we want to read directly out of the decoder buffer, rather than writing to a
+ passed-in pointer. Also did a small refactoring of bufferIsLargeEnoughToContain to use &&
+ rather than an if statement.
+
2021-04-23 Chris Dumez <cdu...@apple.com>
Disable GPUProcess on shipping iOS
Modified: trunk/Source/WTF/wtf/persistence/PersistentDecoder.cpp (276509 => 276510)
--- trunk/Source/WTF/wtf/persistence/PersistentDecoder.cpp 2021-04-23 18:57:30 UTC (rev 276509)
+++ trunk/Source/WTF/wtf/persistence/PersistentDecoder.cpp 2021-04-23 19:06:25 UTC (rev 276510)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2010, 2011, 2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2010-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -47,15 +47,24 @@
return size <= static_cast<size_t>(m_bufferEnd - m_bufferPosition);
}
-bool Decoder::decodeFixedLengthData(uint8_t* data, size_t size)
+const uint8_t* Decoder::bufferPointerForDirectRead(size_t size)
{
if (!bufferIsLargeEnoughToContain(size))
- return false;
+ return nullptr;
- memcpy(data, m_bufferPosition, size);
+ auto data = ""
m_bufferPosition += size;
Encoder::updateChecksumForData(m_sha1, data, size);
+ return data;
+}
+
+bool Decoder::decodeFixedLengthData(uint8_t* data, size_t size)
+{
+ auto buffer = bufferPointerForDirectRead(size);
+ if (!buffer)
+ return false;
+ memcpy(data, buffer, size);
return true;
}
Modified: trunk/Source/WTF/wtf/persistence/PersistentDecoder.h (276509 => 276510)
--- trunk/Source/WTF/wtf/persistence/PersistentDecoder.h 2021-04-23 18:57:30 UTC (rev 276509)
+++ trunk/Source/WTF/wtf/persistence/PersistentDecoder.h 2021-04-23 19:06:25 UTC (rev 276510)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -81,13 +81,11 @@
bool bufferIsLargeEnoughToContain(size_t numElements) const
{
static_assert(std::is_arithmetic<T>::value, "Type T must have a fixed, known encoded size!");
+ return numElements <= std::numeric_limits<size_t>::max() / sizeof(T) && bufferIsLargeEnoughToContain(numElements * sizeof(T));
+ }
- if (numElements > std::numeric_limits<size_t>::max() / sizeof(T))
- return false;
+ WTF_EXPORT_PRIVATE WARN_UNUSED_RETURN const uint8_t* bufferPointerForDirectRead(size_t numBytes);
- return bufferIsLargeEnoughToContain(numElements * sizeof(T));
- }
-
static constexpr bool isIPCDecoder = false;
private:
@@ -103,4 +101,3 @@
}
}
-
Modified: trunk/Source/WebCore/ChangeLog (276509 => 276510)
--- trunk/Source/WebCore/ChangeLog 2021-04-23 18:57:30 UTC (rev 276509)
+++ trunk/Source/WebCore/ChangeLog 2021-04-23 19:06:25 UTC (rev 276510)
@@ -1,3 +1,16 @@
+2021-04-23 Darin Adler <da...@apple.com>
+
+ Remove decoder memory allocations based on untrusted data (sizes) in the stream; related changes
+ https://bugs.webkit.org/show_bug.cgi?id=224984
+
+ Reviewed by Sam Weinig.
+
+ * platform/network/cf/CertificateInfoCFNet.cpp:
+ (WTF::Persistence::decodeCFData): Removed unneeded check for zero size. Removed code that
+ locally allocates a vector before bufferIsLargeEnoughToContain is called. Instead use
+ bufferPointerForDirectRead, which makes does the buffer size check, and pass the pointer
+ directly to CFDataCreate.
+
2021-04-23 Chris Dumez <cdu...@apple.com>
Improve our constructDeletedValue() template specializations
Modified: trunk/Source/WebCore/platform/network/cf/CertificateInfoCFNet.cpp (276509 => 276510)
--- trunk/Source/WebCore/platform/network/cf/CertificateInfoCFNet.cpp 2021-04-23 18:57:30 UTC (rev 276509)
+++ trunk/Source/WebCore/platform/network/cf/CertificateInfoCFNet.cpp 2021-04-23 19:06:25 UTC (rev 276510)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2010, 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2010-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -205,17 +205,15 @@
{
Optional<uint64_t> size;
decoder >> size;
- if (!size)
- return WTF::nullopt;
if (UNLIKELY(!isInBounds<size_t>(*size)))
return WTF::nullopt;
-
- Vector<uint8_t> vector(static_cast<size_t>(*size));
- if (!decoder.decodeFixedLengthData(vector.data(), vector.size()))
+
+ auto pointer = decoder.bufferPointerForDirectRead(static_cast<size_t>(*size));
+ if (!pointer)
return WTF::nullopt;
- return adoptCF(CFDataCreate(nullptr, vector.data(), vector.size()));
+ return adoptCF(CFDataCreate(nullptr, pointer, *size));
}
#if HAVE(SEC_TRUST_SERIALIZATION)
Modified: trunk/Source/WebKit/ChangeLog (276509 => 276510)
--- trunk/Source/WebKit/ChangeLog 2021-04-23 18:57:30 UTC (rev 276509)
+++ trunk/Source/WebKit/ChangeLog 2021-04-23 19:06:25 UTC (rev 276510)
@@ -1,3 +1,40 @@
+2021-04-23 Darin Adler <da...@apple.com>
+
+ Remove decoder memory allocations based on untrusted data (sizes) in the stream; related changes
+ https://bugs.webkit.org/show_bug.cgi?id=224984
+
+ Reviewed by Sam Weinig.
+
+ * Platform/IPC/ArgumentCoders.h: Remove the calls to
+ HashMap::reserveInitialCapacity and HashSet::reserveInitialCapacity, based
+ on number read in from the decoder. This means there will be more wasted
+ memory in these HashMap and HashSet objects, so we have to test to make
+ sure this does not create a performance problem. But without this check,
+ we are trying to allocate memory based on an unstrusted size.
+
+ * Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:
+ (IPC::ArgumentCoder<RefPtr<ApplePayError>>::encode): Removed the coder
+ for a Vector of these RefPtr, replaced it with a coder for an individual one,
+ allowing the Vector ArgumentCoder template to handle vector size and construction.
+ One benefit is that this adds in a shrinkToFit and prevents us from making any
+ separate mistake about pre-sizing the Vector here since we use shared code.
+ (IPC::ArgumentCoder<RefPtr<ApplePayError>>::decode): Ditto.
+ * Shared/WebCoreArgumentCoders.cpp:
+ (IPC::ArgumentCoder<RefPtr<SecurityOrigin>>::encode): Ditto.
+ (IPC::ArgumentCoder<RefPtr<SecurityOrigin>>::decode): Ditto.
+ (IPC::ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::encode):
+ (IPC::ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::decode):
+ Removed unnecessary specialization for the KeyStatusVector. There is already
+ an ArgumentCoder for Vector, for std::pair, for Ref<SharedBuffer>, and for
+ enumerations like CDMKeyStatus, so there's no need to have a specialized
+ coder for this. This function that we are removing had a call to
+ reserveInitialCapacity, but the Vector ArgumentCoder template does not.
+
+ * Shared/WebCoreArgumentCoders.h: Replaced the
+ ArgumentCoder<Vector<RefPtr<WebCore::ApplePayError>>> specialization with
+ ArgumentCoder<RefPtr<WebCore::ApplePayError>>. Removed the
+ ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector> specialization.
+
2021-04-23 Kate Cheney <katherine_che...@apple.com>
Set proper network logging level for full web browsers
Modified: trunk/Source/WebKit/Platform/IPC/ArgumentCoders.h (276509 => 276510)
--- trunk/Source/WebKit/Platform/IPC/ArgumentCoders.h 2021-04-23 18:57:30 UTC (rev 276509)
+++ trunk/Source/WebKit/Platform/IPC/ArgumentCoders.h 2021-04-23 19:06:25 UTC (rev 276510)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2010-2020 Apple Inc. All rights reserved.
+ * Copyright (C) 2010-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -514,7 +514,6 @@
return WTF::nullopt;
HashMapType hashMap;
- hashMap.reserveInitialCapacity(hashMapSize);
for (uint32_t i = 0; i < hashMapSize; ++i) {
Optional<KeyArg> key;
decoder >> key;
@@ -580,7 +579,6 @@
return WTF::nullopt;
HashSetType hashSet;
- hashSet.reserveInitialCapacity(hashSetSize);
for (uint64_t i = 0; i < hashSetSize; ++i) {
Optional<KeyArg> key;
decoder >> key;
Modified: trunk/Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm (276509 => 276510)
--- trunk/Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm 2021-04-23 18:57:30 UTC (rev 276509)
+++ trunk/Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm 2021-04-23 19:06:25 UTC (rev 276510)
@@ -344,38 +344,28 @@
return true;
}
-void ArgumentCoder<Vector<RefPtr<ApplePayError>>>::encode(Encoder& encoder, const Vector<RefPtr<ApplePayError>>& errors)
+void ArgumentCoder<RefPtr<ApplePayError>>::encode(Encoder& encoder, const RefPtr<ApplePayError>& error)
{
- encoder << static_cast<uint64_t>(errors.size());
- for (auto& error : errors) {
- encoder << !!error;
- if (error)
- encoder << *error;
- }
+ encoder << !!error;
+ if (error)
+ encoder << *error;
}
-Optional<Vector<RefPtr<ApplePayError>>> ArgumentCoder<Vector<RefPtr<ApplePayError>>>::decode(Decoder& decoder)
+Optional<RefPtr<ApplePayError>> ArgumentCoder<RefPtr<ApplePayError>>::decode(Decoder& decoder)
{
- uint64_t size;
- if (!decoder.decode(size))
+ Optional<bool> isValid;
+ decoder >> isValid;
+ if (!isValid)
return WTF::nullopt;
- Vector<RefPtr<ApplePayError>> errors;
- for (uint64_t i = 0; i < size; ++i) {
- Optional<bool> isValid;
- decoder >> isValid;
- if (!isValid)
- return WTF::nullopt;
+ RefPtr<ApplePayError> error;
+ if (!*isValid)
+ return { nullptr };
- RefPtr<ApplePayError> error;
- if (*isValid) {
- error = ApplePayError::decode(decoder);
- if (!error)
- return WTF::nullopt;
- }
- errors.append(WTFMove(error));
- }
- return errors;
+ error = ApplePayError::decode(decoder);
+ if (!error)
+ return WTF::nullopt;
+ return error;
}
void ArgumentCoder<WebCore::PaymentSessionError>::encode(Encoder& encoder, const WebCore::PaymentSessionError& error)
Modified: trunk/Source/WebKit/Shared/WebCoreArgumentCoders.cpp (276509 => 276510)
--- trunk/Source/WebKit/Shared/WebCoreArgumentCoders.cpp 2021-04-23 18:57:30 UTC (rev 276509)
+++ trunk/Source/WebKit/Shared/WebCoreArgumentCoders.cpp 2021-04-23 19:06:25 UTC (rev 276510)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2011-2020 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -2908,28 +2908,17 @@
return true;
}
-void ArgumentCoder<Vector<RefPtr<SecurityOrigin>>>::encode(Encoder& encoder, const Vector<RefPtr<SecurityOrigin>>& origins)
+void ArgumentCoder<RefPtr<SecurityOrigin>>::encode(Encoder& encoder, const RefPtr<SecurityOrigin>& origin)
{
- encoder << static_cast<uint64_t>(origins.size());
- for (auto& origin : origins)
- encoder << *origin;
+ encoder << *origin;
}
-bool ArgumentCoder<Vector<RefPtr<SecurityOrigin>>>::decode(Decoder& decoder, Vector<RefPtr<SecurityOrigin>>& origins)
+Optional<RefPtr<SecurityOrigin>> ArgumentCoder<RefPtr<SecurityOrigin>>::decode(Decoder& decoder)
{
- uint64_t dataSize;
- if (!decoder.decode(dataSize))
- return false;
-
- for (uint64_t i = 0; i < dataSize; ++i) {
- auto decodedOriginRefPtr = SecurityOrigin::decode(decoder);
- if (!decodedOriginRefPtr)
- return false;
- origins.append(decodedOriginRefPtr.releaseNonNull());
- }
- origins.shrinkToFit();
-
- return true;
+ auto origin = SecurityOrigin::decode(decoder);
+ if (!origin)
+ return WTF::nullopt;
+ return origin;
}
void ArgumentCoder<FontAttributes>::encode(Encoder& encoder, const FontAttributes& attributes)
@@ -3149,38 +3138,6 @@
return makeOptional<WebCore::CDMInstanceSession::Message>({ type, buffer.releaseNonNull() });
}
-
-void ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::encode(Encoder& encoder, const WebCore::CDMInstanceSession::KeyStatusVector& keyStatuses)
-{
- encoder << static_cast<uint64_t>(keyStatuses.size());
- for (auto& keyStatus : keyStatuses) {
- RefPtr<SharedBuffer> key = keyStatus.first.copyRef();
- encoder << key << keyStatus.second;
- }
-}
-
-Optional<WebCore::CDMInstanceSession::KeyStatusVector> ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::decode(Decoder& decoder)
-{
- uint64_t dataSize;
- if (!decoder.decode(dataSize))
- return WTF::nullopt;
-
- WebCore::CDMInstanceSession::KeyStatusVector keyStatuses;
- keyStatuses.reserveInitialCapacity(dataSize);
-
- for (uint64_t i = 0; i < dataSize; ++i) {
- RefPtr<SharedBuffer> key;
- if (!decoder.decode(key) || !key)
- return WTF::nullopt;
-
- WebCore::CDMInstanceSessionClient::KeyStatus status;
- if (!decoder.decode(status))
- return WTF::nullopt;
-
- keyStatuses.uncheckedAppend({ key.releaseNonNull(), status });
- }
- return keyStatuses;
-}
#endif // ENABLE(ENCRYPTED_MEDIA)
template<typename Encoder>
@@ -3246,7 +3203,7 @@
return WTF::nullopt;
if (!isEngaged)
- return RefPtr<WebCore::ImageData>();
+ return { nullptr };
auto result = ArgumentCoder<Ref<WebCore::ImageData>>::decode(decoder);
if (!result)
Modified: trunk/Source/WebKit/Shared/WebCoreArgumentCoders.h (276509 => 276510)
--- trunk/Source/WebKit/Shared/WebCoreArgumentCoders.h 2021-04-23 18:57:30 UTC (rev 276509)
+++ trunk/Source/WebKit/Shared/WebCoreArgumentCoders.h 2021-04-23 19:06:25 UTC (rev 276510)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2010-2020 Apple Inc. All rights reserved.
+ * Copyright (C) 2010-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -704,9 +704,9 @@
static WARN_UNUSED_RETURN bool decode(Decoder&, WebCore::ApplePaySessionPaymentRequest::MerchantCapabilities&);
};
-template<> struct ArgumentCoder<Vector<RefPtr<WebCore::ApplePayError>>> {
- static void encode(Encoder&, const Vector<RefPtr<WebCore::ApplePayError>>&);
- static Optional<Vector<RefPtr<WebCore::ApplePayError>>> decode(Decoder&);
+template<> struct ArgumentCoder<RefPtr<WebCore::ApplePayError>> {
+ static void encode(Encoder&, const RefPtr<WebCore::ApplePayError>&);
+ static Optional<RefPtr<WebCore::ApplePayError>> decode(Decoder&);
};
template<> struct ArgumentCoder<WebCore::PaymentSessionError> {
@@ -761,9 +761,9 @@
static WARN_UNUSED_RETURN bool decode(Decoder&, WebCore::PromisedAttachmentInfo&);
};
-template<> struct ArgumentCoder<Vector<RefPtr<WebCore::SecurityOrigin>>> {
- static void encode(Encoder&, const Vector<RefPtr<WebCore::SecurityOrigin>>&);
- static WARN_UNUSED_RETURN bool decode(Decoder&, Vector<RefPtr<WebCore::SecurityOrigin>>&);
+template<> struct ArgumentCoder<RefPtr<WebCore::SecurityOrigin>> {
+ static void encode(Encoder&, const RefPtr<WebCore::SecurityOrigin>&);
+ static Optional<RefPtr<WebCore::SecurityOrigin>> decode(Decoder&);
};
template<> struct ArgumentCoder<WebCore::FontAttributes> {
@@ -811,11 +811,6 @@
static void encode(Encoder&, const WebCore::CDMInstanceSession::Message&);
static Optional<WebCore::CDMInstanceSession::Message> decode(Decoder&);
};
-
-template<> struct ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector> {
- static void encode(Encoder&, const WebCore::CDMInstanceSession::KeyStatusVector&);
- static Optional<WebCore::CDMInstanceSession::KeyStatusVector> decode(Decoder&);
-};
#endif
template<> struct ArgumentCoder<RefPtr<WebCore::ImageData>> {
