[WIP] user: skip malformed and empty lines by thaJeztah · Pull Request #230 · moby/sys

thaJeztah · 2026-06-30T13:49:38Z

No description provided.

thaJeztah · 2026-07-08T12:39:53Z

+		ok, err := parseLine(line, &p.Name, &p.Pass, &p.Uid, &p.Gid, &p.Gecos, &p.Home, &p.Shell)
+		if err != nil || !ok {
+			// Skip malformed and empty lines, and don't consider them.
+			continue
+		}


Possibly also skip zero-records;

if p == (User{}) { continue }

thaJeztah · 2026-07-08T12:41:17Z

+		ok, err := parseLine(line, &p.Name, &p.Pass, &p.Gid, &p.List)
+		if err != nil || !ok {
+			// Skip malformed and empty lines, and don't consider them.
+			continue
+		}

 		if filter == nil || filter(p) {
 			out = append(out, p)


Here as well, but Group is not comparable (due to the List []string)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Return an error when failing to parse numeric values instead of silently discarding them. Co-authored-by: Mickael Emirkanian <mickael.emirkanian@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

kolyshkin · 2026-07-10T00:11:54Z

 		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)


nit: you have both () and "" around line, which seems excessive.

Suggested change

return nil, fmt.Errorf("invalid line (%q): %w", string(line), err)

return nil, fmt.Errorf("invalid line %q: %w", string(line), err)

Ah, yes, I think I was going back-and-forth on %q or (%s) - %q isn't always great due to escaping nested quotes if they can be expected.

kolyshkin · 2026-07-10T00:12:10Z

 		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)


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 · 2026-07-10T10:52:05Z

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 · 2026-07-10T11:16:18Z

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 · 2026-07-10T11:22:41Z

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 · 2026-07-10T11:24:56Z

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 force-pushed the parseparts_err branch 2 times, most recently from 1b8a8f3 to 36088f5 Compare June 30, 2026 19:05

thaJeztah force-pushed the parseparts_err branch 2 times, most recently from 706f6e5 to 15f6695 Compare July 8, 2026 12:25

thaJeztah changed the title ~~[WIP] user: skip malformed lines~~ [WIP] user: skip malformed and empty lines Jul 8, 2026

thaJeztah commented Jul 8, 2026

View reviewed changes

thaJeztah force-pushed the parseparts_err branch 2 times, most recently from ffef6e5 to 752b6ed Compare July 9, 2026 12:35

thaJeztah and others added 4 commits July 9, 2026 22:14

user: use custom names for some table tests

c8d93bb

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

user: parseLine, parseParts: add error-return

16c9767

Return an error when failing to parse numeric values instead of silently discarding them. Co-authored-by: Mickael Emirkanian <mickael.emirkanian@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

user: skip empty lines

a52ee4e

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

user: skip malformed lines

600b945

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the parseparts_err branch from 752b6ed to 600b945 Compare July 9, 2026 20:15

kolyshkin reviewed Jul 10, 2026

View reviewed changes

kolyshkin requested changes Jul 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] user: skip malformed and empty lines#230

[WIP] user: skip malformed and empty lines#230
thaJeztah wants to merge 4 commits into
moby:mainfrom
thaJeztah:parseparts_err

thaJeztah commented Jun 30, 2026

Uh oh!

thaJeztah Jul 8, 2026

Uh oh!

thaJeztah Jul 8, 2026

Uh oh!

kolyshkin Jul 10, 2026

Uh oh!

thaJeztah Jul 10, 2026

Uh oh!

kolyshkin Jul 10, 2026

Uh oh!

kolyshkin left a comment

Uh oh!

thaJeztah commented Jul 10, 2026

Uh oh!

thaJeztah commented Jul 10, 2026

Uh oh!

thaJeztah commented Jul 10, 2026

Uh oh!

thaJeztah commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	return nil, fmt.Errorf("invalid line (%q): %w", string(line), err)
	return nil, fmt.Errorf("invalid line %q: %w", string(line), err)

Uh oh!

Conversation

thaJeztah commented Jun 30, 2026

Uh oh!

thaJeztah Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

kolyshkin Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

kolyshkin Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jul 10, 2026

Uh oh!

thaJeztah commented Jul 10, 2026

Uh oh!

thaJeztah commented Jul 10, 2026

Uh oh!

thaJeztah commented Jul 10, 2026

+ / - (NIS) entries

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

`+` / `-` (NIS) entries