[webkit-changes] [WebKit/WebKit] 712657: StringImpl::copyCharacters incorrectly uses memcpy...

Darin Adler noreply at github.com
Sun Oct 16 01:00:10 PDT 2022


  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 71265755b78a83c1cc5614e3fede36899e4e3a05
      https://github.com/WebKit/WebKit/commit/71265755b78a83c1cc5614e3fede36899e4e3a05
  Author: Darin Adler <darin at apple.com>
  Date:   2022-10-16 (Sun, 16 Oct 2022)

  Changed paths:
    M Source/JavaScriptCore/API/OpaqueJSString.cpp
    M Source/JavaScriptCore/jit/ExecutableAllocationFuzz.cpp
    M Source/JavaScriptCore/parser/Lexer.cpp
    M Source/JavaScriptCore/runtime/IntlLocale.cpp
    M Source/JavaScriptCore/runtime/IntlObjectInlines.h
    M Source/JavaScriptCore/runtime/JSString.cpp
    M Source/JavaScriptCore/runtime/JSStringJoiner.cpp
    M Source/JavaScriptCore/runtime/StringPrototype.cpp
    M Source/JavaScriptCore/runtime/StringPrototypeInlines.h
    M Source/JavaScriptCore/tools/JSDollarVM.cpp
    M Source/WTF/wtf/DateMath.cpp
    M Source/WTF/wtf/LogInitialization.cpp
    M Source/WTF/wtf/Logger.cpp
    M Source/WTF/wtf/SortedArrayMap.h
    M Source/WTF/wtf/Threading.cpp
    M Source/WTF/wtf/generic/RunLoopGeneric.cpp
    M Source/WTF/wtf/linux/RealTimeThreads.cpp
    M Source/WTF/wtf/text/ASCIIFastPath.h
    M Source/WTF/wtf/text/AtomString.h
    M Source/WTF/wtf/text/Base64.h
    M Source/WTF/wtf/text/IntegerToStringConversion.h
    M Source/WTF/wtf/text/StringConcatenate.h
    M Source/WTF/wtf/text/StringImpl.h
    M Source/WTF/wtf/text/StringView.h
    M Source/WTF/wtf/text/WTFString.cpp
    M Source/WTF/wtf/text/WTFString.h
    M Source/WTF/wtf/text/icu/UTextProviderLatin1.cpp
    M Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.cpp
    M Source/WebCore/html/HTMLImageElement.cpp
    M Source/WebCore/html/HTMLImageElement.h
    M Source/WebCore/html/LinkRelAttribute.cpp
    M Source/WebCore/html/LinkRelAttribute.h
    M Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp
    M Source/WebCore/html/parser/AtomHTMLToken.h
    M Source/WebCore/html/parser/HTMLMetaCharsetParser.cpp
    M Source/WebCore/html/parser/HTMLMetaCharsetParser.h
    M Source/WebCore/html/parser/HTMLPreloadScanner.cpp
    M Source/WebCore/html/parser/HTMLSrcsetParser.cpp
    M Source/WebCore/html/parser/HTMLSrcsetParser.h
    M Source/WebCore/html/parser/HTMLTreeBuilder.cpp
    M Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.cpp
    M Source/WebCore/page/PageSerializer.cpp
    M Source/WebCore/platform/glib/ApplicationGLib.cpp
    M Source/WebCore/platform/graphics/ComplexTextController.cpp
    M Source/WebCore/platform/graphics/StringTruncator.cpp
    M Source/WebCore/platform/graphics/TextRun.h
    M Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h
    M Source/WebCore/platform/graphics/win/FontCacheWin.cpp
    M Source/WebCore/platform/mediastream/CaptureDevice.h
    M Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp
    M Source/WebCore/platform/win/PasteboardWin.cpp
    M Source/WebCore/platform/xr/openxr/OpenXRInstance.cpp
    M Source/WebKit/Platform/LogInitialization.cpp
    M Source/WebKit/Shared/API/c/WKString.cpp
    M Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm
    M Source/WebKit/webpushd/PushService.mm
    M Source/WebKitLegacy/win/DOMCoreClasses.cpp
    M Source/WebKitLegacy/win/WebKitGraphics.cpp
    M Source/WebKitLegacy/win/WebView.cpp
    M Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp

  Log Message:
  -----------
  StringImpl::copyCharacters incorrectly uses memcpy on destination
pointers that may be null
https://bugs.webkit.org/show_bug.cgi?id=246260

We should assert StringImpl::copyCharacters does not discard information
https://bugs.webkit.org/show_bug.cgi?id=205355

