Skip to content

pwd: implement missing functions#480

Open
julianuziemblo wants to merge 1 commit into
masterfrom
julianuziemblo/RTOS-1341
Open

pwd: implement missing functions#480
julianuziemblo wants to merge 1 commit into
masterfrom
julianuziemblo/RTOS-1341

Conversation

@julianuziemblo

@julianuziemblo julianuziemblo commented May 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

implement: getpwent, setpwent, endpwent, getpwnam_r, getpwuid_r

YT: RTOS-1341

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing linter checks and tests passed.
  • My changes generate no new compilation warnings for any of the targets.

Special treatment

  • This PR needs additional PRs to work (list the PRs, preferably in merge-order).
  • I will merge this PR by myself when appropriate.

github-actions[bot]
github-actions Bot reviewed May 28, 2026
View reviewed changes
Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c Outdated
@julianuziemblo julianuziemblo mentioned this pull request May 28, 2026
Open
14 tasks
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/RTOS-1341 branch from 61fcc68 to 25b46d7 Compare May 28, 2026 10:03
gemini-code-assist[bot]
gemini-code-assist Bot reviewed May 28, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds POSIX-compliant reentrant and non-reentrant password database lookup functions (getpwuid_r, getpwent, setpwent, endpwent) and refactors existing password parsing logic in unistd/pwd.c. The review identifies several critical issues: a potential buffer overflow in readpwentry when maxlen is 0 due to unsigned underflow, multiple bugs in getpwby_r (including incorrect error code returns and incorrect handling of empty files), a critical position-tracking bug in getpwent that skips characters on subsequent lines, and an opportunity to simplify setpwent using rewind.

Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
@github-actions

github-actions Bot commented May 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Unit Test Results

10 911 tests  +51   10 241 ✅ +51   52m 30s ⏱️ - 1m 36s
   649 suites  - 21      670 💤 ± 0 
     1 files   ± 0        0 ❌ ± 0 

Results for commit 9078025. ± Comparison against base commit 7f48b67.

This pull request removes 222 and adds 273 tests. Note that renamed tests count towards both. 
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.getpwd.getpwnam_getnull
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.getpwd.getpwnam_getroot
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.getpwd.getpwnam_nopasswdfile
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.getpwd.getpwuid_getnull
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.getpwd.getpwuid_getroot
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.getpwd.getpwuid_nopasswdfile
phoenix-rtos-tests/libc/misc ‑ armv7a7-imx6ull-evk:phoenix-rtos-tests/libc/misc.getpwd.getpwnam_getnull
phoenix-rtos-tests/libc/misc ‑ armv7a7-imx6ull-evk:phoenix-rtos-tests/libc/misc.getpwd.getpwnam_getroot
phoenix-rtos-tests/libc/misc ‑ armv7a7-imx6ull-evk:phoenix-rtos-tests/libc/misc.getpwd.getpwnam_nopasswdfile
phoenix-rtos-tests/libc/misc ‑ armv7a7-imx6ull-evk:phoenix-rtos-tests/libc/misc.getpwd.getpwuid_getnull
…

phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.pwd.endpwent
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.pwd.getpwent_basic
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.pwd.getpwent_eof
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.pwd.getpwnam_consistent
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.pwd.getpwnam_getnull
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.pwd.getpwnam_getroot
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.pwd.getpwnam_nonexistent
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.pwd.getpwnam_nopasswdfile
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.pwd.getpwnam_r_consistent
phoenix-rtos-tests/libc/misc ‑ aarch64a53-zynqmp-qemu:phoenix-rtos-tests/libc/misc.pwd.getpwnam_r_nonexistent
…

♻️ This comment has been updated with latest results.

@julianuziemblo julianuziemblo force-pushed the julianuziemblo/RTOS-1341 branch from 25b46d7 to c323ff5 Compare May 28, 2026 10:22
@julianuziemblo

julianuziemblo commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

gemini-code-assist[bot]
gemini-code-assist Bot reviewed May 28, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds declarations and implementations for reentrant and stateful password database functions, including getpwuid_r, getpwent, endpwent, and setpwent, alongside refactoring the internal parsing logic in unistd/pwd.c. The review highlights critical issues: a security vulnerability in readid where an empty UID string is parsed as root (0), a bug in getpwby_r where the buffer offset n is not reset when skipping non-matching entries (leading to premature ERANGE errors), and fragility in pwentlast when handling empty or trailing lines.

Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c Outdated
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/RTOS-1341 branch from c323ff5 to 118fd18 Compare May 28, 2026 10:50
@julianuziemblo

julianuziemblo commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

gemini-code-assist[bot]
gemini-code-assist Bot reviewed May 28, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements reentrant password database lookup functions (getpwuid_r, getpwnam_r) and sequential access functions (getpwent, setpwent, endpwent) in unistd/pwd.c, along with their declarations in include/pwd.h. The feedback suggests declaring ret as a signed int in readpwentry to prevent signed/unsigned conversion issues when returning negative error codes, and returning errno instead of a hardcoded EIO when fopen fails in getpwby_r to provide more descriptive error details.

Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/RTOS-1341 branch 2 times, most recently from c4d554c to aa20397 Compare May 28, 2026 11:05
@julianuziemblo julianuziemblo requested a review from a team May 28, 2026 11:06
@julianuziemblo

