Repository navigation
Conversation
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. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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/ 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>
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. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The error returned by parseParts is either ignored by the callers (malformed record), or can never happen (strconv.Atoi and strconv.ParseInt only return ErrSyntax or ErrRange). Drop it, together with the error handling in the callers, so that parseParts just returns a bool. While at it, drop the extra TrimSpace and empty line checks from parseLine and parseParts: the callers already do that, and bytes.Split never returns an empty slice. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since UIDs and GIDs are parsed using strconv.Atoi, a passwd or group record with a negative ID, or an ID above maxID, is accepted, and GetExecUser returns it as is. For example, "adm:x:-1:-1::/:" results in Uid and Gid being -1, which setresuid/setresgid treat as "do not change", so the process keeps its current credentials. In addition, the result is platform-dependent: on 32-bit platforms 2147483648 overflows int and the record is skipped, while on 64-bit it is accepted. This also made the tests fail to compile on 32-bit platforms. Parse such fields using parseNumeric, which checks the range, so that records with out-of-range IDs are treated as malformed and skipped, consistently on all platforms. This makes the validateID check in GetAdditionalGroups redundant, so remove it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
ParseGroupFilter skips lines starting with '#', following glibc, but ParsePasswdFilter does not. As a result, a commented out entry such as "#olduser:x:0:0::/root:/bin/sh" is returned as a user named "#olduser" with UID 0, which GetExecUser can then match by UID. Skip comments in ParsePasswdFilter, too. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For an empty string, parseGroupArg returns nil, nil, which is then dereferenced, resulting in a nil pointer dereference panic. Return an error instead. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
glibc skips whitespace before group members and drops empty ones, so a group line like "grp:x:1234:root, adm,,user" has three members: "root", "adm", and "user". We kept " adm" and "" as is, so user adm was not considered a member of grp, and GetExecUser silently omitted the group from the supplementary groups. Trim whitespace around group members (a member name with a trailing space can not match a valid user name anyway), and drop empty ones. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
ParseSubIDFilter accepts records which shadow-utils rejects, such as
ones with negative or too large IDs or counts ("user:-1:65536",
"user:4294967296:1"), with an empty name, or with an extra field.
Validate records the same way as shadow-utils' subordinate_parse
does: there must be exactly three fields, the name must not be empty,
and the range must fit into 32-bit IDs. Skip records that fail the
validation.
Rename TestParseSubIDFilterOverflow to TestParseSubIDFilter, and add
more test cases.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
force-pushed
the
user-parse-fixes
branch
from
October 30, 2026 04:41
8254db7 to
5991029
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a carry of #248 fixing a few more issues on top of it. If that's acceptable I can rework the series to be more logical (currently it's more like fixup upon fixup)