Bug 205373 – [Cocoa] GPU process should log IPC message name before crashing from invalid WebContent message

• Status: NEW
• Component: WebKit2

[David Kilzer (:ddkilzer)]

GPU process should log IPC message name before crashing from invalid WebContent message.

<rdar://problem/58026088>

[David Kilzer (:ddkilzer)]

Created attachment 385939 [details]
Patch v1

[Tim Horton]

Comment on attachment 385939 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=385939&action=review

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:80
> +    String logMessage = makeString("Received an invalid message '"_s, messageReceiverName.toString().data(), "::"_s, messageName.toString().data(), "' from the WebContent process."_s);

I'm sure Darin could make this line 500x more efficient somehow, but 🤷‍♂️

(I do wonder why you need the .data()s though).

[David Kilzer (:ddkilzer)]

Comment on attachment 385939 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=385939&action=review

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:44
> +#include <wtf/StringConcatenate.h>

This should be:

#include <wtf/text/StringConcatenate.h>

>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:80
>> +    String logMessage = makeString("Received an invalid message '"_s, messageReceiverName.toString().data(), "::"_s, messageName.toString().data(), "' from the WebContent process."_s);
> 
> I'm sure Darin could make this line 500x more efficient somehow, but 🤷‍♂️
> 
> (I do wonder why you need the .data()s though).

Yes, I'll remove the .data() calls.  They were left over from the previous code.

[David Kilzer (:ddkilzer)]

Comment on attachment 385939 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=385939&action=review

>>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:80
>>> +    String logMessage = makeString("Received an invalid message '"_s, messageReceiverName.toString().data(), "::"_s, messageName.toString().data(), "' from the WebContent process."_s);
>> 
>> I'm sure Darin could make this line 500x more efficient somehow, but 🤷‍♂️
>> 
>> (I do wonder why you need the .data()s though).
> 
> Yes, I'll remove the .data() calls.  They were left over from the previous code.

WebKitBuild/Debug/usr/local/include/wtf/text/StringConcatenate.h:349:38: error: implicit instantiation of undefined templ
ate 'WTF::StringTypeAdapter<WTF::CString, void>'
    return tryMakeStringFromAdapters(StringTypeAdapter<StringTypes>(strings)...);
                                     ^
WebKitBuild/Debug/usr/local/include/wtf/text/StringConcatenate.h:355:21: note: in instantiation of function template spec
ialization 'WTF::tryMakeString<WTF::ASCIILiteral, WTF::CString, WTF::ASCIILiteral, WTF::CString, WTF::ASCIILiteral>' req
uested here
    String result = tryMakeString(strings...);
                    ^

So StringTypeAdapter doesn't accept WTF::CString.  Seems like an easy fix.

I also suspect the previous code was using data() before to make sure the const char* values were NUL-terminated.

[David Kilzer (:ddkilzer)]

Created attachment 385973 [details]
Patch v2

[David Kilzer (:ddkilzer)]

Comment on attachment 385939 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=385939&action=review

>>>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:80
>>>> +    String logMessage = makeString("Received an invalid message '"_s, messageReceiverName.toString().data(), "::"_s, messageName.toString().data(), "' from the WebContent process."_s);
>>> 
>>> I'm sure Darin could make this line 500x more efficient somehow, but 🤷‍♂️
>>> 
>>> (I do wonder why you need the .data()s though).
>> 
>> Yes, I'll remove the .data() calls.  They were left over from the previous code.
> 
> WebKitBuild/Debug/usr/local/include/wtf/text/StringConcatenate.h:349:38: error: implicit instantiation of undefined templ
> ate 'WTF::StringTypeAdapter<WTF::CString, void>'
>     return tryMakeStringFromAdapters(StringTypeAdapter<StringTypes>(strings)...);
>                                      ^
> WebKitBuild/Debug/usr/local/include/wtf/text/StringConcatenate.h:355:21: note: in instantiation of function template spec
> ialization 'WTF::tryMakeString<WTF::ASCIILiteral, WTF::CString, WTF::ASCIILiteral, WTF::CString, WTF::ASCIILiteral>' req
> uested here
>     String result = tryMakeString(strings...);
>                     ^
> 
> So StringTypeAdapter doesn't accept WTF::CString.  Seems like an easy fix.
> 
> I also suspect the previous code was using data() before to make sure the const char* values were NUL-terminated.