rdar://problem/100962334

Reviewed by Yusuke Suzuki.

After studying the call sites of StringImpl::copyCharacters and its
implementation, it is clear that many callers can pass nullptr for either
the source or destination when the length passed in is 0. Since
std::memcpy behavior is undefined in such cases, need to change the
implementation of StringImpl::copyCharacters to not use it in those
cases. In the end, decided to check for zero length even though I advised
against that previously.

Visiting all the call sites of StringImpl::copyCharacters led to
discovering some other anomalies that are fixed with improved coding
idioms, no major changes.

* Source/JavaScriptCore/API/OpaqueJSString.cpp:
(OpaqueJSString::characters): Updated for name change of
StringView::getCharacters.

* Source/JavaScriptCore/jit/ExecutableAllocationFuzz.cpp: Added include
of NeverDestroyed.h.

* Source/JavaScriptCore/parser/Lexer.cpp:
(JSC::Lexer<T>::parseCommentDirectiveValue): Use String::make8Bit instead
of StringImpl::createUninitialized, for code that is more straightforward.
Also wondering why we don't need this optimization in more places.

* Source/JavaScriptCore/runtime/IntlLocale.cpp:
(JSC::LocaleIDBuilder::setKeywordValue): Use StringView::getCharacters.

* Source/JavaScriptCore/runtime/IntlObjectInlines.h:
(JSC::ListFormatInput::ListFormatInput): Use String::convertTo16Bit. This
eliminates the need for m_retainedUpconvertedStrings.

* Source/JavaScriptCore/runtime/JSString.cpp:
(JSC::JSRopeString::resolveRopeInternal const): Tweaked the comment
wording for clarity. Use StringView::getCharacters.
(JSC::JSRopeString::resolveRopeInternalNoSubstring const): Ditto.
(JSC::JSRopeString::resolveRopeSlowCase const): Ditto.

* Source/JavaScriptCore/runtime/JSStringJoiner.cpp:
(JSC::appendStringToData): Updated for name change of
StringView::getCharacters.

* Source/JavaScriptCore/runtime/StringPrototype.cpp:
(JSC::jsSpliceSubstrings): Removed unneeded checks for zero length;
adding a branch to optimize that case isn't valuable and it's not needed
for correctness.
* Source/JavaScriptCore/runtime/StringPrototypeInlines.h:
(JSC::jsSpliceSubstringsWithSeparators): Ditto. Also use
StringView::getCharacters.

* Source/JavaScriptCore/tools/JSDollarVM.cpp:
(JSC::functionMake16BitStringIfPossible): Use String::convertTo16Bit.

* Source/JavaScriptCore/jit/ExecutableAllocationFuzz.cpp: Added include
of NeverDestroyed.h.

* Source/WTF/wtf/DateMath.cpp:
(WTF::validateTimeZone): Use StringView::upconvertedCharacters.

* Source/WTF/wtf/LogInitialization.cpp: Added include of NeverDestroyed.h.
* Source/WTF/wtf/Logger.cpp: Ditto.

* Source/WTF/wtf/SortedArrayMap.h: Added support for a case-sensitive
version of PackedASCIILowerCodes, named PackedASCIILiteral, and for using
a span of characters as the key in the "packed" case without first
converting it to a StringView and then having to check is8Bit at runtime.

* Source/WTF/wtf/Threading.cpp: Added include of NeverDestroyed.h.
* Source/WTF/wtf/generic/RunLoopGeneric.cpp: Ditto.
* Source/WTF/wtf/linux/RealTimeThreads.cpp: Ditto.

* Source/WTF/wtf/text/ASCIIFastPath.h:
(WTF::copyLCharsFromUCharSource): Deleted. Moved into
StringImpl::copyCharacters.

* Source/WTF/wtf/text/AtomString.h: Removed unneeded includes and the
constructor that takes a Vector. Removed unneeded explicit
implementations of copy and move constructors and assignment operators
that are the same as the defaults. Added a lookUp function.

* Source/WTF/wtf/text/Base64.h: Removed the overloads that take a String
so we don't implicitly create a String when a StringView will do.

* Source/WTF/wtf/text/StringConcatenate.h: Updated for name change of
StringView::getCharacters.

* Source/WTF/wtf/text/StringImpl.h:
(WTF::StringImpl::copyCharacters): Used overloading instead of
unnecessarily complex template programming. Made the version that uses
memcpy do a zero-length check so we aren't relying on undefined behavior
of memcpy. Moved the code from copyLCharsFromUCharSource here.
(WTF::StringImpl::removeCharactersImpl): Removed unneeded zero-length
check before calling StringImpl::copyCharacters.

