[pkg/dogstatsd] Fix parseEvent header parsing with negative values (#…

…7164)

When `parseEvent` was invoked with a header that contained a negative
value either in the "titleLength" or the "textLength" fields, we were
not correctly discarding that event as erroneous, causing a panic. This
change ensures that a header containing those values will now correctly
return a parsing error instead.

pkg/dogstatsd/parse_events.go

@@ -68,24 +68,38 @@ func splitHeaderEvent(message []byte) ([]byte, []byte, error) {
 }
 
 func parseHeader(rawHeader []byte) (eventHeader, error) {
+	// Basic sanity check on the header (minimum length)
 	if len(rawHeader) < 7 {
 		return eventHeader{}, fmt.Errorf("invalid event header: %q", rawHeader)
 	}
+
+	// Extract out the raw title and text lengths
 	rawLengths := rawHeader[3 : len(rawHeader)-1]
 	sepIndex := bytes.Index(rawLengths, commaSeparator)
 	if sepIndex == -1 {
 		return eventHeader{}, fmt.Errorf("invalid event header: %q", rawHeader)
 	}
+
 	rawTitleLength := rawLengths[:sepIndex]
 	rawTextLength := rawLengths[sepIndex+1:]
+
+	// Convert title length to workable type and do a basic validity check on value
 	titleLength, err := parseInt64(rawTitleLength)
-	if err != nil {
+	if err != nil || titleLength < 0 {
 		return eventHeader{}, fmt.Errorf("invalid event header: %q", rawHeader)
 	}
+
+	// Ensure that title isn't empty
+	if titleLength == 0 {
+		return eventHeader{}, fmt.Errorf("invalid event: empty title")
+	}
+
+	// Convert text length to workable type and do a basic validity check on value
 	textLength, err := parseInt64(rawTextLength)
-	if err != nil {
+	if err != nil || textLength < 0 {
 		return eventHeader{}, fmt.Errorf("invalid event header: %q", rawHeader)
 	}
+
 	return eventHeader{
 		titleLength: int(titleLength),
 		textLength:  int(textLength),
@@ -154,16 +168,16 @@ func (p *parser) parseEvent(message []byte) (dogstatsdEvent, error) {
 	if err != nil {
 		return dogstatsdEvent{}, err
 	}
+
 	header, err := parseHeader(rawHeader)
 	if err != nil {
 		return dogstatsdEvent{}, err
 	}
+
 	if len(rawEvent) < header.textLength+header.titleLength+1 {
 		return dogstatsdEvent{}, fmt.Errorf("invalid event")
 	}
-	if header.titleLength == 0 {
-		return dogstatsdEvent{}, fmt.Errorf("invalid event: empty title")
-	}
+
 	title := cleanEventText(rawEvent[:header.titleLength])
 	text := cleanEventText(rawEvent[header.titleLength+1 : header.titleLength+1+header.textLength])
 

pkg/dogstatsd/parse_events_test.go

@@ -63,10 +63,13 @@ func TestEventError(t *testing.T) {
 	_, err = parseEvent([]byte("_e{10,10}:title|text"))
 	assert.Error(t, err)
 
-	// zero length
+	// zero length title
 	_, err = parseEvent([]byte("_e{0,0}:a|a"))
 	assert.Error(t, err)
 
+	_, err = parseEvent([]byte("_e{0,4}:text"))
+	assert.Error(t, err)
+
 	// missing title or text length
 	_, err = parseEvent([]byte("_e{5555:title|text"))
 	assert.Error(t, err)
@@ -98,6 +101,18 @@ func TestEventError(t *testing.T) {
 	_, err = parseEvent([]byte("_e:|text"))
 	assert.Error(t, err)
 
+	// invalid title length
+	_, err = parseEvent([]byte("_e{-123,-987}:"))
+	assert.Error(t, err)
+
+	// invalid text length
+	_, err = parseEvent([]byte("_e{5,-987}:title"))
+	assert.Error(t, err)
+
+	// malformed message
+	_, err = parseEvent([]byte("_e{0001,-9876"))
+	assert.Error(t, err)
+
 	// invalid timestamp
 	_, err = parseEvent([]byte("_e{5,4}:title|text|d:abc"))
 	assert.NoError(t, err)

pkg/dogstatsd/server_test.go

@@ -309,8 +309,14 @@ func TestUDPReceive(t *testing.T) {
 		assert.FailNow(t, "Timeout on receive channel")
 	}
 
-	// Test erroneous Event
-	conn.Write([]byte("_e{0,9}:|test text\n_e{11,10}:test title2|test\\ntext|t:warning|d:12345|p:low|h:some.host|k:aggKey|s:source test|#tag1,tag2:test"))
+	// Test erroneous Events
+	conn.Write(
+		[]byte("_e{0,9}:|test text\n" +
+			"_e{-5,2}:abc\n" +
+			"_e{11,10}:test title2|test\\ntext|" +
+			"t:warning|d:12345|p:low|h:some.host|k:aggKey|s:source test|#tag1,tag2:test",
+		),
+	)
 	select {
 	case res := <-eventOut:
 		assert.Equal(t, 1, len(res))

releasenotes/notes/fix-dogstatd-event-header-offset-parsing-f879851ece9974aa.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fix parsing of dogstatsd event strings that contained negative lengths for
+    event title and/or event text length.