Oops, CString() assumes the const char* is already NUL-terminated, so that's not why.

IPC::StringReference::data() also returns a const char*, so the .toString() wasn't necessary in the previous code.

Anyway, I'll stop before I over-think this.

[Darin Adler]

Comment on attachment 385939 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=385939&action=review

>>>>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:80
>>>>> +    String logMessage = makeString("Received an invalid message '"_s, messageReceiverName.toString().data(), "::"_s, messageName.toString().data(), "' from the WebContent process."_s);
>>>> 
>>>> I'm sure Darin could make this line 500x more efficient somehow, but 🤷‍♂️
>>>> 
>>>> (I do wonder why you need the .data()s though).
>>> 
>>> Yes, I'll remove the .data() calls.  They were left over from the previous code.
>> 
>> WebKitBuild/Debug/usr/local/include/wtf/text/StringConcatenate.h:349:38: error: implicit instantiation of undefined templ
>> ate 'WTF::StringTypeAdapter<WTF::CString, void>'
>>     return tryMakeStringFromAdapters(StringTypeAdapter<StringTypes>(strings)...);
>>                                      ^
>> WebKitBuild/Debug/usr/local/include/wtf/text/StringConcatenate.h:355:21: note: in instantiation of function template spec
>> ialization 'WTF::tryMakeString<WTF::ASCIILiteral, WTF::CString, WTF::ASCIILiteral, WTF::CString, WTF::ASCIILiteral>' req
>> uested here
>>     String result = tryMakeString(strings...);
>>                     ^
>> 
>> So StringTypeAdapter doesn't accept WTF::CString.  Seems like an easy fix.
>> 
>> I also suspect the previous code was using data() before to make sure the const char* values were NUL-terminated.
> 
> Oops, CString() assumes the const char* is already NUL-terminated, so that's not why.
> 
> IPC::StringReference::data() also returns a const char*, so the .toString() wasn't necessary in the previous code.
> 
> Anyway, I'll stop before I over-think this.

Since Tim mentioned my name, here are some "obvious to me" things that could clean this up a little bit:

- Since we use the UTF-8 form of this string twice in the code below (on Cocoa), then that's what we should store in a local variable: the result of makeString().utf8(), not the result of makeString(). Calling CString::data() twice is fine, designed to be efficient. Calling utf8() twice is not so good, means transcoding twice.

