Summary: | decodeCFData should check bufferIsLargeEnoughToContain before allocating a buffer | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||
Component: | Platform | Assignee: | Darin Adler <darin> | ||||
Status: | RESOLVED DUPLICATE | ||||||
Severity: | Normal | CC: | darin, hi | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 209131 | ||||||
Attachments: |
|
Description
Fujii Hironori
2020-03-18 21:09:27 PDT
Created attachment 393943 [details]
Patch
Comment on attachment 393943 [details] 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. 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. (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 Great! Looking forward to seeing that version. 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. 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. 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.
I’m fixing this in bug 224984. *** This bug has been marked as a duplicate of bug 224984 *** |