Implement Request/Response consuming as FormData

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Implement Request/Response consuming as FormData

[Alex Christensen]

Created attachment 406890 [details]
Patch

[Alex Christensen]

Comment on attachment 406890 [details]
Patch

Uploading for EWS and feedback, but now is a bad time to land this change.  I'll hold off for a few weeks.

[Darin Adler]

Comment on attachment 406890 [details]
Patch

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

> Source/WTF/wtf/text/WTFString.cpp:865
> +    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(utf16Length <= length);

Adding the "=" was neat. Checking this even in production builds is less to my taste, but maybe that’s just the way the wind is blowing.

> Source/WTF/wtf/unicode/UTF8Conversion.cpp:100
> +        U8_NEXT_OR_FFFD(reinterpret_cast<const uint8_t*>(source), sourceOffset, sourceEnd - source, character);

Is this correct for all callers? Let’s figure out if we have tests for all clients of this function that cover invalid characters. I bet there are some where failure would be better than converting to FFFD, but I am not sure without a test.

Clearly we don’t have test coverage for most uses of this, because the only test cases I see with new behavior are the form data ones.

A far less elegant, but more conservative, change would be to change only the code used for form data, since we don’t have test coverage for the other uses.

> Source/WebCore/Modules/fetch/FetchBody.cpp:242
> +        m_consumer.resolveWithData(WTFMove(promise), reinterpret_cast<const uint8_t*>(sharedBuffer->data()), sharedBuffer->size());

Avoid the reinterpret_cast here by using the dataAsUInt8Ptr function.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:48
> +    String string = String::fromUTF8(data, length);

I wouldn’t bother with a local variable here. Just put this inside the call to the URLParser.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:50
> +    for (auto& pair : WTF::URLParser::parseURLEncodedForm(string))

Surprised that we need the WTF prefix here.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:145
> +        promise->resolve<IDLInterface<DOMFormData>>(formDataFromData(buffer ? reinterpret_cast<const uint8_t*>(buffer->data()) : nullptr, buffer ? buffer->size() : 0).get());

Avoid the reinterpret_cast here by using the dataAsUInt8Ptr function.

[Alex Christensen]

Created attachment 406990 [details]
Patch

[Alex Christensen]

Created attachment 406991 [details]
Patch

[Alex Christensen]

Created attachment 406999 [details]
Patch

[Alex Christensen]

Created attachment 407003 [details]
Patch

[Darin Adler]

Comment on attachment 407003 [details]
Patch

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

> Source/WTF/wtf/text/WTFString.cpp:862
> +    constexpr auto function = replaceInvalidSequences ? convertUTF8ToUTF16ReplacingInvalidSequences : convertUTF8ToUTF16;

Not sure the constexpr here has any effect.

> Source/WTF/wtf/text/WTFString.cpp:863
> +    if (!function(stringCurrent, reinterpret_cast<const char *>(stringStart + length), &bufferCurrent, bufferCurrent + buffer.size(), nullptr))

Non-WebKit-code-style space was here in "char *".

> Source/WTF/wtf/unicode/UTF8Conversion.h:48
> +WTF_EXPORT_PRIVATE bool convertUTF8ToUTF16ReplacingInvalidSequences(const char* sourceStart, const char* sourceEnd, UChar** targetStart, UChar* targetEnd, bool* isSourceAllASCII);

What was behind the choice to not include the nullptr default for the last argument?

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:48
> +static bool containsOnlyHTTPQuotedStringTokenCodePoints(const String& string)

Seems like we’d want the argument to be StringView, not String.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:59
> +    for (size_t i = 0; i < string.length(); i++) {
> +        if (!isHTTPQuotedStringTokenCodePoint(string[i]))
> +            return false;
> +    }
> +    return true;

The isAllSpecialCharacters template (which could also be named containsOnly) was designed for use in cases like this so we would not write a loop.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:68
> +static HashMap<String, String> parseParameters(const String& input, size_t position)

Seems like we’d want the argument to be StringView, not String.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:109
> +static Optional<MimeType> parseMIMEType(const String& contentType)

