Bug 234944 – WTF::HashAndUTF8CharactersTranslator::translate() falls through ASSERT_NOT_REACHED()

• Component: Web Template Framework
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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;
    }

[David Kilzer (:ddkilzer)]

This code is in Source/WTF/wtf/text/AtomStringImpl.cpp.

[Radar WebKit Bug Importer]

<rdar://problem/87230618>

[David Kilzer (:ddkilzer)]

Created attachment 451048 [details]
Patch v1

[Darin Adler]

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.

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

Comment on attachment 451048 [details]
Patch v1

I'll change to RELEASE_ASSERT_NOT_REACHED().

[David Kilzer (:ddkilzer)]

Created attachment 451115 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

Comment on attachment 451115 [details]
Patch for landing

Marking cq+ since enough tests passed to land this.

[EWS]

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