julianuziemblo commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

gemini-code-assist[bot]
gemini-code-assist Bot reviewed May 28, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements POSIX-compliant password database functions, including the reentrant versions getpwnam_r and getpwuid_r, as well as the stream-based functions getpwent, setpwent, and endpwent. It also refactors the internal parsing logic in unistd/pwd.c to improve safety and error handling. A review comment points out that getpwnam_r does not set *result to NULL when returning EINVAL on a NULL name argument, which violates POSIX requirements.

Comment thread unistd/pwd.c
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/RTOS-1341 branch from aa20397 to 34db99e Compare May 28, 2026 11:44
@julianuziemblo

julianuziemblo commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

gemini-code-assist[bot]
gemini-code-assist Bot reviewed May 28, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements and declares several password database functions in pwd.h and pwd.c, including the reentrant functions getpwuid_r and getpwnam_r, as well as getpwent, endpwent, and setpwent. It also refactors the internal parsing logic to use static variables and helper functions like readpwentryall and pwentlast for safer and cleaner file reading. There are no review comments to address.

rmikielis
rmikielis reviewed Jun 2, 2026
View reviewed changes
Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/RTOS-1341 branch from 34db99e to 101ba65 Compare June 2, 2026 17:08
@julianuziemblo julianuziemblo requested a review from rmikielis June 2, 2026 17:09
github-actions[bot]
github-actions Bot reviewed Jun 2, 2026
View reviewed changes
Comment thread unistd/pwd.c Outdated
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/RTOS-1341 branch from 101ba65 to fffe8d4 Compare June 2, 2026 17:15
rmikielis
rmikielis reviewed Jun 3, 2026
View reviewed changes

@rmikielis rmikielis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread unistd/pwd.c Outdated
ziemleszcz
ziemleszcz reviewed Jun 9, 2026
View reviewed changes
Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c Outdated
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/RTOS-1341 branch from fffe8d4 to b56a26e Compare June 9, 2026 09:42
ziemleszcz
ziemleszcz reviewed Jun 9, 2026
View reviewed changes
Comment thread unistd/pwd.c Outdated
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/RTOS-1341 branch from b56a26e to 94a27bf Compare June 9, 2026 11:33
ziemleszcz
ziemleszcz reviewed Jun 10, 2026
View reviewed changes
Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c Outdated
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/RTOS-1341 branch from 94a27bf to 7ff7c2c Compare June 10, 2026 13:43
@ziemleszcz

ziemleszcz commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

/gemini review

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Jun 10, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements reentrant password database lookup functions (getpwnam_r, getpwuid_r) and standard entry iteration functions (getpwent, setpwent, endpwent) in unistd/pwd.c. While these additions significantly improve POSIX compliance, several critical issues were identified. First, readpwentryline reads directly into the pointers of the static pwnam struct, which can cause undefined behavior if a user program modifies the returned struct. Second, the overflow check in readid is flawed on both 32-bit and 64-bit architectures, failing to correctly handle maximum UID values. Finally, the pointer arithmetic in getpwby_r is overly complex and prone to errors, and the error code handling should be made more consistent.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/RTOS-1341 branch 2 times, most recently from 580e99f to 9f0247f Compare June 10, 2026 15:57
ziemleszcz
ziemleszcz reviewed Jun 11, 2026
View reviewed changes
Comment thread unistd/pwd.c Outdated
Comment thread unistd/pwd.c
@julianuziemblo
pwd: implement missing functions
9078025 
implement: getpwent, setpwent, endpwent, getpwnam_r, getpwuid_r

YT: RTOS-1341
@julianuziemblo julianuziemblo force-pushed the julianuziemblo/RTOS-1341 branch from 9f0247f to 9078025 Compare June 11, 2026 09:31
ziemleszcz
ziemleszcz reviewed Jun 11, 2026
View reviewed changes
Comment thread include/pwd.h
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c
Comment thread unistd/pwd.c

if (name != NULL && strcmp(pwd->pw_name, name) != 0) {
/* skip to next line */
while ((c = fgetc(fp)) != EOF && c != '\n')

@ziemleszcz ziemleszcz Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inconsistent behavior compared to getpwnam(). Here, an invalid line can be skipped (we know only that name is correct) and subsequent entries are still processed, while getpwnam() returns an error on the first invalid line.

@julianuziemblo julianuziemblo Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think we should omit this "optimisation", or add it to getpwnam?

@ziemleszcz ziemleszcz Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please verify how Linux/BSD handles this. If invalid lines are skipped, that’s probably the better behavior, but then getpwnam() should be consistent and skip these lines too.

@julianuziemblo julianuziemblo Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will check and change the implementation accordingly

Comment thread unistd/pwd.c
}
if (uid != NULL && pwd->pw_uid != *uid) {
/* skip to next line */
while ((c = fgetc(fp)) != EOF && c != '\n')

@ziemleszcz ziemleszcz Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ziemleszcz ziemleszcz ziemleszcz left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@rmikielis rmikielis rmikielis left review comments

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@julianuziemblo @ziemleszcz @rmikielis