Created attachment 406890 [details] Patch

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.

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.

Created attachment 406990 [details] Patch

Created attachment 406991 [details] Patch

Created attachment 406999 [details] Patch

Created attachment 407003 [details] Patch

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.

(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.

Created attachment 407121 [details] Patch

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

Created attachment 407132 [details] Patch

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.

(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+.

Committed r266087: <https://trac.webkit.org/changeset/266087> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407132 [details].

<rdar://problem/67703327>

Rebasing iOS test in a minute...

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...

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

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.

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

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

Is it also related to bug 187461 in some way?