* Source/WTF/wtf/text/StringView.h: Renamed getCharactersWithUpconvert to
just getCharacters since it can be used in many cases that don't involve
upconversion. Also implemented it as a template since there seems no
disadvantage to doing so. Added getCharacters8, getCharacters16, span8,
and span16.

* Source/WTF/wtf/text/IntegerToStringConversion.h: Added a missing
include.

* Source/WTF/wtf/text/WTFString.cpp:
(WTF::String::charactersWithoutNullTermination const): Removed unnecessary
use of size_t.
(WTF::String::make8Bit): Renamed from make8BitFrom16BitSource, changed the
argument type from size_t to unsigned. Changed to return an empty string
rather than a null string by removing a special case for zero length.
(WTF::String::convertTo16Bit): Added. Replaces make16BitFrom8BitSource,
since all callers can do better by converting a String in place, and we
can significantly simplify them since this efficiently does nothing if
characters are already 16-bit.
(WTF::fromUTF8Impl): Rewrote the "crash if size is too big" to use
RELEASE_ASSERT for clarity.
(WTF::String::fromUTF8WithLatin1Fallback): Added a missing RELEASE_ASSERT
for the fallback case, just like the one above.

* Source/WTF/wtf/text/WTFString.h: Updated for the above changes. Removed
some stray semicolons. Removed the constructor that takes a Vector.

* Source/WTF/wtf/text/icu/UTextProviderLatin1.cpp:
(WTF::uTextLatin1Access): Removed unneeded static_cast.
(WTF::uTextLatin1Extract): Ditto.
(WTF::textLatin1ContextAwareMoveInPrimaryContext): Ditto.

* Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.cpp:
(WebCore::ThreadableWebSocketChannelClientWrapper::setSubprotocol):
Updated for name change of StringView::getCharacters.
(WebCore::ThreadableWebSocketChannelClientWrapper::setExtensions): Ditto.
* Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:
(WebCore::CanvasRenderingContext2DBase::normalizeSpaces): Ditto.

* Source/WebCore/html/HTMLImageElement.cpp:
(WebCore::HTMLImageElement::hasLazyLoadableAttributeValue): Take a
StringView.
* Source/WebCore/html/HTMLImageElement.h: Updated for the above.

* Source/WebCore/html/LinkRelAttribute.cpp:
(WebCore::LinkRelAttribute::LinkRelAttribute): Moved most initialization
into the class definition. Take a StringView, and don't allocate any
String as part of parsing.
* Source/WebCore/html/LinkRelAttribute.h: Updated for the above.

* Source/WebCore/html/parser/AtomHTMLToken.h: Updated to use
String::make8Bit. Removed the overload of make8BitFrom16BitSource that
took a Vector directly since this was the only place it was used, and such
functions bloat the String interface with little benefit.

* Source/WebCore/html/parser/HTMLMetaCharsetParser.cpp:
(WebCore::extractCharset): Take StringView.
(WebCore::HTMLMetaCharsetParser::processMeta): Pass StringView to
encodingFromMetaAttributes.
(WebCore::HTMLMetaCharsetParser::encodingFromMetaAttributes): Take
StringView.
(WebCore::HTMLMetaCharsetParser::checkForMetaCharset): Use
AtomString::lookUp since we only need to handle existing known tags, not
any that aren't already in the atom string table.
* Source/WebCore/html/parser/HTMLMetaCharsetParser.h: Update
encodingFromMetaAttributes to take StringView.

* Source/WebCore/html/parser/HTMLPreloadScanner.cpp:
(WebCore::TokenPreloadScanner::tagIdFor): Use a sorted array map instead
of contructing an atom string.
(WebCore::TokenPreloadScanner::StartTagScanner::processAttributes): Use
AtomString::lookUp and StringView.
(WebCore::TokenPreloadScanner::StartTagScanner::processImageAndScriptAttribute):
Use StringView for attribute values, making strings only when the
attribute name indicates we should store the value.
(WebCore::TokenPreloadScanner::StartTagScanner::processAttribute): Ditto.
(WebCore::TokenPreloadScanner::StartTagScanner::setURLToLoad): Take a
StringView, also use a separate named function instead of a mysterious
boolean argument.
(WebCore::TokenPreloadScanner::StartTagScanner::setURLToLoadAllowingReplacement):
More of the same.
(WebCore::HTMLPreloadScanner::scan): Use AtomString::lookUp since we only
need to handle existing known tags, not any that aren't already in the
atom string table.

