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

David Kilzer (:ddkilzer)
Reported 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; }
Attachments
Patch v1 (1.70 KB, patch)
2022-02-06 12:28 PST, David Kilzer (:ddkilzer)
darin: review+
ddkilzer: commit-queue-
Patch for landing (1.49 KB, patch)
2022-02-07 09:26 PST, David Kilzer (:ddkilzer)
no flags
David Kilzer (:ddkilzer)
Comment 1 2022-01-06 19:00:16 PST
This code is in Source/WTF/wtf/text/AtomStringImpl.cpp.
Radar WebKit Bug Importer
Comment 2 2022-01-06 19:00:53 PST
David Kilzer (:ddkilzer)
Comment 3 2022-02-06 12:28:22 PST
Created attachment 451048 [details] Patch v1
Darin Adler
Comment 4 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.
Darin Adler
Comment 5 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.
David Kilzer (:ddkilzer)
Comment 6 2022-02-07 09:23:54 PST
Comment on attachment 451048 [details] Patch v1 I'll change to RELEASE_ASSERT_NOT_REACHED().
David Kilzer (:ddkilzer)
Comment 7 2022-02-07 09:26:01 PST
Created attachment 451115 [details] Patch for landing
David Kilzer (:ddkilzer)
Comment 8 2022-02-07 17:20:09 PST
Comment on attachment 451115 [details] Patch for landing Marking cq+ since enough tests passed to land this.
EWS
Comment 9 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].
Note You need to log in before you can comment on or make changes to this bug.