code review 11975043: encoding/xml: Do not pass through invalid utf8 bytes

encoding/xml: Do not pass through invalid utf8 bytes

EscapeText now escapes 0xFFFD returned from DecodeRune as 0xFFFD, rather than passing through the original byte.
Fixes issue 5880.

[axaxs, 2013-07-28 00:45:41]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[r, 2013-07-28 22:02:21]

https://codereview.appspot.com/11975043/diff/6001/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):

https://codereview.appspot.com/11975043/diff/6001/src/pkg/encoding/xml/xml.go...
src/pkg/encoding/xml/xml.go:1761: if !isInCharacterRange(r) || r == 0xFFFD {
U+FFFD is a valid rune but it encodes to three bytes. the error condition will
have width==1.

you therefore need to check r=='\uFFFD' && width==1

but then we have this question: why are you checking for U+FFFD just to print
U+FFFD?

[r, 2013-07-28 22:03:56]

https://codereview.appspot.com/11975043/diff/6001/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):

https://codereview.appspot.com/11975043/diff/6001/src/pkg/encoding/xml/xml.go...
src/pkg/encoding/xml/xml.go:1763: break
ah i see, the break is important. still, you need to check the width.

[bradfitz, 2013-07-28 22:13:57]

No test?
 On Jul 28, 2013 1:16 AM, <alex@lx.lc> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> xml: Do not pass through invalid utf8 bytes
>
> EscapeText now escapes 0xFFFD returned from DecodeRune as 0xFFFD, rather
> than passing through the original byte.
> Fixes issue 5880.
>
> Please review this at
https://codereview.appspot.**com/11975043/<https://codereview.appspot.com/119...
>
> Affected files:
>   M src/pkg/encoding/xml/xml.go
>
>
> Index: src/pkg/encoding/xml/xml.go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/xml/xml.go
> +++ b/src/pkg/encoding/xml/xml.go
> @@ -1758,7 +1758,7 @@
>                 case '\r':
>                         esc = esc_cr
>                 default:
> -                       if !isInCharacterRange(r) {
> +                       if !isInCharacterRange(r) || r == 0xFFFD {
>                                 esc = esc_fffd
>                                 break
>                         }
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[axaxs, 2013-07-29 02:23:36]

Hello,

@Brad - This is my first attempt at submitting a change, apologies if I'm 
screwing up the format - I did try to follow the contributing guidelines. 
 I'm not sure what test is typical - add to existing test or attach a one 
off?  In this case, I changed the xml_test EscapeText, which previously 
only checked to ensure byte 0 was handled properly, to also check bytes 192 
and 168 per RSC's bug, and also \uFFFD itself.  This is now part of this 
CL.   Please review and let me know if this is not what you meant and I 
should redo a test somehow. 
  
@RSC - I'm not sure what you mean by doing more width checking.  It took me 
a bit also to figure out how the function was written, very clever.  In 
this case of say - 192,168 - DecodeRune returns \uFFFD, 1  which is 
correct.  This means only 192 is written, but as 3 byte \uFFFD which seems 
correct.  Then 168 gets the same treatment.  So, even though it's writing 3 
bytes, it only increments the width by one as it should.  In the case of 
valid \uFFFD, DecodeRune returns a width of three and the same three bytes. 
 I did no editing to any of this functionality, and do not see why we must 
check width further because of this change.  Please guide if you feel 
further is necessary.

Regards,
Alex

On Sunday, July 28, 2013 6:13:55 PM UTC-4, Brad Fitzpatrick wrote:
>
> No test?
>  On Jul 28, 2013 1:16 AM, <al...@lx.lc <javascript:>> wrote:
>
>> Reviewers: golang-dev1,
>>
>> Message:
>> Hello golan...@googlegroups.com <javascript:>,
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go
>>
>>
>> Description:
>> xml: Do not pass through invalid utf8 bytes
>>
>> EscapeText now escapes 0xFFFD returned from DecodeRune as 0xFFFD, rather
>> than passing through the original byte.
>> Fixes issue 5880.
>>
>> Please review this at
https://codereview.appspot.**com/11975043/<https://codereview.appspot.com/119...
>>
>> Affected files:
>>   M src/pkg/encoding/xml/xml.go
>>
>>
>> Index: src/pkg/encoding/xml/xml.go
>> ==============================**==============================**=======
>> --- a/src/pkg/encoding/xml/xml.go
>> +++ b/src/pkg/encoding/xml/xml.go
>> @@ -1758,7 +1758,7 @@
>>                 case '\r':
>>                         esc = esc_cr
>>                 default:
>> -                       if !isInCharacterRange(r) {
>> +                       if !isInCharacterRange(r) || r == 0xFFFD {
>>                                 esc = esc_fffd
>>                                 break
>>                         }
>>
>>
>> -- 
>>
>> ---You received this message because you are subscribed to the Google 
>> Groups "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an 
>> email to golang-dev+...@**googlegroups.com <javascript:>.
>> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
>> .
>>
>>
>>  

[adg, 2013-07-29 02:31:40]

The test case is essentially there in the issue description. Here it is as a
test function:

func TestIssue5880(t *testing.T) {
  type T []byte
  data, err := xml.Marshal(T{192,168,0,1})
  if err != nil {
     t.Errorf("Marshal error: %v'', err)
  }
  if !utf8.Valid(data) {
    t.Errorf("Marshal generated invalid UTF-8: %x", data)
  } 
}

Please add this to your CL, and confirm that it fails before your fix is applied
and passes afterward.

[axaxs, 2013-07-29 02:36:04]

ADG,

Thank you for advice.  I'm sorry to come across as dense but would like to
get this right - should this be its own new test file, or added as part of
existing xml_test.go?

Thanks,
Alex


On Sun, Jul 28, 2013 at 10:31 PM, <adg@golang.org> wrote:

> The test case is essentially there in the issue description. Here it is
> as a test function:
>
> func TestIssue5880(t *testing.T) {
>   type T []byte
>   data, err := xml.Marshal(T{192,168,0,1})
>   if err != nil {
>      t.Errorf("Marshal error: %v'', err)
>   }
>   if !utf8.Valid(data) {
>     t.Errorf("Marshal generated invalid UTF-8: %x", data)
>   }
> }
>
> Please add this to your CL, and confirm that it fails before your fix is
> applied and passes afterward.
>
>
https://codereview.appspot.**com/11975043/<https://codereview.appspot.com/119...
>

[adg, 2013-07-29 02:39:31]

On 29 July 2013 12:36, Alex Skinner <alex@lx.lc> wrote:

> Thank you for advice.  I'm sorry to come across as dense but would like to
> get this right - should this be its own new test file, or added as part of
> existing xml_test.go?


Part of xml_test.go is fine.

Thanks for your help!

[axaxs, 2013-07-29 03:07:56]

Thank you very much, Andrew.

I've resubmitted xml_test.go with properly formatted testcase provided, and
verified that it does pass.

Rob - Ignore previous width comments in my last message(incorrectly
addressed to RSC - sorry!).  You're right - although it works without width
checking by nature of design, it's unnecessary to break on an actual valid
0xFFFD input with a 3 byte width.   So xml.go has been updated to ensure (r
== 0xFFFD && width == 1) before breaking, which indicates an error decoding
the rune.

Please review and thanks for guidance.

Thanks,
Alex


On Sun, Jul 28, 2013 at 10:39 PM, Andrew Gerrand <adg@golang.org> wrote:

>
> On 29 July 2013 12:36, Alex Skinner <alex@lx.lc> wrote:
>
>> Thank you for advice.  I'm sorry to come across as dense but would like
>> to get this right - should this be its own new test file, or added as part
>> of existing xml_test.go?
>
>
> Part of xml_test.go is fine.
>
> Thanks for your help!
>

[r, 2013-07-30 01:48:02]

looks good to me. leaving for adg.

[r, 2013-07-30 01:48:30]

please edit the CL description to start

encoding/xml:

rather than just

xml:

[axaxs, 2013-07-30 01:59:33]

On 2013/07/30 01:48:30, r wrote:
> please edit the CL description to start
> 
> encoding/xml:
> 
> rather than just
> 
> xml:

Done.  Thank you!

[adg, 2013-07-30 03:33:18]

LGTM

Have you signed the CLA? http://golang.org/s/cla

[axaxs, 2013-07-30 04:05:42]

Thank you for the link.  I've just signed the individual CLA.

Alex


On Mon, Jul 29, 2013 at 11:33 PM, <adg@golang.org> wrote:

> LGTM
>
> Have you signed the CLA? http://golang.org/s/cla
>
>
https://codereview.appspot.**com/11975043/<https://codereview.appspot.com/119...
>

[adg, 2013-07-30 04:11:54]

*** Submitted as https://code.google.com/p/go/source/detail?r=986281309b3e ***

encoding/xml: Do not pass through invalid utf8 bytes

EscapeText now escapes 0xFFFD returned from DecodeRune as 0xFFFD, rather than
passing through the original byte.
Fixes issue 5880.

R=golang-dev, r, bradfitz, adg
CC=golang-dev
https://codereview.appspot.com/11975043

Committer: Andrew Gerrand <adg@golang.org>

[adg, 2013-07-30 04:13:35]

Thanks for the fix! :-)