| Summary: | KeyedDecoderGeneric crashes when it accesses a data with empty string key. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Takashi Komori <takashi.komori> | ||||||||
| Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | Basuke.Suzuki, chris.reid, commit-queue, dongseong.hwang, don.olmstead, Hironori.Fujii, ross.kirsling, stephan.szabo, takashi.komori, tomoki.imai, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Takashi Komori
2020-01-21 23:28:42 PST
Created attachment 388399 [details]
Patch
Added test just encodes/decodes a boolean data with empty string key but I only tested on wincairo port. How did you find this bug? Is this a real use case? empty string key. (In reply to Fujii Hironori from comment #3) > How did you find this bug? Is this a real use case? empty string key. This crash occurred and found when I tried to decode broken encoded data (zero filled from middle) by chance. So I think it doesn't occur in normal scenarios. To avoid decoding collapsed data, I tried to verify encoded data by using Decoder::verifyChecksum (PersistentDecoder.cpp) But it didn't work because we can't verify before all decoding is done. Persistence::Decoder calculates checksum while decoding data. String::fromUTF8 will return a null string for invalid UTF-8 sequence. You should check null-string after caling String::fromUTF8. result = String::fromUTF8(buffer.data(), size); Created attachment 388524 [details]
Patch
(In reply to Fujii Hironori from comment #5) > String::fromUTF8 will return a null string for invalid UTF-8 sequence. > You should check null-string after caling String::fromUTF8. > > result = String::fromUTF8(buffer.data(), size); Fixed. Comment on attachment 388524 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388524&action=review > Source/WebCore/platform/generic/KeyedDecoderGeneric.cpp:61 > + result = emptyString(); I think if String::fromUTF8 is failed, a null string and false should be returned. Of course, an empty string should be return for size == 0 case. WDTY? Created attachment 388525 [details]
Patch
(In reply to Fujii Hironori from comment #8) > Comment on attachment 388524 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388524&action=review > > > Source/WebCore/platform/generic/KeyedDecoderGeneric.cpp:61 > > + result = emptyString(); > > I think if String::fromUTF8 is failed, a null string and false should be > returned. > > Of course, an empty string should be return for size == 0 case. > > WDTY? I agree. Failing fromUTF8 means decoded string is collapsed. Fixed. The commit-queue encountered the following flaky tests while processing attachment 388525 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) The commit-queue is continuing to process your patch. Comment on attachment 388525 [details] Patch Clearing flags on attachment: 388525 Committed r254971: <https://trac.webkit.org/changeset/254971> All reviewed patches have been landed. Closing bug. |