[thaJeztah]

• fixes user: parseLine silently ignores uid/gid conversion errors, malformed ids parse as 0 #248

user: split numeric ID parsing from name detection

Split parseNumeric into a strict numeric parser and a parseMaybeNumeric
wrapper that allows non-numeric values to be interpreted as user or group
names.

This allows passwd and group file parsing to reject invalid numeric IDs
without changing the behavior of callers that accept names.

user: skip empty and malformed records

Skip empty and malformed records when parsing passwd and group files,
instead of accepting incomplete records or aborting on invalid numeric
fields.

Update parseLine and parseParts to distinguish malformed records from
other parsing errors, allowing callers to skip invalid entries while
still propagating unexpected errors.

Treat missing fields as empty, allowing empty string fields while
rejecting missing or invalid numeric fields. This aligns with glibc,
which skips malformed records when reading passwd and group files.

Add tests for empty, incomplete, and malformed records, and document
the behavior based on the corresponding glibc implementations.

For reference:

• https://sources.debian.org/src/glibc/2.42-13/nss/nss_fgetent_r.c/
• https://sources.debian.org/src/glibc/2.42-13/nss/fgetpwent_r.c/

user: distinguish malformed records from parsing errors

Treat out-of-range numeric fields as malformed records, consistent
with missing and non-numeric fields.

Use the ok return value from parseLine and parseParts to distinguish
malformed records, which should be skipped, from other parsing errors,
which should be propagated to the caller.

[kolyshkin]

The second patch seems problematic: while it says "skip malformed and empty lines", in fact it returns an error where previously it was ignored.

I think we should merely skip lines that do not conform to the specification, otherwise there will be regressions.

[thaJeztah]

The second patch seems problematic: while it says "skip malformed and empty lines", in fact it returns an error where previously it was ignored.

Yes, I need to clean-up my patch set; temporarily pushed, because I think the second (add error return) patch needed updates to the tests.

I think we should merely skip lines that do not conform to the specification, otherwise there will be regressions.

This was the topic of discussion (so was reordering the patches to make the "skip" last);

Old code wasn't good, because it would silently discard invalid UID/GID (numeric) values; it would use 0 if parsing failed, and preserve malformed records. This would not always be problematic (i.e., if it's not the record we were looking for), but of course still collect records that are just baggage.

The trickier question is what to do if the file contains malformed line(s)?

Currently (see above) we "half" ignore them (bad). Which, even with this patch, may still be true;

We silently ignore truncated lines;

// Ignore cases where we don't have enough fields to populate the arguments.
// Some configuration files like to misbehave.
if len(v) <= i {
    break
}

So a line like adm:x:3 is still treated as an implicit adm:x:3:0 (GID=0)

And we still consider empty fields;

case *int64:
    if len(p) != 0 {
        n, err := strconv.ParseInt(string(p), 10, 64)
        if err != nil {
            return true, fmt.Errorf("parsing integer field %d: %w", i, err)
        }
        *e = n
    }

So adm:x::::::::::: Would be implicicly taken as UID=0,GID=0 ?

The slightly less problematic case is ignoring completely malformed lines, and it's less problematic if such a record was not the one we were looking for (i.e., we wouldn't use it anyway), but of course, if it's malformed, it COULD have been the line we were looking for, and I think we pick "first match", so the malformed (or incoplete) line would shadow the valid one;

adm:x:3
adm:x:42:43:adm:/var/adm:/bin/false

If we skip such lines or error out, that would prevent that (but probably needs more than this patch).

[thaJeztah]

Looks like getent skips over malformed lines;
https://sources.debian.org/src/glibc/2.42-13/nss/fgetpwent_r.c/
https://sources.debian.org/src/glibc/2.42-13/nss/nss_fgetent_r.c/#L46-L48

  if (ret == EINVAL)
    /* Skip over malformed lines.  */
    continue;

Older implementations;
https://sources.debian.org/src/glibc/2.31-13%2Bdeb11u11/pwd/fgetpwent_r.c/#L100-L104

    } while (*p == '\0' || *p == '#' /* Ignore empty and comment lines.  */
	     /* Parse the line.  If it is invalid, loop to
		get the next line of the file to parse.  */
	     || ! parse_line (p, resbuf, (void *) buffer, buflen, &errno));