- No need for _s in arguments to makeString. The makeString function takes C string literals and it’s no more efficient to pass ASCIILiteral instead (that's what _s gives us) instead of just the raw const char*. In fact, it may even be true that passing the ASCIILiteral form results in us making and destroying a temporary String. If so, that could be fixed in StringConcatenate.h, but still no reason to use ASCIILiteral in this case. Would be nice if we were clearer on when we do need ASCIILiteral.

- Since StringReference::data() does not return a NUL-terminated string, we cannot omit the toString().data() calls.

- To make this both cleaner at the call site and more efficient, we can implement a StringTypeAdapter for StringReference. It could go into the .cpp file at first, and then could go into a new StringReferenceConcatenate.h or StringConcatenateIPC.h header. Or if we find a clever way to do it without including StringConcatenate.h in StringReference.h, could just add it to StringReference.h.

[David Kilzer (:ddkilzer)]

Created attachment 386005 [details]
Patch v3

[David Kilzer (:ddkilzer)]

Comment on attachment 385939 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=385939&action=review

>>>>>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:80
>>>>>> +    String logMessage = makeString("Received an invalid message '"_s, messageReceiverName.toString().data(), "::"_s, messageName.toString().data(), "' from the WebContent process."_s);
>>>>> 
>>>>> I'm sure Darin could make this line 500x more efficient somehow, but 🤷‍♂️
>>>>> 
>>>>> (I do wonder why you need the .data()s though).
>>>> 
>>>> Yes, I'll remove the .data() calls.  They were left over from the previous code.
>>> 
>>> WebKitBuild/Debug/usr/local/include/wtf/text/StringConcatenate.h:349:38: error: implicit instantiation of undefined templ
>>> ate 'WTF::StringTypeAdapter<WTF::CString, void>'
>>>     return tryMakeStringFromAdapters(StringTypeAdapter<StringTypes>(strings)...);
>>>                                      ^
>>> WebKitBuild/Debug/usr/local/include/wtf/text/StringConcatenate.h:355:21: note: in instantiation of function template spec
>>> ialization 'WTF::tryMakeString<WTF::ASCIILiteral, WTF::CString, WTF::ASCIILiteral, WTF::CString, WTF::ASCIILiteral>' req
>>> uested here
>>>     String result = tryMakeString(strings...);
>>>                     ^
>>> 
>>> So StringTypeAdapter doesn't accept WTF::CString.  Seems like an easy fix.
>>> 
>>> I also suspect the previous code was using data() before to make sure the const char* values were NUL-terminated.
>> 
>> Oops, CString() assumes the const char* is already NUL-terminated, so that's not why.
>> 
>> IPC::StringReference::data() also returns a const char*, so the .toString() wasn't necessary in the previous code.
>> 
>> Anyway, I'll stop before I over-think this.
> 
> Since Tim mentioned my name, here are some "obvious to me" things that could clean this up a little bit:
> 
> - Since we use the UTF-8 form of this string twice in the code below (on Cocoa), then that's what we should store in a local variable: the result of makeString().utf8(), not the result of makeString(). Calling CString::data() twice is fine, designed to be efficient. Calling utf8() twice is not so good, means transcoding twice.
> 
> - No need for _s in arguments to makeString. The makeString function takes C string literals and it’s no more efficient to pass ASCIILiteral instead (that's what _s gives us) instead of just the raw const char*. In fact, it may even be true that passing the ASCIILiteral form results in us making and destroying a temporary String. If so, that could be fixed in StringConcatenate.h, but still no reason to use ASCIILiteral in this case. Would be nice if we were clearer on when we do need ASCIILiteral.
> 
> - Since StringReference::data() does not return a NUL-terminated string, we cannot omit the toString().data() calls.
> 
> - To make this both cleaner at the call site and more efficient, we can implement a StringTypeAdapter for StringReference. It could go into the .cpp file at first, and then could go into a new StringReferenceConcatenate.h or StringConcatenateIPC.h header. Or if we find a clever way to do it without including StringConcatenate.h in StringReference.h, could just add it to StringReference.h.

Fixed all the things in Patch v3.  Thanks!

(I couldn't think of a clever way to create a template specialization for IPC::StringReference without including StringConcatenate.h in StringReference.h.)

[David Kilzer (:ddkilzer)]

Comment on attachment 386005 [details]
Patch v3

Time to rebase the patch.

[David Kilzer (:ddkilzer)]

Created attachment 386007 [details]
Patch v4

[Darin Adler]

Comment on attachment 385939 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=385939&action=review

>>>>>>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:80
>>>>>>> +    String logMessage = makeString("Received an invalid message '"_s, messageReceiverName.toString().data(), "::"_s, messageName.toString().data(), "' from the WebContent process."_s);
>>>>>> 
>>>>>> I'm sure Darin could make this line 500x more efficient somehow, but 🤷‍♂️
>>>>>> 
>>>>>> (I do wonder why you need the .data()s though).
>>>>> 
>>>>> Yes, I'll remove the .data() calls.  They were left over from the previous code.
>>>> 
>>>> WebKitBuild/Debug/usr/local/include/wtf/text/StringConcatenate.h:349:38: error: implicit instantiation of undefined templ
>>>> ate 'WTF::StringTypeAdapter<WTF::CString, void>'
>>>>     return tryMakeStringFromAdapters(StringTypeAdapter<StringTypes>(strings)...);
>>>>                                      ^
>>>> WebKitBuild/Debug/usr/local/include/wtf/text/StringConcatenate.h:355:21: note: in instantiation of function template spec
>>>> ialization 'WTF::tryMakeString<WTF::ASCIILiteral, WTF::CString, WTF::ASCIILiteral, WTF::CString, WTF::ASCIILiteral>' req
>>>> uested here
>>>>     String result = tryMakeString(strings...);
>>>>                     ^
>>>> 
>>>> So StringTypeAdapter doesn't accept WTF::CString.  Seems like an easy fix.
>>>> 
>>>> I also suspect the previous code was using data() before to make sure the const char* values were NUL-terminated.
>>> 
>>> Oops, CString() assumes the const char* is already NUL-terminated, so that's not why.
>>> 
>>> IPC::StringReference::data() also returns a const char*, so the .toString() wasn't necessary in the previous code.
>>> 
>>> Anyway, I'll stop before I over-think this.
>> 
>> Since Tim mentioned my name, here are some "obvious to me" things that could clean this up a little bit:
>> 
>> - Since we use the UTF-8 form of this string twice in the code below (on Cocoa), then that's what we should store in a local variable: the result of makeString().utf8(), not the result of makeString(). Calling CString::data() twice is fine, designed to be efficient. Calling utf8() twice is not so good, means transcoding twice.
>> 
>> - No need for _s in arguments to makeString. The makeString function takes C string literals and it’s no more efficient to pass ASCIILiteral instead (that's what _s gives us) instead of just the raw const char*. In fact, it may even be true that passing the ASCIILiteral form results in us making and destroying a temporary String. If so, that could be fixed in StringConcatenate.h, but still no reason to use ASCIILiteral in this case. Would be nice if we were clearer on when we do need ASCIILiteral.
>> 
>> - Since StringReference::data() does not return a NUL-terminated string, we cannot omit the toString().data() calls.
>> 
>> - To make this both cleaner at the call site and more efficient, we can implement a StringTypeAdapter for StringReference. It could go into the .cpp file at first, and then could go into a new StringReferenceConcatenate.h or StringConcatenateIPC.h header. Or if we find a clever way to do it without including StringConcatenate.h in StringReference.h, could just add it to StringReference.h.
> 
> Fixed all the things in Patch v3.  Thanks!
> 
> (I couldn't think of a clever way to create a template specialization for IPC::StringReference without including StringConcatenate.h in StringReference.h.)

Oops I meant to say could add a StringReferenceConcatenate.h. Sorry.

[Darin Adler]

Comment on attachment 386007 [details]
Patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=386007&action=review

Looks OK, but I suggest not landing it exactly as-is.

> Source/WTF/wtf/text/StringConcatenate.h:206
> +template<> class StringTypeAdapter<CString, void> : public StringTypeAdapter<String, void> {
> +public:
> +    StringTypeAdapter(const CString& string)
> +        : StringTypeAdapter<String, void> { String::fromUTF8(string) }
> +    {
> +    }
> +};

But also, I don’t think this should be included in this patch. The changed code below uses StringReference concatenation, so this is a related enhancement for possible future use, but not needed in this patch.

There are two minor but significant problems with this implementation:

1) This always unconditionally creates a WTF::String. That's not what we want when processing, say, a makeString; the point of StringAdapter is to not create intermediate WTF::String objects until the end of the entire stringifying operation. We want something more like StringTypeAdapter<const char*, void>.

2) This treats the bytes in the CString as UTF-8. But the other string adapters for char and LChar sequences treat characters as Latin-1. We don’t want CString to be different in this respect. On the other hand, it may be useful in many contexts to support UTF-8; might take a little thinking to figure out a way to fit that in with StringAdapter cleanly. Among other things, UTF-8 support means figuring out how to handle illegal UTF-8 sequences. That issue doesn’t come up with Latin-1 since there's no such thing as "illegal" Latin-1.

I don’t see any harm in landing this, but we’d want to revisit it afterward to fix the above issues. And I suggest landing it separately with test coverage.

> Source/WTF/wtf/text/WTFString.cpp:880
>  String String::fromUTF8(const CString& s)
>  {
> -    return fromUTF8(s.data());
> +    return fromUTF8(s.data(), s.length());
>  }

I don’t think this change should be included in this patch.

The change should only affect CString objects that are created for strings with NUL characters embedded in them. I don’t see why this change is needed for this patch. Seems a separate bug fix.

I suggest landing it separately with test coverage.

> Source/WebKit/Platform/IPC/StringReferenceConcatenate.h:39
> +template<> class StringTypeAdapter<IPC::StringReference, void> : public StringTypeAdapter<CString, void> {
> +public:
> +    StringTypeAdapter(const IPC::StringReference& string)
> +        : StringTypeAdapter<CString, void> { string.toString() }
> +    {
> +    }
> +};

This always unconditionally creates a WTF::String. That's not what we want when processing, say, a makeString; the point of StringAdapter is to not create intermediate WTF::String objects until the end of the entire stringifying operation. We want something more like StringTypeAdapter<StringView, void> (which can be found in StringView.h).

I don’t see any harm in landing this as is, but we’d want to revisit it afterward to fix the issue above.

Also seems like we should include a test of this.