* Source/WebCore/html/parser/HTMLSrcsetParser.cpp:
(WebCore::bestFitSourceForImageAttributes): Take StringView.
* Source/WebCore/html/parser/HTMLSrcsetParser.h: Update for the above.

* Source/WebCore/html/parser/HTMLTreeBuilder.cpp:
(WebCore::HTMLTreeBuilder::ExternalCharacterTokenBuffer::makeString const):
Updated for name change of String::make8Bit.

* Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.cpp:
(WebCore::Layout::endsWithSoftWrapOpportunity): Use
String::convertTo16Bit.

* Source/WebCore/platform/glib/ApplicationGLib.cpp: Added include of
NeverDestroyed.h.

* Source/WebCore/platform/graphics/ComplexTextController.cpp:
(WebCore::ComplexTextController::collectComplexTextRuns): Use
String::convertTo16Bit.

* Source/WebCore/page/PageSerializer.cpp:
(WebCore::isCharsetSpecifyingNode): Handle the type of the argument in a
simpler way, and use HTMLMetaElement instead of metaTag. Use StringView
instead of atom strings for encodingFromMetaAttributes.

* Source/WebCore/platform/graphics/StringTruncator.cpp:
(WebCore::centerTruncateToBuffer): Updated for name change of
StringView::getCharacters.
(WebCore::rightTruncateToBuffer): Ditto.
(WebCore::rightClipToCharacterBuffer): Ditto.
(WebCore::rightClipToWordBuffer): Ditto.
(WebCore::leftTruncateToBuffer): Ditto.
(WebCore::truncateString): Ditto.

* Source/WebCore/platform/graphics/TextRun.h: Added textAsString.
There's one new call site that takes advantage of taking the string out.
We could also just consider changing the text funtion to return a
const String&, but to do that safely we'd have to make sure no callers
are using substring or similar functions.

* Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h:
(WebCore::GStreamerEMEUtilities::uuidToKeySystem): Removed the peculiar
use of NeverDestroyed here. Since ASCIILiteral is just a pointer, used
it as a return type instead of const ASCIILiteral&.

* Source/WebCore/platform/graphics/win/FontCacheWin.cpp:
(WebCore::FontCache::systemFallbackForCharacters): Updated for name change
of StringView::getCharacters. Would be more elegant to add a
platform-specific StringView::getWideCharactersWithNullTermination
function since this pattern repeats.
(WebCore::createGDIFont): Ditto.
(WebCore::FontCache::getFontSelectionCapabilitiesInFamily): Ditto.

* Source/WebCore/platform/mediastream/CaptureDevice.h: Added include of
NeverDestroyed.h.

* Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:
(WebCore::createGlobalData): Updated for name change of
StringView::getCharacters.
* Source/WebCore/platform/win/PasteboardWin.cpp:
(WebCore::fileSystemPathFromURLOrTitle): Ditto.
(WebCore::Pasteboard::writeURLToDataObject): Ditto.
(WebCore::createGlobalImageFileDescriptor): Ditto.

* Source/WebKitLegacy/win/DOMCoreClasses.cpp:
(DOMElement::font): Updated for name change of StringView::getCharacters.
Would be more elegant to add a platform-specific StringView::getWideCharacters
functionsince this pattern repeats.

* Source/WebCore/platform/xr/openxr/OpenXRInstance.cpp: Added include of
NeverDestroyed.h.
* Source/WebKit/Platform/LogInitialization.cpp: Ditto.

* Source/WebKit/Shared/API/c/WKString.cpp:
(WKStringGetCharacters): Updated for name change of
StringView::getCharacters.

* Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm:
(WebKit::WebInspectorUIProxy::showSavePanel): Make the String to pass to
base64Decode explicitly from the NSString.

* Source/WebKit/webpushd/PushService.mm:
(WebPushD::base64URLDecode): Added. Makes the String to pass explicitly
from the NSString passed in.
(WebPushD::base64Decode): Ditto.

* Source/WebKitLegacy/win/WebKitGraphics.cpp:
(CenterTruncateStringToWidth): Ditto.
(RightTruncateStringToWidth): Ditto.

* Source/WebKitLegacy/win/WebView.cpp:
(WebView::onIMERequestReconvertString): Ditto.

* Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp: Added
include of NeverDestroyed.h.

Canonical link: https://commits.webkit.org/255600@main




More information about the webkit-changes mailing list