Bug 159180 - Implement "replacement" codec
Summary: Implement "replacement" codec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-06-27 16:53 PDT by Jiewen Tan
Modified: 2016-06-29 13:07 PDT (History)
9 users (show)

See Also:


Attachments
Patch (21.00 KB, patch)
2016-06-27 17:19 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (21.01 KB, patch)
2016-06-27 17:21 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (699.53 KB, application/zip)
2016-06-27 18:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (947.51 KB, application/zip)
2016-06-27 18:13 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.37 MB, application/zip)
2016-06-27 18:18 PDT, Build Bot
no flags Details
Patch (25.22 KB, patch)
2016-06-27 19:03 PDT, Jiewen Tan
bfulgham: review-
bfulgham: commit-queue-
Details | Formatted Diff | Diff
Patch (26.36 KB, patch)
2016-06-28 10:26 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (26.49 KB, patch)
2016-06-28 15:53 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (26.49 KB, patch)
2016-06-28 16:08 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2016-06-27 16:53:30 PDT
Implement "replacement" codec according to spec: https://encoding.spec.whatwg.org/#replacement
Comment 1 Jiewen Tan 2016-06-27 16:54:53 PDT
<rdar://problem/26015178>
Comment 2 Jiewen Tan 2016-06-27 17:19:34 PDT
Created attachment 282190 [details]
Patch
Comment 3 Jiewen Tan 2016-06-27 17:21:36 PDT
Created attachment 282191 [details]
Patch
Comment 4 Build Bot 2016-06-27 18:02:59 PDT
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
Comment 5 Build Bot 2016-06-27 18:03:02 PDT
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
Comment 6 Build Bot 2016-06-27 18:13:07 PDT
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
Comment 7 Build Bot 2016-06-27 18:13:10 PDT
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
Comment 8 Build Bot 2016-06-27 18:18:48 PDT
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
Comment 9 Build Bot 2016-06-27 18:18:52 PDT
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
Comment 10 Jiewen Tan 2016-06-27 19:03:08 PDT
Created attachment 282205 [details]
Patch
Comment 11 Jiewen Tan 2016-06-28 10:26:45 PDT
Created attachment 282255 [details]
Patch
Comment 12 Brent Fulgham 2016-06-28 15:09:16 PDT
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.
Comment 13 Brent Fulgham 2016-06-28 15:13:05 PDT
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'.
Comment 14 Jiewen Tan 2016-06-28 15:49:27 PDT
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.
Comment 15 Jiewen Tan 2016-06-28 15:53:30 PDT
Created attachment 282291 [details]
Patch
Comment 16 Jiewen Tan 2016-06-28 16:08:52 PDT
Created attachment 282293 [details]
Patch
Comment 17 Brent Fulgham 2016-06-28 17:42:19 PDT
Comment on attachment 282293 [details]
Patch

R=me
Comment 18 WebKit Commit Bot 2016-06-28 18:04:26 PDT
Comment on attachment 282293 [details]
Patch

Clearing flags on attachment: 282293

Committed r202599: <http://trac.webkit.org/changeset/202599>
Comment 19 WebKit Commit Bot 2016-06-28 18:04:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Jiewen Tan 2016-06-29 13:07:00 PDT
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.