code review 5530051: strconv: return ErrSyntax when unquoting illegal octal ...

strconv: return ErrSyntax when unquoting illegal octal sequences.  This
is consistent with what the Go compiler returns when such sequences
appear in string literals.

Fixes issue 2658.

[Sameer Ajmani, 2012-01-09 16:45:58]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2012-01-09 19:25:41]

LGTM

[r, 2012-01-09 19:26:46]

http://codereview.appspot.com/5530051/diff/2001/src/pkg/html/template/escape_...
File src/pkg/html/template/escape_test.go (right):

http://codereview.appspot.com/5530051/diff/2001/src/pkg/html/template/escape_...
src/pkg/html/template/escape_test.go:303: // This test is broken by the fix to
issue 2658.
is the test broken or the problem gone? that is, can you repair the test, or is
what it's testing no longer relevant?

[r2, 2012-01-09 19:27:29]

On Jan 9, 2012, at 11:25 AM, Russ Cox wrote:

> LGTM

does not LGTM

[Sameer Ajmani, 2012-01-09 23:13:31]

http://codereview.appspot.com/5530051/diff/2001/src/pkg/html/template/escape_...
File src/pkg/html/template/escape_test.go (right):

http://codereview.appspot.com/5530051/diff/2001/src/pkg/html/template/escape_...
src/pkg/html/template/escape_test.go:303: // This test is broken by the fix to
issue 2658.
On 2012/01/09 19:26:47, r wrote:
> is the test broken or the problem gone? that is, can you repair the test, or
is
> what it's testing no longer relevant?

Per rsc, the test is broken, since it contains an invalid octal sequence.  He
suggested Nigel take a look, so I'll add him.

[rsc, 2012-01-09 23:16:07]

