Bug 159180 – Implement "replacement" codec

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Jiewen Tan]

Implement "replacement" codec according to spec: https://encoding.spec.whatwg.org/#replacement

[Jiewen Tan]

<rdar://problem/26015178>

[Jiewen Tan]

Created attachment 282190 [details]
Patch

[Jiewen Tan]

Created attachment 282191 [details]
Patch

[Build Bot]

Comment on attachment 282191 [details]
Patch

Attachment 282191 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1583047

New failing tests:
imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization.html

[Build Bot]

Created attachment 282197 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 282191 [details]
Patch

Attachment 282191 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1583104

New failing tests:
imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization.html

[Build Bot]

Created attachment 282198 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282191 [details]
Patch

Attachment 282191 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1583103

New failing tests:
imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization.html

[Build Bot]

Created attachment 282201 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Jiewen Tan]

Created attachment 282205 [details]
Patch

[Jiewen Tan]

Created attachment 282255 [details]
Patch

[Brent Fulgham]

Comment on attachment 282205 [details]
Patch

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

I think this looks great, but I'm concerned about your string handling routine. Please correct that, and I think this will be ready to go.

r- to fix the build and to fix the string handling.

> Source/WebCore/ChangeLog:24
> +        This change refers some of the Blink changes:

This change is based on the following Blink changes:

> Source/WebCore/ChangeLog:28
> +        * WebCore.xcodeproj/project.pbxproj:

You also need to add your new TextCodecReplacement.cpp file to CMakeLists.txt

> Source/WebCore/platform/text/TextEncoding.cpp:52
> +        m_name = 0;

m_name = nullptr;

> Source/WebCore/platform/text/TextEncoding.cpp:61
> +        m_name = 0;

m_name = nullptr;

> Source/WebCore/platform/text/TextEncodingRegistry.cpp:273
> +    return alias && !strcasecmp(alias, "replacement");

strcasecmp is pretty unsafe, since it will always attempt to read 11 bytes of alias, unless there is a proper null termination in the memory address.

You do check for null, but what if alias was allocated as 2 bytes with no null terminator? We'd buffer overrun.

At minimum, it seems like you need to confirm that 'alias' is 11 characters, and return false if it doesn't.

if (!alias)
    return false;

if (strlen(alias) != 11)
    return false;

return !strcasecmp(alias, "replacement");

> Source/WebCore/platform/text/TextEncodingRegistry.cpp:278
> +    return alias == "replacement";

Here you do a case-sensitive comparison. Is that correct?

If 'strcasecmp' was correct above, then this should be something like 'equalLettersIgnoringASCIICase'.

> LayoutTests/imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization-expected.txt:-655
> -FAIL Name "replacement" has label "iso-2022-kr" (charset) assert_equals: expected "replacement" but got "ISO-2022-KR"

Oh, interesting! So these had not been handled properly by us ever! Nice fix.

[Brent Fulgham]

Comment on attachment 282255 [details]
Patch

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

I think this looks great, but I'm concerned about your string handling routine. Please correct that, and I think this will be ready to go.

r- to fix the string handling.

> Source/WebCore/ChangeLog:24
> +        This change refers some of the Blink changes:

This change is based on the following Blink changes:

> Source/WebCore/platform/text/TextEncoding.cpp:52
> +        m_name = 0;

m_name = nullptr;

> Source/WebCore/platform/text/TextEncoding.cpp:61
> +        m_name = 0;

Ditto.

> Source/WebCore/platform/text/TextEncodingRegistry.cpp:273
> +    return alias && !strcasecmp(alias, "replacement");

strcasecmp is pretty unsafe, since it will always attempt to read 11 bytes of alias, unless there is a proper null termination in the memory address.

You do check for null, but what if alias was allocated as 2 bytes with no null terminator? We'd buffer overrun. At minimum, it seems like you need to confirm that 'alias' is 11 characters, and return false if it doesn't.

    if (!alias)
        return false;

    if (strlen(alias) != 11)
         return false;

    return !strcasecmp(alias, "replacement");

> Source/WebCore/platform/text/TextEncodingRegistry.cpp:278
> +    return alias == "replacement";

Here you do a case-sensitive comparison. Is that correct? If 'strcasecmp' was correct above, then this should be something like 'equalLettersIgnoringASCIICase'.

[Jiewen Tan]

Comment on attachment 282255 [details]
Patch

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

>> Source/WebCore/ChangeLog:24
>> +        This change refers some of the Blink changes:
> 
> This change is based on the following Blink changes:

Fixed.

>> Source/WebCore/platform/text/TextEncoding.cpp:52
>> +        m_name = 0;
> 
> m_name = nullptr;

Fixed.

>> Source/WebCore/platform/text/TextEncoding.cpp:61
>> +        m_name = 0;
> 
> Ditto.

Fixed.

>> Source/WebCore/platform/text/TextEncodingRegistry.cpp:273
>> +    return alias && !strcasecmp(alias, "replacement");
> 
> strcasecmp is pretty unsafe, since it will always attempt to read 11 bytes of alias, unless there is a proper null termination in the memory address.
> 
> You do check for null, but what if alias was allocated as 2 bytes with no null terminator? We'd buffer overrun. At minimum, it seems like you need to confirm that 'alias' is 11 characters, and return false if it doesn't.
> 
>     if (!alias)
>         return false;
> 
>     if (strlen(alias) != 11)
>          return false;
> 
>     return !strcasecmp(alias, "replacement");

Fixed.

>> Source/WebCore/platform/text/TextEncodingRegistry.cpp:278
>> +    return alias == "replacement";
> 
> Here you do a case-sensitive comparison. Is that correct? If 'strcasecmp' was correct above, then this should be something like 'equalLettersIgnoringASCIICase'.

This is definitely not correct. Fixed.

[Jiewen Tan]

Created attachment 282291 [details]
Patch

[Jiewen Tan]

Created attachment 282293 [details]
Patch

[Brent Fulgham]

Comment on attachment 282293 [details]
Patch

R=me

[WebKit Commit Bot]

Comment on attachment 282293 [details]
Patch

Clearing flags on attachment: 282293

Committed r202599: <http://trac.webkit.org/changeset/202599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jiewen Tan]

Committed r202635: <http://trac.webkit.org/changeset/202635>
Committed r202636: <http://trac.webkit.org/changeset/202636>
Committed r202643: <http://trac.webkit.org/changeset/202643>

Attempts to fix the ASAN build.