Bug 209270 – decodeCFData should check bufferIsLargeEnoughToContain before allocating a buffer

• Component: Platform
• Status: RESOLVED DUPLICATE

[Fujii Hironori]

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)

[Fujii Hironori]

Created attachment 393943 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Fujii Hironori]

(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

[Darin Adler]

Great! Looking forward to seeing that version.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Darin Adler]

I’m fixing this in bug 224984.

*** This bug has been marked as a duplicate of bug 224984 ***