Package html is using go's strconv.UnquoteChar to unescape a
non-Go string literal (maybe a JavaScript string literal?
I'm not sure).  In doing so it was depending on buggy
behavior in order to handle escapes that are valid in
C but not in Go, like "\129" (== "\012" "9" in C but
invalid syntax in Go).

[rsc, 2012-01-09 23:16:36]

I should add that I think it's fine to disable the test in html for now
and leave it for the html package authors to fix the bug in html
separately.

Russ

[r2, 2012-01-09 23:48:32]

On Jan 9, 2012, at 3:16 PM, Russ Cox wrote:

> I should add that I think it's fine to disable the test in html for now
> and leave it for the html package authors to fix the bug in html
> separately.

LGTM

[nigeltao, 2012-01-09 23:54:15]

On 10 January 2012 10:16, Russ Cox <rsc@golang.org> wrote:
> I should add that I think it's fine to disable the test in html for now
> and leave it for the html package authors to fix the bug in html
> separately.

CC'ing mikesamuel for the html/template change.

[Sameer Ajmani, 2012-01-10 00:57:05]

*** Submitted as http://code.google.com/p/go/source/detail?r=41199a4a9166 ***

strconv: return ErrSyntax when unquoting illegal octal sequences.  This
is consistent with what the Go compiler returns when such sequences
appear in string literals.

Fixes issue 2658.

R=golang-dev, rsc, r, r, nigeltao
CC=golang-dev
http://codereview.appspot.com/5530051

[MikeSamuel, 2012-01-11 20:20:44]

2012/1/9 Nigel Tao <nigeltao@golang.org>:
> On 10 January 2012 10:16, Russ Cox <rsc@golang.org> wrote:
>> I should add that I think it's fine to disable the test in html for now
>> and leave it for the html package authors to fix the bug in html
>> separately.
>
> CC'ing mikesamuel for the html/template change.

In what way are those test broken?

[MikeSamuel, 2012-01-11 20:25:03]

http://codereview.appspot.com/5530051/diff/5003/src/pkg/html/template/escape_...
File src/pkg/html/template/escape_test.go (right):

http://codereview.appspot.com/5530051/diff/5003/src/pkg/html/template/escape_...
src/pkg/html/template/escape_test.go:306: // 	`<p style="width: {{" 
e\78preS\0Sio/**/n(alert(1337))"}}">`,
I'm stupid.  This and the below are meant to be testing literal CSS text, so 

   "  e\78preS\0Sio/**/n(alert(1337))"

should be

   "  e\\78preS\0Sio/**/n(alert(1337))"

The \78 is not meant to be an octal sequence as seen by the template parser, but
as a hex sequence as seen by the CSS decoder.

http://codereview.appspot.com/5530051/diff/5003/src/pkg/html/template/escape_...
src/pkg/html/template/escape_test.go:317: // 	`<p style="{{" 
-mo\7a-B\0I/**/nding(alert(1337))"}}: ...">`,
Similarly for the \7a here.

    "  -mo\7a-B\0I/**/nding(alert(1337))"

should be

    "  -mo\\7a-B\0I/**/nding(alert(1337))"

[r2, 2012-01-11 20:27:13]

On Jan 11, 2012, at 12:25 PM, mikesamuel@gmail.com wrote:

> 
>
http://codereview.appspot.com/5530051/diff/5003/src/pkg/html/template/escape_...
> File src/pkg/html/template/escape_test.go (right):
> 
>
http://codereview.appspot.com/5530051/diff/5003/src/pkg/html/template/escape_...
> src/pkg/html/template/escape_test.go:306: // 	`<p style="width: {{"
> e\78preS\0Sio/**/n(alert(1337))"}}">`,
> I'm stupid.  This and the below are meant to be testing literal CSS
> text, so
> 
>   "  e\78preS\0Sio/**/n(alert(1337))"

> 
> should be
> 
>   "  e\\78preS\0Sio/**/n(alert(1337))"
> 
> The \78 is not meant to be an octal sequence as seen by the template
> parser, but as a hex sequence as seen by the CSS decoder.

in go, octal constants must be three digits, so you need to do something about
\0 too.

> 
>
http://codereview.appspot.com/5530051/diff/5003/src/pkg/html/template/escape_...
> src/pkg/html/template/escape_test.go:317: // 	`<p style="{{"
> -mo\7a-B\0I/**/nding(alert(1337))"}}: ...">`,
> Similarly for the \7a here.
> 
>    "  -mo\7a-B\0I/**/nding(alert(1337))"
> 
> should be
> 
>    "  -mo\\7a-B\0I/**/nding(alert(1337))"
> 
> http://codereview.appspot.com/5530051/

[MikeSamuel, 2012-01-11 21:30:53]

2012/1/11 Rob 'Commander' Pike <r@google.com>:
>
> On Jan 11, 2012, at 12:25 PM, mikesamuel@gmail.com wrote:
>
>>
>>
http://codereview.appspot.com/5530051/diff/5003/src/pkg/html/template/escape_...
>> File src/pkg/html/template/escape_test.go (right):
>>
>>
http://codereview.appspot.com/5530051/diff/5003/src/pkg/html/template/escape_...
>> src/pkg/html/template/escape_test.go:306: //  `<p style="width: {{"
>> e\78preS\0Sio/**/n(alert(1337))"}}">`,
>> I'm stupid.  This and the below are meant to be testing literal CSS
>> text, so
>>
>>   "  e\78preS\0Sio/**/n(alert(1337))"
>
>>
>> should be
>>
>>   "  e\\78preS\0Sio/**/n(alert(1337))"
>>
>> The \78 is not meant to be an octal sequence as seen by the template
>> parser, but as a hex sequence as seen by the CSS decoder.
>
> in go, octal constants must be three digits, so you need to do something about
\0 too.
>
>>
>>
http://codereview.appspot.com/5530051/diff/5003/src/pkg/html/template/escape_...
>> src/pkg/html/template/escape_test.go:317: //  `<p style="{{"
>> -mo\7a-B\0I/**/nding(alert(1337))"}}: ...">`,
>> Similarly for the \7a here.
>>
>>    "  -mo\7a-B\0I/**/nding(alert(1337))"
>>
>> should be
>>
>>    "  -mo\\7a-B\0I/**/nding(alert(1337))"
>>
>> http://codereview.appspot.com/5530051/
>

I will send out a CL shortly.