systemd: simplify parser and fix infinite loop

This commit simplifies the systemd parser logic, and it solves an
infinite loop when using a continuation line.

Closes: containers#24810

Signed-off-by: Giuseppe Scrivano <[email protected]>

Author: giuseppe

pkg/systemd/parser/unitfile.go

@@ -48,7 +48,6 @@ type UnitFileParser struct {
 
 	currentGroup    *unitGroup
 	pendingComments []*unitLine
-	lineNr          int
 }
 
 func newUnitLine(key string, value string, isComment bool) *unitLine {
@@ -347,7 +346,7 @@ func (p *UnitFileParser) parseKeyValuePair(line string) error {
 	return nil
 }
 
-func (p *UnitFileParser) parseLine(line string) error {
+func (p *UnitFileParser) parseLine(line string, lineNr int) error {
 	switch {
 	case lineIsComment(line):
 		return p.parseComment(line)
@@ -356,7 +355,7 @@ func (p *UnitFileParser) parseLine(line string) error {
 	case lineIsKeyValuePair(line):
 		return p.parseKeyValuePair(line)
 	default:
-		return fmt.Errorf("file contains line %d: “%s” which is not a key-value pair, group, or comment", p.lineNr, line)
+		return fmt.Errorf("file contains line %d: “%s” which is not a key-value pair, group, or comment", lineNr, line)
 	}
 }
 
@@ -376,53 +375,34 @@ func (p *UnitFileParser) flushPendingComments(toComment bool) {
 	}
 }
 
-func nextLine(data string, afterPos int) (string, string) {
-	rest := data[afterPos:]
-	if i := strings.Index(rest, "\n"); i >= 0 {
-		return strings.TrimSpace(data[:i+afterPos]), data[i+afterPos+1:]
-	}
-	return data, ""
-}
-
-func trimSpacesFromLines(data string) string {
-	lines := strings.Split(data, "\n")
-	for i, line := range lines {
-		lines[i] = strings.TrimSpace(line)
-	}
-	return strings.Join(lines, "\n")
-}
-
 // Parse an already loaded unit file (in the form of a string)
 func (f *UnitFile) Parse(data string) error {
 	p := &UnitFileParser{
-		file:   f,
-		lineNr: 1,
+		file: f,
 	}
 
-	data = trimSpacesFromLines(data)
-
-	for len(data) > 0 {
-		origdata := data
-		nLines := 1
-		var line string
-		line, data = nextLine(data, 0)
+	lines := strings.Split(strings.TrimSuffix(data, "\n"), "\n")
+	remaining := ""
 
+	for lineNr, line := range lines {
 		if !lineIsComment(line) {
-			// Handle multi-line continuations
-			// Note: This doesn't support comments in the middle of the continuation, which systemd does
-			if lineIsKeyValuePair(line) {
-				for len(data) > 0 && line[len(line)-1] == '\\' {
-					line, data = nextLine(origdata, len(line)+1)
-					nLines++
+			line = strings.TrimSpace(line)
+			if strings.HasSuffix(line, "\\") {
+				line = line[:len(line)-1]
+				// check whether the line is a continuation of the previous line
+				if lineNr != len(lines)-1 {
+					remaining += line
+					continue
 				}
 			}
+			if remaining != "" {
+				line = remaining + line
+				remaining = ""
+			}
 		}
-
-		if err := p.parseLine(line); err != nil {
+		if err := p.parseLine(line, lineNr+1); err != nil {
 			return err
 		}
-
-		p.lineNr += nLines
 	}
 
 	if p.currentGroup == nil {

pkg/systemd/parser/unitfile_test.go

@@ -2,6 +2,7 @@ package parser
 
 import (
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -119,7 +120,9 @@ After=dbus.socket
 
 [Service]
 BusName=org.freedesktop.login1
-CapabilityBoundingSet=CAP_SYS_ADMIN CAP_MAC_ADMIN CAP_AUDIT_CONTROL CAP_CHOWN CAP_DAC_READ_SEARCH CAP_DAC_OVERRIDE CAP_FOWNER CAP_SYS_TTY_CONFIG CAP_LINUX_IMMUTABLE
+CapabilityBoundingSet=CAP_SYS_ADMIN CAP_MAC_ADMIN CAP_AUDIT_CONTROL CAP_CHOWN CAP_DAC_READ_SEARCH CAP_DAC_OVERRIDE # comment inside a continuation line \
+CAP_FOWNER \
+CAP_SYS_TTY_CONFIG CAP_LINUX_IMMUTABLE
 DeviceAllow=block-* r
 DeviceAllow=char-/dev/console rw
 DeviceAllow=char-drm rw
@@ -158,8 +161,8 @@ SystemCallFilter=@system-service
 
 # Increase the default a bit in order to allow many simultaneous logins since
 # we keep one fd open per session.
-LimitNOFILE=524288
-`
+LimitNOFILE=524288  \`
+
 const systemdnetworkdService = `#  SPDX-License-Identifier: LGPL-2.1-or-later
 #
 #  This file is part of systemd.
@@ -264,6 +267,14 @@ var sampleDropinPaths = map[string][]string{
 	sampleDropinTemplateInstance: sampleDropinTemplateInstancePaths,
 }
 
+func filterComments(input string) string {
+	// merge continuation lines
+	joined := strings.ReplaceAll(input, "\\\n", "")
+
+	// remove trailing newlines and backslashes
+	return strings.TrimSuffix(strings.TrimSuffix(joined, "\n"), "\\")
+}
+
 func TestRanges_Roundtrip(t *testing.T) {
 	for i := range samples {
 		sample := samples[i]
@@ -278,7 +289,7 @@ func TestRanges_Roundtrip(t *testing.T) {
 			panic(e)
 		}
 
-		assert.Equal(t, sample, asStr)
+		assert.Equal(t, filterComments(sample), filterComments(asStr))
 	}
 }
 
@@ -292,3 +303,16 @@ func TestUnitDropinPaths_Search(t *testing.T) {
 		assert.True(t, reflect.DeepEqual(expectedPaths, generatedPaths))
 	}
 }
+
+func FuzzParser(f *testing.F) {
+	for _, sample := range samples {
+		f.Add([]byte(sample))
+	}
+
+	f.Fuzz(func(t *testing.T, orig []byte) {
+		unitFile := NewUnitFile()
+		unitFile.Path = "foo/bar"
+		unitFile.Filename = "bar"
+		_ = unitFile.Parse(string(orig))
+	})
+}

test/e2e/quadlet/emptyline.container

@@ -0,0 +1,4 @@
+[Container]
+Exec=true \
+
+# must have a blank line above, but this line can be anything (including another blank line)