code review 49430046: go/scanner: report too short escape sequences (0 day bug)

go/scanner: report too short escape sequences

Generally improve error messages for escape sequences.

[gri, 2014-01-15 01:00:29]

Hello adonovan@google.com (cc: golang-codereviews@googlegroups.com),

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

[adonovan, 2014-01-15 01:59:46]

On 2014/01/15 01:00:29, gri wrote:
> Hello mailto:adonovan@google.com (cc:
mailto:golang-codereviews@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go/

What do you mean by 0-day here?  Is there a security threat?

[adonovan, 2014-01-15 01:59:52]

https://codereview.appspot.com/49430046/diff/40001/src/pkg/go/scanner/scanner.go
File src/pkg/go/scanner/scanner.go (right):

https://codereview.appspot.com/49430046/diff/40001/src/pkg/go/scanner/scanner...
src/pkg/go/scanner/scanner.go:412: return false //
Delete comment.

This return violates the documented postcondition, since the last char has been
consumed.
Is it possible to rewind?

[gri, 2014-01-15 05:02:07]

By 0-day bug I meant that the bug must have been present since the very first
versions of the scanner. If I recall correctly, in the very beginning we did not
fix (only limit) the number of say octal digits. Later we decided to fix it
always 3.

There's no security issue here. But as is, literals that were incorrect were
accepted and neither the scanner nor parser complained.

https://codereview.appspot.com/49430046/diff/40001/src/pkg/go/scanner/scanner.go
File src/pkg/go/scanner/scanner.go (right):

https://codereview.appspot.com/49430046/diff/40001/src/pkg/go/scanner/scanner...
src/pkg/go/scanner/scanner.go:412: return false //
On 2014/01/15 01:59:53, adonovan wrote:
> Delete comment.
> 
> This return violates the documented postcondition, since the last char has
been
> consumed.
> Is it possible to rewind?

Adjusted function comment. It's not crucial to not consume the offending chars,
just desirable for error recovery. In this case we actually want to consume it
all because it's syntactically correct.

[gri, 2014-01-15 05:02:35]

PTAL
- gri


On Tue, Jan 14, 2014 at 9:02 PM, <gri@golang.org> wrote:

> By 0-day bug I meant that the bug must have been present since the very
> first versions of the scanner. If I recall correctly, in the very
> beginning we did not fix (only limit) the number of say octal digits.
> Later we decided to fix it always 3.
>
> There's no security issue here. But as is, literals that were incorrect
> were accepted and neither the scanner nor parser complained.
>
>
>
> https://codereview.appspot.com/49430046/diff/40001/src/
> pkg/go/scanner/scanner.go
> File src/pkg/go/scanner/scanner.go (right):
>
> https://codereview.appspot.com/49430046/diff/40001/src/
> pkg/go/scanner/scanner.go#newcode412
> src/pkg/go/scanner/scanner.go:412: return false //
> On 2014/01/15 01:59:53, adonovan wrote:
>
>> Delete comment.
>>
>
>  This return violates the documented postcondition, since the last char
>>
> has been
>
>> consumed.
>> Is it possible to rewind?
>>
>
> Adjusted function comment. It's not crucial to not consume the offending
> chars, just desirable for error recovery. In this case we actually want
> to consume it all because it's syntactically correct.
>
> https://codereview.appspot.com/49430046/
>

[adonovan, 2014-01-15 12:35:38]

On 2014/01/15 05:02:35, gri wrote:
> PTAL
> - gri
> 
> 
> On Tue, Jan 14, 2014 at 9:02 PM, <mailto:gri@golang.org> wrote:
> 
> > By 0-day bug I meant that the bug must have been present since the very
> > first versions of the scanner. If I recall correctly, in the very
> > beginning we did not fix (only limit) the number of say octal digits.
> > Later we decided to fix it always 3.
> >
> > There's no security issue here. But as is, literals that were incorrect
> > were accepted and neither the scanner nor parser complained.
> >
> >
> >
> > https://codereview.appspot.com/49430046/diff/40001/src/
> > pkg/go/scanner/scanner.go
> > File src/pkg/go/scanner/scanner.go (right):
> >
> > https://codereview.appspot.com/49430046/diff/40001/src/
> > pkg/go/scanner/scanner.go#newcode412
> > src/pkg/go/scanner/scanner.go:412: return false //
> > On 2014/01/15 01:59:53, adonovan wrote:
> >
> >> Delete comment.
> >>
> >
> >  This return violates the documented postcondition, since the last char
> >>
> > has been
> >
> >> consumed.
> >> Is it possible to rewind?
> >>
> >
> > Adjusted function comment. It's not crucial to not consume the offending
> > chars, just desirable for error recovery. In this case we actually want
> > to consume it all because it's syntactically correct.
> >
> > https://codereview.appspot.com/49430046/
> >


LGTM

(BTW, the term "zero-day bug" in security usually means that a bug previously
unknown to the software maintainer was used for an attack, i.e. the attack
effectively reported the bug, and thus occurred "zero days after".  An attack
based on a publicly reported bug would be an n-day attack where n > 0  is the
age of the bug report.)

[gri, 2014-01-15 17:50:59]

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

go/scanner: report too short escape sequences

Generally improve error messages for escape sequences.

R=adonovan
CC=golang-codereviews
https://codereview.appspot.com/49430046