209270 2020-03-18 21:09:27 -0700 decodeCFData should check bufferIsLargeEnoughToContain before allocating a buffer 2021-04-23 10:46:12 -0700 1 1 1 Unclassified WebKit Platform WebKit Nightly Build Unspecified Unspecified RESOLVED DUPLICATE 224984 P2 Normal --- 209131 1 fujii darin darin hi oldest_to_newest 1631529 0 fujii 2020-03-18 21:09:27 -0700 decodeCFData should check bufferIsLargeEnoughToContain before allocating a buffer This is a sub-task of Bug 209131. Bug 209131 – Don't allocate a buffer with the decoded size without ensuring bufferIsLargeEnoughToContain(size) 1631531 1 393943 fujii 2020-03-18 21:11:28 -0700 Created attachment 393943 Patch 1631547 2 393943 darin 2020-03-18 22:36:41 -0700 Comment on attachment 393943 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393943&action=review > Source/WebCore/platform/network/cf/CertificateInfo.h:118 > + if (!decoder.bufferIsLargeEnoughToContain<uint8_t>(static_cast<size_t>(size))) This cast to size_t doesn’t look right, nor does the code below. If the size is larger than what will fit in size_t, this will do the wrong thing. We can clean up this mix of types. CFDataCreate takes an argument of a type called CFIndex. So I think we need to write this: if (size > std::numeric_limits<CFIndex>::max()) return false; CFIndex length = size; Then we should use length instead of static_cast<size_t>(size) in both places that it appears. 1631548 3 darin 2020-03-18 22:38:01 -0700 I’m also thinking that we could use a helper to decode a size, check the size, allocate a vector of that size, then decode the data into the vector. I think this pattern is happening over and over again. And every time we are writing the same code. 1631569 4 fujii 2020-03-19 01:22:22 -0700 (In reply to Darin Adler from comment #3) > I’m also thinking that we could use a helper to decode a size, check the > size, allocate a vector of that size, then decode the data into the vector. > I think this pattern is happening over and over again. And every time we are > writing the same code. Oops. There are already coder classes for Vector there. Will rewrite code with them. https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/persistence/PersistentCoders.h#L167 https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/Platform/IPC/ArgumentCoders.h#L375 1631698 5 darin 2020-03-19 09:56:16 -0700 Great! Looking forward to seeing that version. 1631839 6 fujii 2020-03-19 14:45:46 -0700 VectorArgumentCoder is just only for WTF::Vector. VectorArgumentCoder can't be used for this case of encodeCFData because encodeCFData doesn't treat WTF::Vector. I will use IPC::VectorArgumentCoder and WTF::Persistence::VectorCoder for other cases in ohter bug tickets. 1631840 7 fujii 2020-03-19 14:49:16 -0700 encodeCFData doesn't need to convert CFIndex to uint64_t. If I understand correctly, CertificateInfo is passed only via IPC between web process and UI process, not in persistent storage. I think it's safe to change the encoding format. 1632251 8 fujii 2020-03-20 13:38:26 -0700 WTF::Persistence::Encoder can't encode 'long'. WTF::Persistence::Encoder has int32_t (int) and int64_t (long long) encoders, but long. Even though both long and long long are 64bit, they are different types. > CompileC /Users/fujii/webkit/ga/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/ResourceResponseCocoa.o platform/network/cocoa/ResourceResponseCocoa.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler > cd /Users/fujii/webkit/ga/Source/WebCore > export LANG=en_US.US-ASCII > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang -x objective-c++ -target x86_64-apple-macos10.15 -fmessage-length=0 -fdiagnostics-show-note-include-stack -fmacro-backtrace-limit=0 -std=gnu++1z -stdlib=libc++ -f$ > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cocoa/ResourceResponseCocoa.mm:27: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cf/ResourceResponse.h:28: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/ResourceResponseBase.h:30: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cf/CertificateInfo.h:32: > In file included from /Users/fujii/webkit/ga/WebKitBuild/Debug/usr/local/include/wtf/persistence/PersistentCoders.h:36: > In file included from /Users/fujii/webkit/ga/WebKitBuild/Debug/usr/local/include/wtf/persistence/PersistentDecoder.h:30: > /Users/fujii/webkit/ga/WebKitBuild/Debug/usr/local/include/wtf/persistence/PersistentCoder.h:37:10: error: member reference base type 'const long' is not a structure or union > t.encode(encoder); > ~^~~~~~~ > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cocoa/ResourceResponseCocoa.mm:27: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cf/ResourceResponse.h:28: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/ResourceResponseBase.h:30: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cf/CertificateInfo.h:32: > In file included from /Users/fujii/webkit/ga/WebKitBuild/Debug/usr/local/include/wtf/persistence/PersistentCoders.h:37: > /Users/fujii/webkit/ga/WebKitBuild/Debug/usr/local/include/wtf/persistence/PersistentEncoder.h:65:19: note: in instantiation of member function 'WTF::Persistence::Coder<long>::encode' requested here > Coder<T>::encode(*this, t); > ^ > /Users/fujii/webkit/ga/WebKitBuild/Debug/usr/local/include/wtf/persistence/PersistentEncoder.h:70:9: note: in instantiation of function template specialization 'WTF::Persistence::Encoder::encode<long>' requested here > encode(t); > ^ > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cocoa/ResourceResponseCocoa.mm:27: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cf/ResourceResponse.h:28: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/ResourceResponseBase.h:30: > /Users/fujii/webkit/ga/Source/WebCore/platform/network/cf/CertificateInfo.h:108:13: note: in instantiation of function template specialization 'WTF::Persistence::Encoder::operator<<<long>' requested here > encoder << length; > ^ > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cocoa/ResourceResponseCocoa.mm:27: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cf/ResourceResponse.h:28: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/ResourceResponseBase.h:30: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cf/CertificateInfo.h:32: > In file included from /Users/fujii/webkit/ga/WebKitBuild/Debug/usr/local/include/wtf/persistence/PersistentCoders.h:36: > In file included from /Users/fujii/webkit/ga/WebKitBuild/Debug/usr/local/include/wtf/persistence/PersistentDecoder.h:30: > /Users/fujii/webkit/ga/WebKitBuild/Debug/usr/local/include/wtf/persistence/PersistentCoder.h:42:16: error: type 'long' cannot be used prior to '::' because it has no members > return T::decode(decoder, t); > ^ > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cocoa/ResourceResponseCocoa.mm:27: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cf/ResourceResponse.h:28: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/ResourceResponseBase.h:30: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cf/CertificateInfo.h:32: > In file included from /Users/fujii/webkit/ga/WebKitBuild/Debug/usr/local/include/wtf/persistence/PersistentCoders.h:36: > /Users/fujii/webkit/ga/WebKitBuild/Debug/usr/local/include/wtf/persistence/PersistentDecoder.h:85:26: note: in instantiation of member function 'WTF::Persistence::Coder<long>::decode' requested here > return Coder<T>::decode(*this, t); > ^ > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cocoa/ResourceResponseCocoa.mm:27: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/cf/ResourceResponse.h:28: > In file included from /Users/fujii/webkit/ga/Source/WebCore/platform/network/ResourceResponseBase.h:30: > /Users/fujii/webkit/ga/Source/WebCore/platform/network/cf/CertificateInfo.h:115:18: note: in instantiation of function template specialization 'WTF::Persistence::Decoder::decode<long>' requested here > if (!decoder.decode(size)) > ^ > 2 errors generated. 1753637 9 darin 2021-04-23 10:46:12 -0700 I’m fixing this in bug 224984. *** This bug has been marked as a duplicate of bug 224984 *** 393943 2020-03-18 21:11:28 -0700 2020-03-18 22:36:41 -0700 Patch bug-209270-20200319131127.patch text/plain 1526 fujii U3VidmVyc2lvbiBSZXZpc2lvbjogMjU4Njc0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOGM5NmM2ZGE2YjM5Mzg5 YjJjZTBlZjJhMTQ1YTQwNTQwYTdjZjYyNy4uZmIwNmYxYjNhZmUxYjAwNmNkZGI3ZGFiNWE2NzAy ZjRmOWI5YjIzYyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDIwLTAzLTE4ICBGdWpp aSBIaXJvbm9yaSAgPEhpcm9ub3JpLkZ1amlpQHNvbnkuY29tPgorCisgICAgICAgIGRlY29kZUNG RGF0YSBzaG91bGQgY2hlY2sgYnVmZmVySXNMYXJnZUVub3VnaFRvQ29udGFpbiBiZWZvcmUgYWxs b2NhdGluZyBhIGJ1ZmZlcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MjA5MjcwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgKiBwbGF0Zm9ybS9uZXR3b3JrL2NmL0NlcnRpZmljYXRlSW5mby5oOgorICAgICAg ICAoV1RGOjpQZXJzaXN0ZW5jZTo6ZGVjb2RlQ0ZEYXRhKTogQ2hlY2sgc2l6ZSBieSB1c2luZyBi dWZmZXJJc0xhcmdlRW5vdWdoVG9Db250YWluLgorCiAyMDIwLTAzLTE4ICBBbmRyZXMgR29uemFs ZXogIDxhbmRyZXNnXzIyQGFwcGxlLmNvbT4KIAogICAgICAgICBTZXZlcmFsIFRleHRNYXJrZXIg YXR0cmlidXRlcyBuZWVkIHRvIHJ1biBvbiB0aGUgbWFpbiB0aHJlYWQuCmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL2NmL0NlcnRpZmljYXRlSW5mby5oIGIvU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9jZi9DZXJ0aWZpY2F0ZUluZm8uaAppbmRleCBm OTMxMTEzMDc5M2MxMzQ4NWFjYzY1N2M0MGQ2YWY1MTk5YjA0OWY4Li43YTg3MDI0ZWQwYzY3OTIw ZThkODQzOWIwNGMzZGNhNTA4YjY4MGQ0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9uZXR3b3JrL2NmL0NlcnRpZmljYXRlSW5mby5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL25ldHdvcmsvY2YvQ2VydGlmaWNhdGVJbmZvLmgKQEAgLTExNSw2ICsxMTUsOSBAQCBz dGF0aWMgYm9vbCBkZWNvZGVDRkRhdGEoRGVjb2RlciYgZGVjb2RlciwgUmV0YWluUHRyPENGRGF0 YVJlZj4mIGRhdGEpCiAgICAgaWYgKCFkZWNvZGVyLmRlY29kZShzaXplKSkKICAgICAgICAgcmV0 dXJuIGZhbHNlOwogCisgICAgaWYgKCFkZWNvZGVyLmJ1ZmZlcklzTGFyZ2VFbm91Z2hUb0NvbnRh aW48dWludDhfdD4oc3RhdGljX2Nhc3Q8c2l6ZV90PihzaXplKSkpCisgICAgICAgIHJldHVybiBm YWxzZTsKKwogICAgIFZlY3Rvcjx1aW50OF90PiB2ZWN0b3Ioc3RhdGljX2Nhc3Q8c2l6ZV90Pihz aXplKSk7CiAgICAgaWYgKCFkZWNvZGVyLmRlY29kZUZpeGVkTGVuZ3RoRGF0YSh2ZWN0b3IuZGF0 YSgpLCB2ZWN0b3Iuc2l6ZSgpKSkKICAgICAgICAgcmV0dXJuIGZhbHNlOwo=