Seems like we’d want the argument to be StringView, not String.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:140
> +#if PLATFORM(WIN)
> +const void* memmem(const void* haystack, size_t haystackLength, const void* needle, size_t needleLength)
> +{
> +    const char* pointer = static_cast<const char*>(haystack);
> +    while (haystackLength >= needleLength) {
> +        if (!memcmp(pointer, needle, needleLength))
> +            return pointer;
> +        pointer++;
> +        haystackLength--;
> +    }
> +    return nullptr;
> +}
> +#endif

In the past we put functions from <string.h> that are missing on Windows in <wtf/StringExtras.h>, and I suggest moving it there. If here, it should be marked static. If there, inline.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:162
> +        size_t contentDispsitionParametersBegin = header.find(';', contentDispositionBegin + strlen(contentDispositionCharacters));

"disposition" is misspelled here.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:166
> +        auto parameters = parseParameters(header.substring(contentDispsitionParametersBegin, contentDispositionEnd - contentDispsitionParametersBegin), 0);

Seems unnecessary to construct a WTF::String and allocate another copy in memory just to parse a substring. This is what StringView is for.

[Alex Christensen]

(In reply to Darin Adler from comment #8)
> > Source/WTF/wtf/unicode/UTF8Conversion.h:48
> > +WTF_EXPORT_PRIVATE bool convertUTF8ToUTF16ReplacingInvalidSequences(const char* sourceStart, const char* sourceEnd, UChar** targetStart, UChar* targetEnd, bool* isSourceAllASCII);
> 
> What was behind the choice to not include the nullptr default for the last
> argument?
It wasn't necessary.  It doesn't hurt, though.

> > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:109
> > +static Optional<MimeType> parseMIMEType(const String& contentType)
> 
> Seems like we’d want the argument to be StringView, not String.
In this case it's more convenient to have a const String& for stripLeadingAndTrailingHTTPSpaces, and it doesn't make a difference here because we always have a const String&.  parseParameters, though, is significantly better with a StringView.

[Alex Christensen]

Created attachment 407121 [details]
Patch

[Alex Christensen]

String::operator[] has bounds checks that StringView::operator[] does not.  I need to be more careful with StringView.

[Alex Christensen]

Created attachment 407132 [details]
Patch

[Alex Christensen]

Comment on attachment 407132 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:71
> +        if (position >= input.length())

This check was needed.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:79
> +        if (position >= input.length())

This check is redundant.  Will commit without it.

[Alex Christensen]

(In reply to Alex Christensen from comment #13)
> > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:79
> > +        if (position >= input.length())
> 
> This check is redundant.  Will commit without it.

Actually, I think it is necessary. cq+.

[EWS]

Committed r266087: <https://trac.webkit.org/changeset/266087>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407132 [details].

[Radar WebKit Bug Importer]

<rdar://problem/67703327>

[Alex Christensen]

Rebasing iOS test in a minute...

[Alex Christensen]

https://trac.webkit.org/r266098

[Alex Christensen]

Comment on attachment 407132 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:191
> +        const char* boundary = boundaryWithDashes.utf8().data();
> +        size_t boundaryLength = strlen(boundary);

This is not only using strlen to get the length of a string we just got from a CString and already know the length, but also a use-after-free because the CString returned by utf8() is a temporary that is deallocated after we get a pointer to its contents.  Fixing...

[Alex Christensen]

http://trac.webkit.org/r266140 fixed rdar://problem/67738130

[Darin Adler]

Comment on attachment 407132 [details]
Patch

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

>> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:191
>> +        size_t boundaryLength = strlen(boundary);
> 
> This is not only using strlen to get the length of a string we just got from a CString and already know the length, but also a use-after-free because the CString returned by utf8() is a temporary that is deallocated after we get a pointer to its contents.  Fixing...

Oops! Should have caught that in review.

[Philip Jägenstedt]

Is this the same as bug 212858? I found my way here from https://github.com/mdn/browser-compat-data/pull/6616.

[Alex Christensen]

*** Bug 161190 has been marked as a duplicate of this bug. ***

[nyro]

Is it also related to bug 187461 in some way?