plugin/hosts: don't drop over-long fields silently (#8551)

#8496 fixed a silent truncation here and named the invariant in its own commit
message: entries were dropped and the hosts file simply looked shorter than it
is, with nothing in the log.

#8516 replaced that mechanism with a streaming parser bounded by maxFieldSize.
The error log #8496 added is still in parse(), but it can no longer report a
dropped entry: bufio.ErrBufferFull is consumed by the read loop, so only a real
I/O error reaches it. A field over maxFieldSize is discarded in lineParser with
no log at all, and when that field is the address the whole line goes with it.

Report both cases, once per dropped field, with the line number and the source
the entries came from.

Signed-off-by: Baltasar Blanco <baltasarblanco.dev@gmail.com>

plugin/hosts/hostsfile.go

@@ -12,6 +12,7 @@ import (
 	"io"
 	"net"
 	"os"
+	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -161,7 +162,7 @@ func (h *Hostsfile) initInline(inline []string) {
 		return
 	}
 
-	h.inline = h.parse(strings.NewReader(strings.Join(inline, "\n")))
+	h.inline = h.parseSource(strings.NewReader(strings.Join(inline, "\n")), "Inline hosts entries")
 }
 
 // maxFieldSize bounds the memory used while assembling a single field that
@@ -170,13 +171,19 @@ func (h *Hostsfile) initInline(inline []string) {
 const maxFieldSize = 1024
 
 // Parse reads the hostsfile and populates the byName and addr maps.
+func (h *Hostsfile) parse(r io.Reader) *Map {
+	return h.parseSource(r, "Hosts file "+strconv.Quote(h.path))
+}
+
+// parseSource is parse with an explicit source name, so that entries inlined in
+// the Corefile are not reported as coming from the hosts file.
 //
 // Lines are read with a bufio.Reader and parsed field by field as the data
 // arrives, so a line of any length is handled with a fixed amount of memory
 // and never aborts the parse of the entries that follow it.
-func (h *Hostsfile) parse(r io.Reader) *Map {
+func (h *Hostsfile) parseSource(r io.Reader, src string) *Map {
 	hmap := newMap()
-	p := lineParser{h: h, hmap: hmap}
+	p := lineParser{h: h, hmap: hmap, src: src, line: 1}
 
 	reader := bufio.NewReader(r)
 	for {
@@ -200,10 +207,12 @@ func (h *Hostsfile) parse(r io.Reader) *Map {
 type lineParser struct {
 	h    *Hostsfile
 	hmap *Map
+	src  string // where the entries came from, for diagnostics
 
 	field     []byte // the field being assembled, possibly spanning chunks
 	oversized bool   // the current field exceeded maxFieldSize and is dropped
 	index     int    // number of fields already seen on this line
+	line      int    // 1-based number of the line being parsed, for diagnostics
 	comment   bool   // the rest of this line is a comment
 	addr      net.IP // address of the current line, nil if unusable
 	family    int
@@ -222,6 +231,7 @@ func (p *lineParser) feed(chunk []byte, last bool) {
 	}
 	if last {
 		p.index, p.comment, p.addr = 0, false, nil
+		p.line++
 	}
 }
 
@@ -279,6 +289,8 @@ func (p *lineParser) emit() {
 	if p.index == 1 {
 		// The first field is the address; without it the line is unusable.
 		if oversized {
+			log.Errorf("%s, line %d: address longer than %d bytes, dropping the line",
+				p.src, p.line, maxFieldSize)
 			return
 		}
 		p.addr = parseIP(string(field))
@@ -292,7 +304,12 @@ func (p *lineParser) emit() {
 		}
 		return
 	}
-	if p.addr == nil || oversized {
+	if p.addr == nil {
+		return
+	}
+	if oversized {
+		log.Errorf("%s, line %d: name longer than %d bytes, dropping the name",
+			p.src, p.line, maxFieldSize)
 		return
 	}
 	p.addName(string(field))

plugin/hosts/hostsfile_test.go

@@ -5,7 +5,10 @@
 package hosts
 
 import (
+	"bytes"
 	"fmt"
+	"io"
+	golog "log"
 	"net"
 	"os"
 	"reflect"
@@ -386,3 +389,129 @@ func TestParseLongLineWithComment(t *testing.T) {
 		t.Errorf("LookupStaticHostV4(after.example.org.) = %v, want [127.0.0.3]", addrs)
 	}
 }
+
+func TestParseLogsOversizedField(t *testing.T) {
+	// #8496 established that a hosts file entry must never be dropped without a
+	// trace: "the hosts file simply looked shorter than it is, with nothing in
+	// the log". A field over maxFieldSize is dropped, and when it is the address
+	// the whole line goes with it, so both cases have to be reported.
+	var logBuf bytes.Buffer
+	golog.SetOutput(&logBuf)
+	defer golog.SetOutput(io.Discard)
+
+	long := strings.Repeat("a", maxFieldSize+1)
+	h := &Hostsfile{
+		Origins: []string{"."},
+		hmap:    newMap(),
+		inline:  newMap(),
+		options: newOptions(),
+		path:    "/tmp/hosts.test",
+	}
+	h.hmap = h.parse(strings.NewReader(
+		"127.0.0.1 before.example.org\n" +
+			"127.0.0.2 " + long + ".example.org\n" +
+			long + " orphan.example.org\n" +
+			"127.0.0.4 after.example.org\n"))
+
+	// Controls: the entries around the dropped fields are still parsed.
+	for _, tc := range []struct{ name, addr string }{
+		{"before.example.org.", "127.0.0.1"},
+		{"after.example.org.", "127.0.0.4"},
+	} {
+		if addrs := h.LookupStaticHostV4(tc.name); len(addrs) != 1 || addrs[0].String() != tc.addr {
+			t.Errorf("LookupStaticHostV4(%s) = %v, want [%s]", tc.name, addrs, tc.addr)
+		}
+	}
+	if addrs := h.LookupStaticHostV4("orphan.example.org."); len(addrs) != 0 {
+		t.Errorf("LookupStaticHostV4(orphan.example.org.) = %v, want []", addrs)
+	}
+
+	got := logBuf.String()
+	for _, want := range []string{
+		`[ERROR] plugin/hosts: Hosts file "/tmp/hosts.test", line 2:`,
+		`[ERROR] plugin/hosts: Hosts file "/tmp/hosts.test", line 3:`,
+		"dropping the line",
+		"dropping the name",
+	} {
+		if !strings.Contains(got, want) {
+			t.Errorf("Expected log to contain %q, got %q", want, got)
+		}
+	}
+	// One report per dropped field, not one per read: the field on line 3 is
+	// assembled across several calls to append.
+	if n := strings.Count(got, "[ERROR] plugin/hosts:"); n != 2 {
+		t.Errorf("Expected exactly 2 reports, got %d in %q", n, got)
+	}
+}
+
+func TestParseLogsOversizedFieldSpanningReads(t *testing.T) {
+	// The line number must survive a field that crosses the read buffer: feed
+	// runs once per chunk, but only the last chunk of a line ends it. A field
+	// this long is also reported once, not once per read.
+	var logBuf bytes.Buffer
+	golog.SetOutput(&logBuf)
+	defer golog.SetOutput(io.Discard)
+
+	long := strings.Repeat("a", 5<<20)
+	h := &Hostsfile{
+		Origins: []string{"."},
+		hmap:    newMap(),
+		inline:  newMap(),
+		options: newOptions(),
+		path:    "/tmp/hosts.test",
+	}
+	h.hmap = h.parse(strings.NewReader(
+		"127.0.0.1 before.example.org\n" +
+			"127.0.0.2 " + long + ".example.org one.example.org\n" +
+			"127.0.0.3 after.example.org\n"))
+
+	for _, tc := range []struct{ name, addr string }{
+		{"one.example.org.", "127.0.0.2"},
+		{"after.example.org.", "127.0.0.3"},
+	} {
+		if addrs := h.LookupStaticHostV4(tc.name); len(addrs) != 1 || addrs[0].String() != tc.addr {
+			t.Errorf("LookupStaticHostV4(%s) = %v, want [%s]", tc.name, addrs, tc.addr)
+		}
+	}
+
+	got := logBuf.String()
+	if want := fmt.Sprintf("line 2: name longer than %d bytes", maxFieldSize); !strings.Contains(got, want) {
+		t.Errorf("Expected log to contain %q, got %q", want, got)
+	}
+	if n := strings.Count(got, "[ERROR] plugin/hosts:"); n != 1 {
+		t.Errorf("Expected exactly 1 report, got %d in %q", n, got)
+	}
+}
+
+func TestInitInlineReportsItsOwnSource(t *testing.T) {
+	// Inline entries are parsed with the same parser but do not come from the
+	// hosts file, so naming the file in a diagnostic would send the operator to
+	// the wrong place, at a line number that does not exist there.
+	var logBuf bytes.Buffer
+	golog.SetOutput(&logBuf)
+	defer golog.SetOutput(io.Discard)
+
+	h := &Hostsfile{
+		Origins: []string{"."},
+		hmap:    newMap(),
+		inline:  newMap(),
+		options: newOptions(),
+		path:    "/tmp/hosts.test",
+	}
+	h.initInline([]string{
+		"127.0.0.1 first.example.org",
+		strings.Repeat("a", maxFieldSize+1) + " orphan.example.org",
+	})
+
+	if addrs := h.inline.name4["first.example.org."]; len(addrs) != 1 {
+		t.Errorf("inline name4[first.example.org.] = %v, want one address", addrs)
+	}
+
+	got := logBuf.String()
+	if want := "Inline hosts entries, line 2: address longer"; !strings.Contains(got, want) {
+		t.Errorf("Expected log to contain %q, got %q", want, got)
+	}
+	if strings.Contains(got, "/tmp/hosts.test") {
+		t.Errorf("Inline entries reported as coming from the hosts file: %q", got)
+	}
+}