Bug 234944

Summary: WTF::HashAndUTF8CharactersTranslator::translate() falls through ASSERT_NOT_REACHED()
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: Web Template FrameworkAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, darin, ews-watchlist, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 234932    
Bug Blocks:    
Attachments:
Description Flags
Patch v1
darin: review+, ddkilzer: commit-queue-
Patch for landing none

Description David Kilzer (:ddkilzer) 2022-01-06 18:59:51 PST
WTF::HashAndUTF8CharactersTranslator::translate() falls through ASSERT_NOT_REACHED().

Also uses the `isAllASCII` stack variable uninitialized.  The `newString` backing buffer may not be initialized, either.

    static void translate(PackedPtr<StringImpl>& location, const HashAndUTF8Characters& buffer, unsigned hash)
    {
        UChar* target;
        auto newString = StringImpl::createUninitialized(buffer.utf16Length, target);

        bool isAllASCII;
        const char* source = buffer.characters;
        if (!convertUTF8ToUTF16(source, source + buffer.length, &target, target + buffer.utf16Length, &isAllASCII))
            ASSERT_NOT_REACHED();

        if (isAllASCII)
            newString = StringImpl::create(buffer.characters, buffer.length);

        auto* pointer = &newString.leakRef();
        pointer->setHash(hash);
        pointer->setIsAtom(true);
        location = pointer;
    }
Comment 1 David Kilzer (:ddkilzer) 2022-01-06 19:00:16 PST
This code is in Source/WTF/wtf/text/AtomStringImpl.cpp.
Comment 2 Radar WebKit Bug Importer 2022-01-06 19:00:53 PST
<rdar://problem/87230618>
Comment 3 David Kilzer (:ddkilzer) 2022-02-06 12:28:22 PST
Created attachment 451048 [details]
Patch v1
Comment 4 Darin Adler 2022-02-06 12:40:44 PST
Comment on attachment 451048 [details]
Patch v1

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

> Source/WTF/ChangeLog:12
> +        - Force copy of the original buffer after falling through
> +          ASSERT_NOT_REACHED() statement on Release builds.

Or could just do RELEASE_ASSERT_NOT_REACHED? I think we’d rather crash than just doing the wrong thing here.
Comment 5 Darin Adler 2022-02-06 16:52:54 PST
Comment on attachment 451048 [details]
Patch v1

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

> Source/WTF/wtf/text/AtomStringImpl.cpp:201
> +            isAllASCII = true; // Force copy of original buffer.

This doesn't make a lot of sense; it will incorrectly convert UTF-8 sequences into Latin-1 characters. I think CRASH() or abort() or whatever would be better. However, it really doesn’t matter because this code won’t ever be reached.
Comment 6 David Kilzer (:ddkilzer) 2022-02-07 09:23:54 PST
Comment on attachment 451048 [details]
Patch v1

I'll change to RELEASE_ASSERT_NOT_REACHED().
Comment 7 David Kilzer (:ddkilzer) 2022-02-07 09:26:01 PST
Created attachment 451115 [details]
Patch for landing
Comment 8 David Kilzer (:ddkilzer) 2022-02-07 17:20:09 PST
Comment on attachment 451115 [details]
Patch for landing

Marking cq+ since enough tests passed to land this.
Comment 9 EWS 2022-02-07 17:47:02 PST
Committed r289254 (246938@main): <https://commits.webkit.org/246938@main>

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