code review 2967041: Inspect byte sequences read in xml.text() for presence ...

Inspect byte sequences read in xml.text() for presence of XML
disallowed characters.

Uses a second bytes.Buffer to inspect bytes read into the data
slice rune-by-rune.  New var CharacterRange []unicode.Range
(taken from http://www.xml.com/axml/testaxml.htm Section 2.2
Characters) created to assist in the inspection.

Test case "TestDisallowedCharacters" added to xml_test.go.

This change adds time to parses: locally, a sample NLM Medline
file of 59 megabytes averages 9.3 seconds over 5 consecutive
runs to process without the inspection, and 10.0 seconds to
process with the inspection.  Very short files appear to take
some thousands of nano-seconds longer.  Not a scientific analysis
of the difference.

Fixes issue 1259.

[nk, 2010-11-07 17:41:02]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[nigeltao, 2010-11-08 05:14:58]

Cosmetic-only pass for now.

http://codereview.appspot.com/2967041/diff/2001/src/pkg/xml/xml.go
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/2967041/diff/2001/src/pkg/xml/xml.go#newcode874
src/pkg/xml/xml.go:874: // inspect data's contents for disallowed characters
Go commenting style is to use complete sentences, which start with a capital
letter and end with a full stop.

http://codereview.appspot.com/2967041/diff/2001/src/pkg/xml/xml.go#newcode876
src/pkg/xml/xml.go:876: for r, _, runeErr := inspect.ReadRune(); runeErr == nil;
r, _, runeErr = inspect.ReadRune() {
s/runeErr/err/g

http://codereview.appspot.com/2967041/diff/2001/src/pkg/xml/xml.go#newcode877
src/pkg/xml/xml.go:877: if ! unicode.Is( CharacterRange, r ) {
Please run your go files through gofmt and re-upload.

http://codereview.appspot.com/2967041/diff/2001/src/pkg/xml/xml.go#newcode1600
src/pkg/xml/xml.go:1600: // #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] |
[#x10000-#x10FFFF]
I don't think this comment adds anything to the code. Instead, please add a
reference to the source or derivation of these tables, similar to the comment
for var first above.

http://codereview.appspot.com/2967041/diff/2001/src/pkg/xml/xml.go#newcode1607
src/pkg/xml/xml.go:1607: {0x10000, 0x10ffff, 1},
s/ffff/FFFF/ to be consistent with the rest of this file.

http://codereview.appspot.com/2967041/diff/2001/src/pkg/xml/xml_test.go
File src/pkg/xml/xml_test.go (right):

http://codereview.appspot.com/2967041/diff/2001/src/pkg/xml/xml_test.go#newco...
src/pkg/xml/xml_test.go:431: 
Superfluous empty line.

[nk, 2010-11-08 15:28:25]

Hello rsc, nigeltao (cc: golang-dev@googlegroups.com),

Please take another look.

[nigeltao, 2010-11-09 00:24:38]

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml.go
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml.go#newcode875
src/pkg/xml/xml.go:875: inspect := bytes.NewBuffer(data)
Using a bytes.Buffer just to pick runes off seems like an unnecessary
abstraction. I'd use utf8.DecodeRune directly, and it might be worthwhile to
re-run the benchmark.

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml.go#newcode877
src/pkg/xml/xml.go:877: if !unicode.Is(CharacterRange, r) {
Since CharacterRange is pretty small (compared to the variables first and
second), a hand-written test instead of the more generic unicode.Is is feasible,
and might also affect the benchmark.

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml.go#newcode1603
src/pkg/xml/xml.go:1603: var CharacterRange = []unicode.Range{
If code outside of this package won't need to refer to this variable, then
s/C/c/.

If code outside will need to, then it needs a doc comment to say what it's for.

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml.go#newcode1608
src/pkg/xml/xml.go:1608: {0x10000, 0X10FFFF, 1},
s/X/x/

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml_test.go
File src/pkg/xml/xml_test.go (right):

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml_test.go#newco...
src/pkg/xml/xml_test.go:403: inputs := []string{
Can we make at least one of these tests have the illegal character at the end of
the string?

Can we also test something containing "&\x01" and (in a separate test)
"&lt\x02"? What does the spec say to do in those cases?

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml_test.go#newco...
src/pkg/xml/xml_test.go:416: for index, input := range inputs {
s/index/i/

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml_test.go#newco...
src/pkg/xml/xml_test.go:425: t.Fatalf("input %d synerr.Msg wrong: want '%s', got
'%s'", index, messages[index], synerr.Msg)
Use %q instead of '%s'.

[nk, 2010-11-09 14:28:49]

Thanks for the review.

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml.go
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml.go#newcode877
src/pkg/xml/xml.go:877: if !unicode.Is(CharacterRange, r) {
I will look at putting some benchmarks in per the testing package first, then
vary this implementation.

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml.go#newcode1603
src/pkg/xml/xml.go:1603: var CharacterRange = []unicode.Range{
Outside code probably does not need this variable: the parser itself is
performing the inspection, hence no post-parse code needs to check.

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml_test.go
File src/pkg/xml/xml_test.go (right):

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml_test.go#newco...
src/pkg/xml/xml_test.go:403: inputs := []string{
I will add some more coverage as you suggest, and cull some more from the
Conformance Suite.

I will need to check the spec exactly, but \0x1 and \x02 not being Name
characters, I believe those sequences would be well-formedness errors.

[rsc1, 2010-11-15 21:43:55]

Thanks.  This is a nice place to put the check.

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml.go
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml.go#newcode875
src/pkg/xml/xml.go:875: inspect := bytes.NewBuffer(data)
The use of a bytes.Buffer and ReadRune is a bit
of overkill: the actual function you want is
utf8.DecodeRune.

This can do

buf := data
for len(buf) > 0 {
    r, size := utf8.DecodeRune(buf)
    if r == utf8.RuneError && size == 1 {
        p.err = p.syntaxError("invalid UTF-8")
        return nil
    }
    buf = buf[size:]
    if !isValid(r) {
        ...

Since the number of ranges is so small, I would 
just write a function isValid(r int) bool instead of
making the table.  Then you don't need to worry
about checking the speed.

The other uses of unicode.Is are on much larger tables.

[nk, 2010-11-16 20:07:29]

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml.go
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/2967041/diff/9001/src/pkg/xml/xml.go#newcode875
src/pkg/xml/xml.go:875: inspect := bytes.NewBuffer(data)
Duly noted!

[nk, 2010-11-19 14:50:31]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[nk, 2010-11-19 15:18:56]

Hello rsc, et a,

I am sorry for this empty review, I am apparently having a problem locally of
some kind that puts no files in the review.  I apologize for the noise.

cheers,
Nigel

On 2010/11/19 14:50:31, nk wrote:
> Hello rsc (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

[nk, 2010-11-23 14:31:52]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-12-07 18:02:48]

looks pretty good

http://codereview.appspot.com/2967041/diff/27001/src/pkg/xml/xml.go
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/2967041/diff/27001/src/pkg/xml/xml.go#newcode909
src/pkg/xml/xml.go:909: return (rune == 0x09 ||
can drop () everywhere in this function

http://codereview.appspot.com/2967041/diff/27001/src/pkg/xml/xml_test.go
File src/pkg/xml/xml_test.go (right):

http://codereview.appspot.com/2967041/diff/27001/src/pkg/xml/xml_test.go#newc...
src/pkg/xml/xml_test.go:409: inputs := []string{
instead of parallel arrays, use a real data structure.  at top level:

var characterTests = []struct {
    in string
    err string
}{
    {"\x12<doc/>", "illegal character code U+0012"},
    ...
}

then below 

for i, tt := range characterTests {
     use tt.in, tt.err, i

http://codereview.appspot.com/2967041/diff/27001/src/pkg/xml/xml_test.go#newc...
src/pkg/xml/xml_test.go:430: for _, err = p.Token(); err == nil; _, err =
p.Token() {
for err == nil {
    _, err = p.Token()
}

can delete blank line above this one

http://codereview.appspot.com/2967041/diff/27001/src/pkg/xml/xml_test.go#newc...
src/pkg/xml/xml_test.go:433: if synerr, ok := err.(*SyntaxError); ok {
handle immediate errors first.

synerr, ok := err.(*SyntaxError)
if !ok {
    t.Fatalf("input ...")
}
if synerr.Msg != messages[i] {
    t.Fatalf("...")
}

http://golang.org/doc/effective_go.html#if

[nk, 2010-12-09 02:23:49]

This all duly noted.  I appreciate the time you have take on the style
corrections here in the test, it is enlightening.

On 2010/12/07 18:02:48, rsc wrote:
> looks pretty good
> 
> http://codereview.appspot.com/2967041/diff/27001/src/pkg/xml/xml.go
> File src/pkg/xml/xml.go (right):
> 
> http://codereview.appspot.com/2967041/diff/27001/src/pkg/xml/xml.go#newcode909
> src/pkg/xml/xml.go:909: return (rune == 0x09 ||
> can drop () everywhere in this function
> 
> http://codereview.appspot.com/2967041/diff/27001/src/pkg/xml/xml_test.go
> File src/pkg/xml/xml_test.go (right):
> 
>
http://codereview.appspot.com/2967041/diff/27001/src/pkg/xml/xml_test.go#newc...
> src/pkg/xml/xml_test.go:409: inputs := []string{
> instead of parallel arrays, use a real data structure.  at top level:
> 
> var characterTests = []struct {
>     in string
>     err string
> }{
>     {"\x12<doc/>", "illegal character code U+0012"},
>     ...
> }
> 
> then below 
> 
> for i, tt := range characterTests {
>      use tt.in, tt.err, i
> 
>
http://codereview.appspot.com/2967041/diff/27001/src/pkg/xml/xml_test.go#newc...
> src/pkg/xml/xml_test.go:430: for _, err = p.Token(); err == nil; _, err =
> p.Token() {
> for err == nil {
>     _, err = p.Token()
> }
> 
> can delete blank line above this one
> 
>
http://codereview.appspot.com/2967041/diff/27001/src/pkg/xml/xml_test.go#newc...
> src/pkg/xml/xml_test.go:433: if synerr, ok := err.(*SyntaxError); ok {
> handle immediate errors first.
> 
> synerr, ok := err.(*SyntaxError)
> if !ok {
>     t.Fatalf("input ...")
> }
> if synerr.Msg != messages[i] {
>     t.Fatalf("...")
> }
> 
> http://golang.org/doc/effective_go.html#if

[nk, 2010-12-09 03:15:05]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc1, 2010-12-09 18:23:01]

LGTM

[rsc, 2010-12-09 18:24:18]

It looks like you need to complete a CLA as
described at http://golang.org/doc/contribute.html#copyright.

Thanks.
Russ

[rsc, 2010-12-09 19:51:03]

*** Submitted as 09689e5864bb ***

xml: disallow invalid Unicode code points

Fixes issue 1259.

R=rsc
CC=golang-dev
http://codereview.appspot.com/2967041

Committer: Russ Cox <rsc@golang.org>