The right implementation of CGPT label conversion between UTF8 and UTF16.

The right implementation of CGPT label conversion between UTF8 and UTF16.

For security quick fix, the original UTF8/UTF16 conversion only supports
ASCII area. This CL extends the library to support multiple code units
conversion between UTF8 and UTF16. The UTF8/UTF16 encoded byte(s) would be
decoded to code point first, then be encoded to UTF16/UTF8 correspondingly.

Bill, please kindly review the UTF8/UTF16 conversion.
Peter, please kindly comment if any security concern.

Thanks.

Change-Id: I99c558ff27556e0b8635ba2b8d9925d042e75cb2

BUG=chromium-os:7542
TEST=RUNTESTS=1 emerge-x86-generic vboot_reference
Manually tested the following commands (intentionally mix Chinese and ASCII):
  export C=.../cgpt
  export D=/tmp/hda

  $C add $D -i 1 -l 批P踢T踢T許C夕C餐
  $C find $D -l 批P踢T踢T許C夕C餐
  $C show $D
  $C add $D -i 1 -l 批P踢T踢T許C夕C餐
  $C find $D -l 批P踢T踢T許C夕C餐
  $C add $D -i 1 -l abc012
  $C add $D -i 1 -l 是否看過坊間常見的許茹芸淚海慶功宴吃蓋飯第四集
  $C add $D -i 1 -l 0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ  # ok and truncated
  $C add $D -i 1 -l `printf "\xf4\x91\x81\x81"`  # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xf4\x8f\xbf\xbf"`
  $C add $D -i 1 -l `printf "\xf4\x8f\x44\x44"`  # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xf4\x8f\xbf"`      # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xf0\xbf\xbf\xbf"`
  $C add $D -i 1 -l `printf "\xf0\xbf\xbf\x44"`  # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xf0\x80\x80\x80"`  # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xf0\x80\x84\x80"`  # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xf0\x80\x90\x80"`  # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xf0\x88\x80\x80"`  # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xed\x80\x80"`
  $C add $D -i 1 -l `printf "\xed\xa0\x80"`      # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xe0\xbf\xbf"`
  $C add $D -i 1 -l `printf "\xe0\xbf\x44"`      # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xe0\x80\x80"`      # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xe0\x90\x80"`      # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xe0\xbf"`          # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xd0\x80"`
  $C add $D -i 1 -l `printf "\xd0\x11"`          # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xd0"`              # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\xc0\xaf"`          # (EXPECT: failed)
  $C add $D -i 1 -l `printf "\x80"`              # (EXPECT: failed)

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=6965cbf

[Louis, 2010-11-16 11:34:48]

re-send for typo.

[Bill Richardson, 2010-11-17 17:26:39]