But it looks like it does reject lines with empty numeric columns;
https://sources.debian.org/src/glibc/2.42-13/nss/fgetpwent_r.c#L57-L58

	 INT_FIELD (result->pw_uid, ISCOLON, 0, 10,)
	 INT_FIELD (result->pw_gid, ISCOLON, 0, 10,)

Except for + special case?
https://sources.debian.org/src/glibc/2.42-13/nss/fgetpwent_r.c#L33-L46

 STRING_FIELD (result->pw_name, ISCOLON, 0);
 if (line[0] == '\0'
     && (result->pw_name[0] == '+' || result->pw_name[0] == '-'))
   {
     /* This a special case.  We allow lines containing only a `+' sign
	since this is used for nss_compat.  All other services will
	reject this entry later.  Initialize all other fields now.  */
     result->pw_passwd = NULL;
     result->pw_uid = 0;
     result->pw_gid = 0;
     result->pw_gecos = NULL;
     result->pw_dir = NULL;
     result->pw_shell = NULL;
   }

[thaJeztah]

Quick AI summary;

---

• Empty lines are not parse errors. They are recognized before parsing and skipped.
• Comment lines are also recognized before parsing and skipped. The comment syntax is simply a line whose first non-NUL character is # (i.e., the first character on the line is #; there is no support for trailing/end-of-line comments in passwd/group files).
• Malformed records do cause the parser to report a parse failure (EINVAL internally), but that failure is treated as "this record is invalid", not "the file cannot be read". The iterator discards the record and continues with the next line.
• Numeric fields (UID/GID for normal passwd entries) must contain valid integers. Empty or non-numeric values are considered malformed.
• String fields may be empty. For example, an empty GECOS, home directory, or shell is perfectly valid.

So the overall behavior is:

Input	Result
Empty line	skipped
# comment	skipped
Valid record	returned
Malformed record (missing required separators, bad UID/GID, etc.)	skipped
I/O error / scanner error	returned to caller

That's a fairly common pattern in libc: the iteration APIs (fgetpwent_r, fgetgrent_r, etc.) are designed to enumerate valid records from what may be a partially-corrupt file. A malformed line is treated as a bad record, not as a fatal error for the entire database.

I think that's a strong precedent for moby/sys: if the library is exposing "iterate over passwd/group records", then skipping malformed records without aborting the whole scan is consistent with glibc's behavior. The only remaining design question is what constitutes malformed—and I'd argue empty numeric fields should be included in that category if the goal is to match glibc closely.

[thaJeztah]

And for the + / - special case;

---

+ / - (NIS) entries

One exception to the above is the historical NIS ("Yellow Pages") extension, where passwd entries starting with + or - are not normal passwd records, but directives for the NSS/NIS compatibility layer.

Examples:

+::::::::       # include all NIS users
+jane:::::::    # include one NIS user
-sam:::::::     # exclude one NIS user

glibc explicitly special-cases these records:

• Normal passwd entries (root, adm, ...):

  • UID/GID must contain valid integers.
  • Empty or invalid numeric fields → malformed → skipped.

• NIS + / - entries:

  • UID/GID may be empty.
  • Empty means "unspecified", not 0.

This exists solely for compatibility with the legacy NIS passwd format.

For moby/sys, it may be reasonable not to implement this compatibility extension. The library is parsing flat passwd/group files rather than implementing glibc's full NSS stack, and NIS is effectively obsolete on modern Linux systems. In that case, +/- entries could simply be treated as unsupported/malformed records and skipped.

[thaJeztah]

note to self; this should not use parseNumeric, because it's too permissive. Perhaps we should rename it to parseMaybeNumeric to be more clear on intent (it's used for usernames that may be numeric);

sys/user/user.go

