From 64e94efb95e099db93ee8a28735549268490190f Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 12 Dec 2024 15:19:18 +0100 Subject: [PATCH] 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: https://github.com/containers/podman/issues/24810 Signed-off-by: Giuseppe Scrivano --- pkg/systemd/parser/unitfile.go | 64 +++++++++++----------------- pkg/systemd/parser/unitfile_test.go | 42 ++++++++++++++++-- test/e2e/quadlet/emptyline.container | 4 ++ 3 files changed, 66 insertions(+), 44 deletions(-) create mode 100644 test/e2e/quadlet/emptyline.container diff --git a/pkg/systemd/parser/unitfile.go b/pkg/systemd/parser/unitfile.go index 84be6ab623..0fefdb138d 100644 --- a/pkg/systemd/parser/unitfile.go +++ b/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,39 @@ 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 := "" - 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++ + for lineNr, line := range lines { + line = strings.TrimSpace(line) + if lineIsComment(line) { + // ignore the comment is inside a continuation line. + if remaining != "" { + continue + } + } else { + if strings.HasSuffix(line, "\\") { + line = line[:len(line)-1] + if lineNr != len(lines)-1 { + remaining += line + continue } } + // check whether the line is a continuation of the previous line + 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 { @@ -690,7 +675,6 @@ func (f *UnitFile) LookupInt(groupName string, key string, defaultValue int64) i } intVal, err := convertNumber(v) - if err != nil { return defaultValue } diff --git a/pkg/systemd/parser/unitfile_test.go b/pkg/systemd/parser/unitfile_test.go index ea805ddcdc..72b5db65e2 100644 --- a/pkg/systemd/parser/unitfile_test.go +++ b/pkg/systemd/parser/unitfile_test.go @@ -2,6 +2,7 @@ package parser import ( "reflect" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -119,7 +120,10 @@ 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 +162,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 +268,23 @@ var sampleDropinPaths = map[string][]string{ sampleDropinTemplateInstance: sampleDropinTemplateInstancePaths, } +func filterComments(input string) string { + lines := strings.Split(input, "\n") + filtered := make([]string, 0, len(lines)) + for _, line := range lines { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "#") || strings.HasPrefix(line, ";") { + continue + } + filtered = append(filtered, line) + } + // merge continuation lines + joined := strings.ReplaceAll(strings.Join(filtered, "\n"), "\\\n", "") + + // and remove any trailing new line, backslash or space + return strings.TrimRight(joined, "\n\\ ") +} + func TestRanges_Roundtrip(t *testing.T) { for i := range samples { sample := samples[i] @@ -278,7 +299,7 @@ func TestRanges_Roundtrip(t *testing.T) { panic(e) } - assert.Equal(t, sample, asStr) + assert.Equal(t, filterComments(sample), filterComments(asStr)) } } @@ -292,3 +313,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)) + }) +} diff --git a/test/e2e/quadlet/emptyline.container b/test/e2e/quadlet/emptyline.container new file mode 100644 index 0000000000..f7aba88d66 --- /dev/null +++ b/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)