Found some problems.

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c
File cgpt/cgpt_common.c (right):

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c#newcode353
cgpt/cgpt_common.c:353: * twice the number of UTF16 chars plus the terminating
'\0'.
I think this size bound is wrong. It should be "three times", not "twice". For
example, if the input is a single 0xee00, then the output will be 0xee, 0x80,
0x80. The same comments appears in the header file too.

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c#newcode378
cgpt/cgpt_common.c:378: unsigned short codeunit = le16toh(utf16[s16idx]);
Shouldn't codeunit be uint16_t instead of unsigned short, just for consistency?

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c#newcode404
cgpt/cgpt_common.c:404: if (code_point <= 0x7F && maxoutput >= 1) {
All these "maxoutput >=" tests should be "maxoutput >" instead, to leave room
for the terminating '\0'.

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c#newcode457
cgpt/cgpt_common.c:457: unsigned char code_unit;
uint8_t instead of unsigned char ?

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c#newcode477
cgpt/cgpt_common.c:477: break;
I don't think this handles all the valid input. For example,
0xF4 0x91 0x81 0x81 is invalid but I think it will pass here.  You'll probably
need to implement a case statement based on the value of need_more_code_unit in
order to handle it correcly. Look carefully at the Second Byte column of Table
3-7 on page 93 of chapter 3 of the Unicode Standard Version 5.2
(http://www.unicode.org/versions/Unicode5.2.0/ch03.pdf). It's not as regular as
you might expect.

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c#newcode500
cgpt/cgpt_common.c:500: } else if (code_point <= 0x10FFFF && maxoutput >= 2) {
maxoutput > 2, to account for the trailing \0000.

http://codereview.chromium.org/5025003/diff/1/cgpt/cmd_find.c
File cgpt/cmd_find.c (right):

http://codereview.chromium.org/5025003/diff/1/cgpt/cmd_find.c#newcode185
cgpt/cmd_find.c:185: sizeof(entry->name) / sizeof(entry->name[0]),
Should emit an error message here to indicate that you didn't understand the
label. Otherwise you might just think the partition didn't exist.

[Bill Richardson, 2010-11-17 17:41:49]

one more nit.

http://codereview.chromium.org/5025003/diff/1/cgpt/cmd_add.c
File cgpt/cmd_add.c (right):

http://codereview.chromium.org/5025003/diff/1/cgpt/cmd_add.c#newcode249
cgpt/cmd_add.c:249: goto bad;
We should print an error message indicating that the label can't be converted.

[Louis, 2010-11-18 05:35:21]

Bill, thanks for your kind review. I've changed some code according to your
suggestions. Please kindly take a look again.

Note that: to make sure the ill-formed UTF8 sequence is not accepted by cgpt, I
generated commands to test (see the TESTED parameter in the issue comment). I
think they covered well- and ill-formed sequences. But let me know if I lost
anything.

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c
File cgpt/cgpt_common.c (right):

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c#newcode353
cgpt/cgpt_common.c:353: * twice the number of UTF16 chars plus the terminating
'\0'.
Hm...  my initial idea should be "UTF16 bytes", instead of "UTF16 chars". My bad
caused the misleading. I have fixed the comment.

In worst case (code point 0x0800~0xFFFF), a 2-byte UTF16 char unit could be
converted to 4 UTF8 bytes. For code point 0x10000~0x10FFFF, the UTF16 encoding
is 2 code units, which is 4 bytes, and its corresponding UTF8 encoding is also 4
bytes.

See the following table for encoding lengths.
 
     Code point       UTF16       UTF8
   0x0000-0x007F     2 bytes     1 byte
   0x0080-0x07FF     2 bytes     2 bytes
   0x0800-0xFFFF     2 bytes     3 bytes
  0x10000-0x10FFFF   4 bytes     4 bytes


On 2010/11/17 17:26:39, Bill Richardson wrote:
> I think this size bound is wrong. It should be "three times", not "twice". For
> example, if the input is a single 0xee00, then the output will be 0xee, 0x80,
> 0x80. The same comments appears in the header file too.

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c#newcode378
cgpt/cgpt_common.c:378: unsigned short codeunit = le16toh(utf16[s16idx]);
Done. Good catch!
On 2010/11/17 17:26:39, Bill Richardson wrote:
> Shouldn't codeunit be uint16_t instead of unsigned short, just for
consistency?

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c#newcode404
cgpt/cgpt_common.c:404: if (code_point <= 0x7F && maxoutput >= 1) {
They don't because in line 373, the space was reserved already.

maxoutput--;                             /* plan for termination now */

On 2010/11/17 17:26:39, Bill Richardson wrote:
> All these "maxoutput >=" tests should be "maxoutput >" instead, to leave room
> for the terminating '\0'.
>

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c#newcode457
cgpt/cgpt_common.c:457: unsigned char code_unit;
Done. Thanks again.  My stupidness.

On 2010/11/17 17:26:39, Bill Richardson wrote:
> uint8_t instead of unsigned char ?

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c#newcode477
cgpt/cgpt_common.c:477: break;
Done. You are right. I changed the need_more_code_unit into expected_units and
decoded_units. Also checked:

1. shortest possible UTF8 sequence
2. illegal UTF8 sequence
3. surrogate
4. invalid code point


On 2010/11/17 17:26:39, Bill Richardson wrote:
> I don't think this handles all the valid input. For example,
> 0xF4 0x91 0x81 0x81 is invalid but I think it will pass here.  You'll probably
> need to implement a case statement based on the value of need_more_code_unit
in
> order to handle it correcly. Look carefully at the Second Byte column of Table
> 3-7 on page 93 of chapter 3 of the Unicode Standard Version 5.2
> (http://www.unicode.org/versions/Unicode5.2.0/ch03.pdf). It's not as regular
as
> you might expect.

http://codereview.chromium.org/5025003/diff/1/cgpt/cgpt_common.c#newcode500
cgpt/cgpt_common.c:500: } else if (code_point <= 0x10FFFF && maxoutput >= 2) {
In line 452, the space has been reserved.

On 2010/11/17 17:26:39, Bill Richardson wrote:
> maxoutput > 2, to account for the trailing \0000.

http://codereview.chromium.org/5025003/diff/1/cgpt/cmd_add.c
File cgpt/cmd_add.c (right):

http://codereview.chromium.org/5025003/diff/1/cgpt/cmd_add.c#newcode249
cgpt/cmd_add.c:249: goto bad;
On 2010/11/17 17:41:49, Bill Richardson wrote:
> We should print an error message indicating that the label can't be converted.

Done.

http://codereview.chromium.org/5025003/diff/1/cgpt/cmd_find.c
File cgpt/cmd_find.c (right):

http://codereview.chromium.org/5025003/diff/1/cgpt/cmd_find.c#newcode185
cgpt/cmd_find.c:185: sizeof(entry->name) / sizeof(entry->name[0]),
On 2010/11/17 17:26:39, Bill Richardson wrote:
> Should emit an error message here to indicate that you didn't understand the
> label. Otherwise you might just think the partition didn't exist.

Done.

[Bill Richardson, 2010-11-18 19:36:56]

Looks great. Thanks, Louis.

LGTM.

[sosa, 2010-11-19 14:00:36]

I had to revert this.  This change broke the tree.

On Thu, Nov 18, 2010 at 11:36 AM,  <wfrichar@chromium.org> wrote:
> Looks great. Thanks, Louis.
>
> LGTM.
>
>
> http://codereview.chromium.org/5025003/
>

[Bill Richardson, 2010-11-19 15:58:09]

Oops:

vboot_reference-1.0-r144: cc1: warnings being treated as errors
vboot_reference-1.0-r144: cgpt_common.c: In function 'UTF16ToUTF8':
vboot_reference-1.0-r144: cgpt_common.c:373: error: 'code_point' may be used
uninitialized in this function

Looks like a simple fix. Louis, I'll give you a "LGTM" via IM on this one.

Bill

On Fri, Nov 19, 2010 at 6:00 AM, Chris Sosa <sosa@chromium.org> wrote:

> I had to revert this.  This change broke the tree.
>
> On Thu, Nov 18, 2010 at 11:36 AM,  <wfrichar@chromium.org> wrote:
> > Looks great. Thanks, Louis.
> >
> > LGTM.
> >
> >
> > http://codereview.chromium.org/5025003/
> >
>

[Louis, 2010-11-22 01:49:38]

Hm... my carelessness. The state machine should always initialize the
variable. Will fix it soon.


On Fri, Nov 19, 2010 at 11:57 PM, Bill Richardson <wfrichar@chromium.org>wrote:

> Oops:
>
> vboot_reference-1.0-r144: cc1: warnings being treated as errors
> vboot_reference-1.0-r144: cgpt_common.c: In function 'UTF16ToUTF8':
> vboot_reference-1.0-r144: cgpt_common.c:373: error: 'code_point' may be
> used uninitialized in this function
>
> Looks like a simple fix. Louis, I'll give you a "LGTM" via IM on this one.
>
> Bill
>
> On Fri, Nov 19, 2010 at 6:00 AM, Chris Sosa <sosa@chromium.org> wrote:
>
>> I had to revert this.  This change broke the tree.
>>
>> On Thu, Nov 18, 2010 at 11:36 AM,  <wfrichar@chromium.org> wrote:
>> > Looks great. Thanks, Louis.
>> >
>> > LGTM.
>> >
>> >
>> > http://codereview.chromium.org/5025003/
>> >
>>
>
>

[Louis, 2010-11-22 02:21:06]

re-send in http://codereview.chromium.org/5104009.