Lines 245 to 253 in 4a8455d

	func parseNumeric(val string) (int, bool, error) {
	id, err := strconv.Atoi(val)
	if err != nil {
	if errors.Is(err, strconv.ErrSyntax) {
	// Discard the error, because non-numeric values are expected
	// when passing a username or group-name.
	return 0, false, nil
	}
	if errors.Is(err, strconv.ErrRange) {

[Copilot]

🟡 Changes recommended

The shared numeric parser imposes a MaxInt32 limit on passwd/group records, silently dropping previously supported large IDs on 64-bit systems.

1 open issue

• Avoid MaxInt32 limit when parsing passwd and group IDs · New

1 resolved since last review

• Return ok=false for overflowing int64 filter fields

_{Review effort: Balanced}

[kolyshkin]

A few issues outside of the diff hunks (so not inline):

1. GetExecUser (user.Uid = users[0].Uid, and the same for Gid) copies the parsed IDs without passing them through the new validateID. The description says out-of-range numeric fields make a record malformed, but parseParts only rejects strconv overflow, so negative IDs and IDs above maxID still get through. For example, adm:x:-1:-1::/: with GetExecUser("adm", ...) returns Uid: -1, Gid: -1 and a nil error. A caller that does setresuid(-1, ...) with that changes nothing, so the process keeps its current (often root) credentials. On 64-bit, adm:x:2147483648:4: is also accepted. GetAdditionalGroups does call validateID, so the two paths are inconsistent. I suggest validating the range for Uid/Gid (either in the parsers, treating such records as malformed, or in GetExecUser).

2. GetAdditionalGroups([]string{""}, r) panics with a nil pointer dereference: parseGroupArg("") returns nil, nil, and then *groupArg is dereferenced without a check. This is not new, but this PR touches the function, and now groups with empty names are accepted (:x:0: is in the expected test output), which makes empty group strings more likely to reach this code.

_{This review was written by an AI assistant (Claude) on my behalf.}

[kolyshkin]

2147483648 overflows int on 32-bit platforms, so the test package does not compile there: GOARCH=386 go vet ./user fails with cannot use 2147483648 (untyped int constant) as int value in struct literal (overflows). CI only runs tests on amd64, so it doesn't catch this.

Even if this compiled, on 32-bit Atoi("2147483648") returns ErrRange and the record would be skipped, so the expected result is platform-dependent. Maybe move these cases into a separate test guarded by strconv.IntSize == 64, or use a value that is out of range on all platforms.

[kolyshkin]

Same as above: 2147483648 overflows int on 32-bit.

[kolyshkin]

ParsePasswdFilter still does not skip # comment lines, unlike ParseGroupFilter and unlike glibc, which this PR says it follows. A commented-out entry whose numeric fields are valid is returned as a real user: #olduser:x:0:0::/root:/bin/sh gives a user named #olduser with UID 0, which the default GetExecUser filter (u.Uid == user.Uid) can match. The new skipping only drops comment lines that happen to fail numeric parsing.

[kolyshkin]

This changes the behavior of the public parsers without saying so: lines that used to be returned with zeroed numeric fields are now silently dropped. That covers passwd/group lines with empty or non-numeric IDs, and subuid/idmap lines with missing counts (e.g. name:100000 used to give Count: 0). It's probably the right thing to do, but it's worth calling out in the PR description / commit message, since callers that counted or validated entries lose that signal.

[kolyshkin]

These return true, fmt.Errorf(...) branches (here and in the *int64 case) can't be reached: strconv.Atoi and strconv.ParseInt(s, 10, 64) only return ErrSyntax or ErrRange. So the (bool, error) contract, and the four if err != nil { return nil, fmt.Errorf("invalid line %q: %w", ...) } blocks in the callers, are dead code, and they make readers think some lines can abort parsing. Simpler: have parseParts return just ok bool.

[kolyshkin]

For every malformed line this builds a formatted error (fmt.Errorf with %q in the ErrSyntax case), and every caller throws it away because it checks !ok first. For subuid/subgid files, which have no # skip, every comment line goes through this path. Returning just false (or a sentinel error) avoids the allocation.

[kolyshkin]

The *int and *int64 cases repeat the same error classification and differ only in the parse call. Maybe parse once with strconv.ParseInt(string(p), 10, 64) (or bitSize 0 for *int), handle the error once, then assign to the target type.

[kolyshkin]

All callers already do line := bytes.TrimSpace(s.Bytes()); if len(line) == 0 { continue }, so this trim and empty check never do anything, and neither does len(parts) == 0 in parseParts (bytes.Split never returns an empty slice). I suggest keeping this check in one place.