[WIP] user: skip malformed and empty lines

user/user.go

@@ -56,13 +56,13 @@ type IDMap struct {
 	Count    int64
 }
 
-func parseLine(line []byte, v ...any) {
-	parseParts(bytes.Split(line, []byte(":")), v...)
+func parseLine(line []byte, v ...any) error {
+	return parseParts(bytes.Split(line, []byte(":")), v...)
 }
 
-func parseParts(parts [][]byte, v ...any) {
+func parseParts(parts [][]byte, v ...any) error {
 	if len(parts) == 0 {
-		return
+		return nil
 	}
 
 	for i, p := range parts {
@@ -78,10 +78,24 @@ func parseParts(parts [][]byte, v ...any) {
 		case *string:
 			*e = string(p)
 		case *int:
-			// "numbers", with conversion errors ignored because of some misbehaving configuration files.
-			*e, _ = strconv.Atoi(string(p))
+			if len(p) != 0 {
+				n, ok, err := parseNumeric(string(p))
+				if err != nil {
+					return fmt.Errorf("parsing integer field %d: %w", i, err)
+				}
+				if !ok {
+					return fmt.Errorf("parsing integer field %d: %q is not numeric", i, p)
+				}
+				*e = n
+			}
 		case *int64:
-			*e, _ = strconv.ParseInt(string(p), 10, 64)
+			if len(p) != 0 {
+				n, err := strconv.ParseInt(string(p), 10, 64)
+				if err != nil {
+					return fmt.Errorf("parsing integer field %d: %w", i, err)
+				}
+				*e = n
+			}
 		case *[]string:
 			// Comma-separated lists.
 			if len(p) != 0 {
@@ -94,6 +108,7 @@ func parseParts(parts [][]byte, v ...any) {
 			panic(fmt.Sprintf("parseLine only accepts {*string, *int, *int64, *[]string} as arguments! %#v is not a pointer!", e))
 		}
 	}
+	return nil
 }
 
 func ParsePasswdFile(path string) ([]User, error) {
@@ -135,7 +150,9 @@ func ParsePasswdFilter(r io.Reader, filter func(User) bool) ([]User, error) {
 		//  root:x:0:0:root:/root:/bin/bash
 		//  adm:x:3:4:adm:/var/adm:/bin/false
 		p := User{}
-		parseLine(line, &p.Name, &p.Pass, &p.Uid, &p.Gid, &p.Gecos, &p.Home, &p.Shell)
+		if err := parseLine(line, &p.Name, &p.Pass, &p.Uid, &p.Gid, &p.Gecos, &p.Home, &p.Shell); err != nil {
+			return nil, fmt.Errorf("invalid line (%q): %w", string(line), err)
+		}
 
 		if filter == nil || filter(p) {
 			out = append(out, p)
@@ -194,7 +211,9 @@ func ParseGroupFilter(r io.Reader, filter func(Group) bool) ([]Group, error) {
 		//  root:x:0:root
 		//  adm:x:4:root,adm,daemon
 		p := Group{}
-		parseLine(line, &p.Name, &p.Pass, &p.Gid, &p.List)
+		if err := parseLine(line, &p.Name, &p.Pass, &p.Gid, &p.List); err != nil {
+			return nil, fmt.Errorf("invalid line (%q): %w", string(line), err)
+		}
 
 		if filter == nil || filter(p) {
 			out = append(out, p)
@@ -508,7 +527,9 @@ func ParseSubIDFilter(r io.Reader, filter func(SubID) bool) ([]SubID, error) {
 
 		// see: man 5 subuid
 		p := SubID{}
-		parseLine(line, &p.Name, &p.SubID, &p.Count)
+		if err := parseLine(line, &p.Name, &p.SubID, &p.Count); err != nil {
+			return nil, fmt.Errorf("invalid line (%q): %w", string(line), err)
+		}
 
 		if filter == nil || filter(p) {
 			out = append(out, p)
@@ -556,7 +577,9 @@ func ParseIDMapFilter(r io.Reader, filter func(IDMap) bool) ([]IDMap, error) {
 
 		// see: man 7 user_namespaces
 		p := IDMap{}
-		parseParts(bytes.Fields(line), &p.ID, &p.ParentID, &p.Count)
+		if err := parseParts(bytes.Fields(line), &p.ID, &p.ParentID, &p.Count); err != nil {
+			return nil, fmt.Errorf("invalid line (%q): %w", string(line), err)
+		}
 
 		if filter == nil || filter(p) {
 			out = append(out, p)

user/user_test.go

@@ -21,42 +21,66 @@ func TestParseLine(t *testing.T) {
 		d    int
 	)
 
-	parseLine([]byte(""), &a, &b)
+	err := parseLine([]byte(""), &a, &b)
+	if err != nil {
+		t.Fatal(err)
+	}
 	if a != "" || b != "" {
 		t.Fatalf("a and b should be empty ('%v', '%v')", a, b)
 	}
 
-	parseLine([]byte("a"), &a, &b)
+	err = parseLine([]byte("a"), &a, &b)
+	if err != nil {
+		t.Fatal(err)
+	}
 	if a != "a" || b != "" {
 		t.Fatalf("a should be 'a' and b should be empty ('%v', '%v')", a, b)
 	}
 
-	parseLine([]byte("bad boys:corny cows"), &a, &b)
+	err = parseLine([]byte("bad boys:corny cows"), &a, &b)
+	if err != nil {
+		t.Fatal(err)
+	}
 	if a != "bad boys" || b != "corny cows" {
 		t.Fatalf("a should be 'bad boys' and b should be 'corny cows' ('%v', '%v')", a, b)
 	}
 
-	parseLine([]byte(""), &c)
+	err = parseLine([]byte(""), &c)
+	if err != nil {
+		t.Fatal(err)
+	}
 	if len(c) != 0 {
 		t.Fatalf("c should be empty (%#v)", c)
 	}
 
-	parseLine([]byte("d,e,f:g:h:i,j,k"), &c, &a, &b, &c)
+	err = parseLine([]byte("d,e,f:g:h:i,j,k"), &c, &a, &b, &c)
+	if err != nil {
+		t.Fatal(err)
+	}
 	if a != "g" || b != "h" || len(c) != 3 || c[0] != "i" || c[1] != "j" || c[2] != "k" {
 		t.Fatalf("a should be 'g', b should be 'h', and c should be ['i','j','k'] ('%v', '%v', '%#v')", a, b, c)
 	}
 
-	parseLine([]byte("::::::::::"), &a, &b, &c)
+	err = parseLine([]byte("::::::::::"), &a, &b, &c)
+	if err != nil {
+		t.Fatal(err)
+	}
 	if a != "" || b != "" || len(c) != 0 {
 		t.Fatalf("a, b, and c should all be empty ('%v', '%v', '%#v')", a, b, c)
 	}
 
-	parseLine([]byte("not a number"), &d)
+	err = parseLine([]byte("not a number"), &d)
+	if err == nil {
+		t.Fatal("expected an error")
+	}
 	if d != 0 {
 		t.Fatalf("d should be 0 (%v)", d)
 	}
 
-	parseLine([]byte("b:12:c"), &a, &d, &b)
+	err = parseLine([]byte("b:12:c"), &a, &d, &b)
+	if err != nil {
+		t.Fatal(err)
+	}
 	if a != "b" || b != "c" || d != 12 {
 		t.Fatalf("a should be 'b' and b should be 'c', and d should be 12 ('%v', '%v', %v)", a, b, d)
 	}
@@ -548,12 +572,6 @@ this is just some garbage data
 }
 
 func TestGetAdditionalGroups(t *testing.T) {
-	type foo struct {
-		groups   []string
-		expected []int
-		hasError bool
-	}
-
 	groupContent := `
 root:x:0:root
 adm:x:43:
@@ -564,88 +582,92 @@ adm:x:4343:root,adm-duplicate
 toolarge:x:2147483648:
 this is just some garbage data
 ` + largeGroup()
-	tests := []foo{
+	tests := []struct {
+		doc      string
+		groups   []string
+		expected []int
+		hasError bool
+	}{
 		{
-			// empty group
+			doc:      "empty group",
 			groups:   []string{},
 			expected: []int{},
 		},
 		{
-			// single group
+			doc:      "single group",
 			groups:   []string{"adm"},
 			expected: []int{43},
 		},
 		{
-			// numeric group miss must continue checking remaining groups
+			doc:      "numeric group miss must continue checking remaining groups",
 			groups:   []string{"10001", "adm"},
 			expected: []int{43, 10001},
 		},
 		{
-			// multiple groups
+			doc:      "multiple groups",
 			groups:   []string{"adm", "grp"},
 			expected: []int{43, 1234},
 		},
 		{
-			// invalid group
+			doc:      "invalid group",
 			groups:   []string{"adm", "grp", "not-exist"},
 			expected: nil,
 			hasError: true,
 		},
 		{
-			// group with numeric id
+			doc:      "group with numeric id",
 			groups:   []string{"43"},
 			expected: []int{43},
 		},
 		{
-			// group with unknown numeric id
+			doc:      "group with unknown numeric id",
 			groups:   []string{"adm", "10001"},
 			expected: []int{43, 10001},
 		},
 		{
-			// groups specified twice with numeric and name
+			doc:      "groups specified twice with numeric and name",
 			groups:   []string{"adm", "43"},
 			expected: []int{43},
 		},
 		{
-			// groups with too small id
+			doc:      "groups with negative id",
 			groups:   []string{"-1"},
 			expected: nil,
 			hasError: true,
 		},
 		{
-			// groups with too large id
+			doc:      "groups with too large id",
 			groups:   []string{strconv.FormatInt(1<<31, 10)},
 			expected: nil,
 			hasError: true,
 		},
 		{
-			// group with very long list of users
+			doc:      "group with very long list of users",
 			groups:   []string{"largegroup"},
 			expected: []int{1000},
 		},
 		{
-			// numeric group must not resolve as group name
+			doc:      "maxID + 1 group must not resolve as group name",
 			groups:   []string{"2147483648"},
 			expected: nil,
 			hasError: true,
 		},
 		{
-			// numeric group must not resolve as group name
+			doc:      "maxInt64+1 group must not resolve as group name",
 			groups:   []string{"9223372036854775808"},
 			expected: nil,
 			hasError: true,
 		},
 		{
-			// group entry with out-of-range gid
+			doc:      "group entry with out-of-range gid",
 			groups:   []string{"toolarge"},
 			expected: nil,
 			hasError: true,
 		},
 	}
 
 	for _, tc := range tests {
-		name := strings.Join(tc.groups, ",")
-		t.Run(name, func(t *testing.T) {
+		t.Run(tc.doc, func(t *testing.T) {
 			group := strings.NewReader(groupContent)
 
 			gids, err := GetAdditionalGroups(tc.groups, group)
@@ -665,26 +687,26 @@ this is just some garbage data
 
 func TestGetAdditionalGroupsNumeric(t *testing.T) {
 	tests := []struct {
+		doc      string
 		groups   []string
 		expected []int
 		hasError bool
 	}{
 		{
-			// numeric groups only
+			doc:      "numeric groups only",
 			groups:   []string{"1234", "5678"},
 			expected: []int{1234, 5678},
 		},
 		{
-			// numeric and alphabetic
+			doc:      "numeric and alphabetic",
 			groups:   []string{"1234", "fake"},
 			expected: nil,
 			hasError: true,
 		},
 	}
 
 	for _, tc := range tests {
-		name := strings.Join(tc.groups, ",")
-		t.Run(name, func(t *testing.T) {
+		t.Run(tc.doc, func(t *testing.T) {
 			gids, err := GetAdditionalGroups(tc.groups, nil)
 			if tc.hasError && err == nil {
 				t.Fatalf("Parse(%#v) expects error but has none", tc)