KeyedDecoderGeneric crashes when it accesses a data with empty string key.

[Takashi Komori]

When KeyedDecoderGeneric decodes a record associated with empty string key, it tries to add a record to a HashMap with null string (not empty string) but adding HashMap fails because HashMap can't treat null string as a key.

While adding a record with null string key, StringHash::hash() crashes by accessing null pointer.

[Takashi Komori]

Created attachment 388399 [details]
Patch

[Takashi Komori]

Added test just encodes/decodes a boolean data with empty string key but I only tested on wincairo port.

[Fujii Hironori]

How did you find this bug? Is this a real use case? empty string key.

[Takashi Komori]

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

[Fujii Hironori]

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

[Takashi Komori]

Created attachment 388524 [details]
Patch

[Takashi Komori]

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

[Fujii Hironori]

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?

[Takashi Komori]

Created attachment 388525 [details]
Patch

[Takashi Komori]

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

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 388525 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 388525 [details]
Patch

Clearing flags on attachment: 388525

Committed r254971: <https://trac.webkit.org/changeset/254971>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58829480>