Prohibit newlines before record data in the ZoneParser (#979)

* Merge setRR into ZoneParser.Next

* Remove file argument from RR.parse

This was only used to fill in the ParseError file field. Instead we now
fill in that field in ZoneParser.Next.

* Move dynamic update check out of RR.parse

This consolidates all the dynamic update checks into one place.

* Check for unexpected newline before parsing RR data

* Move rr.parse call into if-statement

* Allow dynamic updates for TKEY and RFC3597 records

* Document that ParseError file field is unset from parse

* Inline allowDynamicUpdate into ZoneParser.Next

* Improve and simplify TestUnexpectedNewline

dns.go

@@ -54,7 +54,7 @@ type RR interface {
 	// parse parses an RR from zone file format.
 	//
 	// This will only be called on a new and empty RR type with only the header populated.
-	parse(c *zlexer, origin, file string) *ParseError
+	parse(c *zlexer, origin string) *ParseError
 
 	// isDuplicate returns whether the two RRs are duplicates.
 	isDuplicate(r2 RR) bool
@@ -105,7 +105,7 @@ func (h *RR_Header) unpack(msg []byte, off int) (int, error) {
 	panic("dns: internal error: unpack should never be called on RR_Header")
 }
 
-func (h *RR_Header) parse(c *zlexer, origin, file string) *ParseError {
+func (h *RR_Header) parse(c *zlexer, origin string) *ParseError {
 	panic("dns: internal error: parse should never be called on RR_Header")
 }
 

edns.go

@@ -88,7 +88,7 @@ func (rr *OPT) len(off int, compression map[string]struct{}) int {
 	return l
 }
 
-func (rr *OPT) parse(c *zlexer, origin, file string) *ParseError {
+func (rr *OPT) parse(c *zlexer, origin string) *ParseError {
 	panic("dns: internal error: parse should never be called on OPT")
 }
 

privaterr.go

@@ -68,7 +68,7 @@ func (r *PrivateRR) unpack(msg []byte, off int) (int, error) {
 	return off, err
 }
 
-func (r *PrivateRR) parse(c *zlexer, origin, file string) *ParseError {
+func (r *PrivateRR) parse(c *zlexer, origin string) *ParseError {
 	var l lex
 	text := make([]string, 0, 2) // could be 0..N elements, median is probably 1
 Fetch:
@@ -85,7 +85,7 @@ Fetch:
 
 	err := r.Data.Parse(text)
 	if err != nil {
-		return &ParseError{file, err.Error(), l}
+		return &ParseError{"", err.Error(), l}
 	}
 
 	return nil

scan.go

@@ -503,9 +503,8 @@ func (zp *ZoneParser) Next() (RR, bool) {
 				return zp.setParseError("expecting $TTL value, not this...", l)
 			}
 
-			if e := slurpRemainder(zp.c, zp.file); e != nil {
-				zp.parseErr = e
-				return nil, false
+			if err := slurpRemainder(zp.c); err != nil {
+				return zp.setParseError(err.err, err.lex)
 			}
 
 			ttl, ok := stringToTTL(l.token)
@@ -527,9 +526,8 @@ func (zp *ZoneParser) Next() (RR, bool) {
 				return zp.setParseError("expecting $ORIGIN value, not this...", l)
 			}
 
-			if e := slurpRemainder(zp.c, zp.file); e != nil {
-				zp.parseErr = e
-				return nil, false
+			if err := slurpRemainder(zp.c); err != nil {
+				return zp.setParseError(err.err, err.lex)
 			}
 
 			name, ok := toAbsoluteName(l.token, zp.origin)
@@ -650,19 +648,44 @@ func (zp *ZoneParser) Next() (RR, bool) {
 
 			st = zExpectRdata
 		case zExpectRdata:
-			r, e := setRR(*h, zp.c, zp.origin, zp.file)
-			if e != nil {
-				// If e.lex is nil than we have encounter a unknown RR type
-				// in that case we substitute our current lex token
-				if e.lex.token == "" && e.lex.value == 0 {
-					e.lex = l // Uh, dirty
-				}
-
-				zp.parseErr = e
-				return nil, false
+			var rr RR
+			if newFn, ok := TypeToRR[h.Rrtype]; ok && canParseAsRR(h.Rrtype) {
+				rr = newFn()
+				*rr.Header() = *h
+			} else {
+				rr = &RFC3597{Hdr: *h}
 			}
 
-			return r, true
+			_, isPrivate := rr.(*PrivateRR)
+			if !isPrivate && zp.c.Peek().token == "" {
+				// This is a dynamic update rr.
+
+				// TODO(tmthrgd): Previously slurpRemainder was only called
+				// for certain RR types, which may have been important.
+				if err := slurpRemainder(zp.c); err != nil {
+					return zp.setParseError(err.err, err.lex)
+				}
+
+				return rr, true
+			} else if l.value == zNewline {
+				return zp.setParseError("unexpected newline", l)
+			}
+
+			if err := rr.parse(zp.c, zp.origin); err != nil {
+				// err is a concrete *ParseError without the file field set.
+				// The setParseError call below will construct a new
+				// *ParseError with file set to zp.file.
+
+				// If err.lex is nil than we have encounter an unknown RR type
+				// in that case we substitute our current lex token.
+				if err.lex == (lex{}) {
+					return zp.setParseError(err.err, l)
+				}
+
+				return zp.setParseError(err.err, err.lex)
+			}
+
+			return rr, true
 		}
 	}
 
@@ -671,6 +694,18 @@ func (zp *ZoneParser) Next() (RR, bool) {
 	return nil, false
 }
 
+// canParseAsRR returns true if the record type can be parsed as a
+// concrete RR. It blacklists certain record types that must be parsed
+// according to RFC 3597 because they lack a presentation format.
+func canParseAsRR(rrtype uint16) bool {
+	switch rrtype {
+	case TypeANY, TypeNULL, TypeOPT, TypeTSIG:
+		return false
+	default:
+		return true
+	}
+}
+
 type zlexer struct {
 	br io.ByteReader
 
@@ -682,7 +717,8 @@ type zlexer struct {
 	comBuf  string
 	comment string
 
-	l lex
+	l       lex
+	cachedL *lex
 
 	brace  int
 	quote  bool
@@ -748,13 +784,37 @@ func (zl *zlexer) readByte() (byte, bool) {
 	return c, true
 }
 
+func (zl *zlexer) Peek() lex {
+	if zl.nextL {
+		return zl.l
+	}
+
+	l, ok := zl.Next()
+	if !ok {
+		return l
+	}
+
+	if zl.nextL {
+		// Cache l. Next returns zl.cachedL then zl.l.
+		zl.cachedL = &l
+	} else {
+		// In this case l == zl.l, so we just tell Next to return zl.l.
+		zl.nextL = true
+	}
+
+	return l
+}
+
 func (zl *zlexer) Next() (lex, bool) {
 	l := &zl.l
-	if zl.nextL {
+	switch {
+	case zl.cachedL != nil:
+		l, zl.cachedL = zl.cachedL, nil
+		return *l, true
+	case zl.nextL:
 		zl.nextL = false
 		return *l, true
-	}
-	if l.err {
+	case l.err:
 		// Parsing errors should be sticky.
 		return lex{value: zEOF}, false
 	}
@@ -1302,18 +1362,18 @@ func locCheckEast(token string, longitude uint32) (uint32, bool) {
 }
 
 // "Eat" the rest of the "line"
-func slurpRemainder(c *zlexer, f string) *ParseError {
+func slurpRemainder(c *zlexer) *ParseError {
 	l, _ := c.Next()
 	switch l.value {
 	case zBlank:
 		l, _ = c.Next()
 		if l.value != zNewline && l.value != zEOF {
-			return &ParseError{f, "garbage after rdata", l}
+			return &ParseError{"", "garbage after rdata", l}
 		}
 	case zNewline:
 	case zEOF:
 	default:
-		return &ParseError{f, "garbage after rdata", l}
+		return &ParseError{"", "garbage after rdata", l}
 	}
 	return nil
 }

scan_test.go

@@ -192,6 +192,41 @@ func TestParseZoneReadError(t *testing.T) {
 	}
 }
 
+func TestUnexpectedNewline(t *testing.T) {
+	zone := `
+example.com. 60 PX
+1000 TXT 1K
+`
+	zp := NewZoneParser(strings.NewReader(zone), "example.com.", "")
+	for _, ok := zp.Next(); ok; _, ok = zp.Next() {
+	}
+
+	const expect = `dns: unexpected newline: "\n" at line: 2:18`
+	if err := zp.Err(); err == nil || err.Error() != expect {
+		t.Errorf("expected error to contain %q, got %v", expect, err)
+	}
+
+	// Test that newlines inside braces still work.
+	zone = `
+example.com. 60 PX (
+1000 TXT 1K )
+`
+	zp = NewZoneParser(strings.NewReader(zone), "example.com.", "")
+
+	var count int
+	for _, ok := zp.Next(); ok; _, ok = zp.Next() {
+		count++
+	}
+
+	if count != 1 {
+		t.Errorf("expected 1 record, got %d", count)
+	}
+
+	if err := zp.Err(); err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
+}
+
 func BenchmarkNewRR(b *testing.B) {
 	const name1 = "12345678901234567890123456789012345.12345678.123."
 	const s = name1 + " 3600 IN MX 10 " + name1

tsig.go

@@ -54,7 +54,7 @@ func (rr *TSIG) String() string {
 	return s
 }
 
-func (rr *TSIG) parse(c *zlexer, origin, file string) *ParseError {
+func (rr *TSIG) parse(c *zlexer, origin string) *ParseError {
 	panic("dns: internal error: parse should never be called on TSIG")
 }
 

types.go

@@ -238,7 +238,7 @@ type ANY struct {
 
 func (rr *ANY) String() string { return rr.Hdr.String() }
 
-func (rr *ANY) parse(c *zlexer, origin, file string) *ParseError {
+func (rr *ANY) parse(c *zlexer, origin string) *ParseError {
 	panic("dns: internal error: parse should never be called on ANY")
 }
 
@@ -253,7 +253,7 @@ func (rr *NULL) String() string {
 	return ";" + rr.Hdr.String() + rr.Data
 }
 
-func (rr *NULL) parse(c *zlexer, origin, file string) *ParseError {
+func (rr *NULL) parse(c *zlexer, origin string) *ParseError {
 	panic("dns: internal error: parse should never be called on NULL")
 }