!libphoenix: make pthread attrs (more) POSIX-compliant by adamgreloch · Pull Request #484 · phoenix-rtos/libphoenix

adamgreloch · 2026-06-15T12:48:04Z

TASK: RTOS-1353

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)

Breaking change: adds dependency on new schedInfo syscall.

How Has This Been Tested?

Already covered by automatic testing.
New test added: damianloew/posix_tests
Tested by hand on: riscv64

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).
proc: add sched_info syscall phoenix-rtos-kernel#791
I will merge this PR by myself when appropriate.

TASK: RTOS-1353

gemini-code-assist

Code Review

This pull request adds NULL pointer checks to pthread_attr_init and pthread_attr_destroy to prevent potential dereferences, and fixes a logical bug in pthread_attr_setdetachstate by changing the logical OR (||) to a logical AND (&&) when validating the detachstate parameter. No review comments were provided, so there is no additional feedback.

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.

github-actions · 2026-06-15T13:05:45Z

Unit Test Results

10 860 tests ±0 10 190 ✅ ±0 51m 34s ⏱️ - 1m 24s
670 suites ±0 670 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 126d2be. ± Comparison against base commit fb9670d.

♻️ This comment has been updated with latest results.

adamgreloch · 2026-06-17T11:25:21Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the pthread_attr_t structure, introduces new fields such as guardsize and inheritsched, and implements several missing pthread_attr_* getter and setter functions. However, two critical issues were identified in pthread_create: first, the thread's detached state is incorrectly initialized using the default attributes instead of the user-provided attributes; second, adding the stack size and guard size can result in an integer overflow, potentially leading to an undersized stack allocation and memory corruption.

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.

Fixes: phoenix-rtos/phoenix-rtos-project#1637 TASK: RTOS-1353

The ceiling should be done only when the stack is mmaped. Ceiling in setstacksize violates POSIX and doesn't make sense if the stack is provided by the caller. Fixes: phoenix-rtos/phoenix-rtos-project#1641 TASK: RTOS-1353

TASK: RTOS-1353

ziemleszcz · 2026-06-17T14:39:44Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the pthread attributes implementation, updating the pthread_attr_t structure to support guardsize and inheritsched, renaming fields for clarity, and introducing macro-generated getters. Feedback points out POSIX compliance issues: sched_get_priority_max and sched_get_priority_min should set errno and return -1 on failure rather than returning the error code directly, and pthread_create should ignore guardsize when a custom stackaddr is provided.

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.

gemini-code-assist

Code Review

This pull request refactors the pthread attribute implementation, introducing support for guard size and inherit scheduling attributes, updating the pthread_attr_t structure, and cleaning up function declarations. The feedback highlights POSIX compliance issues: sched_get_priority_max and sched_get_priority_min should return -1 and set errno on failure instead of returning the error code directly, and pthread_attr_setschedparam should return EINVAL rather than ENOTSUP for invalid scheduling priorities.

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.

gemini-code-assist

Code Review

This pull request refactors the pthread implementation by updating the pthread_attr_t structure, introducing support for guard size and scheduling inheritance, and adding corresponding getter and setter functions. The feedback highlights a critical issue where the guard area is not properly protected with PROT_NONE via mprotect, rendering it ineffective against stack overflows. Additionally, the reviewer points out that sched_get_priority_max and sched_get_priority_min should return -1 and set errno to EINVAL on failure to comply with POSIX standards.

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.

adamgreloch · 2026-06-17T16:25:52Z

/gemini review

(but this time please don't repeat yourself 3 times)

gemini-code-assist

Code Review

This pull request refactors the pthread attribute handling and scheduling priority queries. Key changes include updating the pthread_attr_t structure to support guard size and inherit-scheduling attributes, implementing guard page protection using mprotect in pthread_create, and replacing hardcoded scheduling priority limits with dynamic queries via sched_info. Additionally, extern specifiers were cleaned up across header files, and helper macros were introduced to generate attribute getter functions. There are no review comments, so no further feedback is provided.

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.

ziemleszcz · 2026-06-18T05:27:47Z

With the new syscall, this is now a breaking change.

ziemleszcz · 2026-06-18T05:41:49Z

+	ctx->is_detached = (attrs->detachstate == PTHREAD_CREATE_DETACHED) ? 1 : 0;
 	ctx->start_routine = start_routine;
 	ctx->arg = arg;
 	ctx->stack = stack;


What about guard page?

it's already included in the stack. As the guard comes from the same allocation as the stack, this simplifies its management. Added a comment about this in pthread_ctx.

ziemleszcz · 2026-06-18T05:45:06Z

 	}

+	if (guardsize > 0) {
+		if (mprotect(stack, guardsize, PROT_NONE) != 0) {


Did you test how this works in practice?

Yes, after some issues it's finally working. I've been mistakenly creating a thread with stack pointed to stack, while it should be stack + guardsize. This mistake unveiled a bug in the kernel, as it turns out we don't validate the stack passed from userspace at all.

ziemleszcz · 2026-06-22T14:51:47Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the pthread and sched headers and implementations, removing the extern keyword from function declarations, introducing guardsize and inheritsched attributes to pthread_attr_t, and updating scheduling policy checks. Feedback on the changes highlights a potential memory leak in pthread_create when ctx allocation fails due to using the incorrect stack size in munmap, and suggests a more robust check for scheduling policies in pthread_attr_setschedpolicy instead of assuming contiguous policy values.

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.

TASK: RTOS-1353

adamgreloch · 2026-06-23T15:45:31Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the pthread implementation to support additional thread attributes, including guard size, detach state, and scheduling inheritance. It also updates priority-related functions to retrieve limits dynamically via a new schedInfo system call instead of using hardcoded values. The feedback suggests optimizing pthread_attr_setschedparam by calling schedInfo directly once to avoid redundant system calls and prevent unintended side-effects on errno.

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.

TASK: RTOS-1353

…llcheck TASK: RTOS-1353

TASK: RTOS-1353

PS-C-REC-016 TASK: RTOS-1353

POSIX requires either 0 or positive error number. TASK: RTOS-1353

TASK: RTOS-1353

pthread: check if attr is NULL in pthread_attr_{init,destroy}

91fd52f

TASK: RTOS-1353

gemini-code-assist Bot reviewed Jun 15, 2026

View reviewed changes

adamgreloch marked this pull request as ready for review June 17, 2026 11:25

gemini-code-assist Bot reviewed Jun 17, 2026

View reviewed changes

Comment thread pthread/pthread.c

Comment thread pthread/pthread.c Outdated

adamgreloch force-pushed the adamgreloch/RTOS-1353 branch 2 times, most recently from 35b09b7 to 4dea89e Compare June 17, 2026 12:11

adamgreloch requested a review from ziemleszcz June 17, 2026 12:11

adamgreloch changed the title ~~libphoenix: make pthread attrs POSIX-compliant~~ libphoenix: make pthread attrs (more) POSIX-compliant Jun 17, 2026

adamgreloch force-pushed the adamgreloch/RTOS-1353 branch 2 times, most recently from b20dbf9 to 68a70c5 Compare June 17, 2026 12:24

ziemleszcz reviewed Jun 17, 2026

View reviewed changes

adamgreloch changed the title ~~libphoenix: make pthread attrs (more) POSIX-compliant~~ !libphoenix: make pthread attrs (more) POSIX-compliant Jun 17, 2026

adamgreloch changed the title ~~!libphoenix: make pthread attrs (more) POSIX-compliant~~ libphoenix: make pthread attrs (more) POSIX-compliant Jun 17, 2026

adamgreloch added 3 commits June 17, 2026 15:51

pthread: fix always-true detachstate validation condition

37f8e1e

Fixes: phoenix-rtos/phoenix-rtos-project#1637 TASK: RTOS-1353

pthread: don't ceil stacksize in pthread_attr_setstacksize

88024ca

The ceiling should be done only when the stack is mmaped. Ceiling in setstacksize violates POSIX and doesn't make sense if the stack is provided by the caller. Fixes: phoenix-rtos/phoenix-rtos-project#1641 TASK: RTOS-1353

pthread: define pthread_attr getters with macro

525ef03

TASK: RTOS-1353

adamgreloch force-pushed the adamgreloch/RTOS-1353 branch from 68a70c5 to e2a03e2 Compare June 17, 2026 13:54

adamgreloch requested a review from ziemleszcz June 17, 2026 13:54

gemini-code-assist Bot reviewed Jun 17, 2026

View reviewed changes

Comment thread pthread/pthread.c

Comment thread pthread/pthread.c

Comment thread pthread/pthread.c Outdated

gemini-code-assist Bot reviewed Jun 17, 2026

View reviewed changes

Comment thread pthread/pthread.c

Comment thread pthread/pthread.c

Comment thread pthread/pthread.c Outdated

gemini-code-assist Bot reviewed Jun 17, 2026

View reviewed changes

Comment thread pthread/pthread.c Outdated

Comment thread pthread/pthread.c

Comment thread pthread/pthread.c

adamgreloch force-pushed the adamgreloch/RTOS-1353 branch from e2a03e2 to 9011c66 Compare June 17, 2026 16:22

gemini-code-assist Bot reviewed Jun 17, 2026

View reviewed changes

ziemleszcz reviewed Jun 18, 2026

View reviewed changes

adamgreloch force-pushed the adamgreloch/RTOS-1353 branch from 9011c66 to 0cbc7b2 Compare June 22, 2026 13:59

gemini-code-assist Bot reviewed Jun 22, 2026

View reviewed changes

Comment thread pthread/pthread.c Outdated

Comment thread pthread/pthread.c Outdated

adamgreloch force-pushed the adamgreloch/RTOS-1353 branch from 0cbc7b2 to 2158e17 Compare June 22, 2026 14:55

pthread: implement pthread_attr_{get,set}inheritsched

dc2d58d

TASK: RTOS-1353

adamgreloch force-pushed the adamgreloch/RTOS-1353 branch from 2158e17 to 78c62f8 Compare June 23, 2026 15:34

adamgreloch added 3 commits June 23, 2026 17:40

pthread: implement pthread_attr_{get,set}guardsize

ef5ea57

TASK: RTOS-1353

include/sched.h: use sched defines from the kernel header

2741ecb

TASK: RTOS-1353

sched: add schedInfo syscall declaration and use it in priority getters

6ba62b3

TASK: RTOS-1353

adamgreloch force-pushed the adamgreloch/RTOS-1353 branch from 78c62f8 to 3688750 Compare June 23, 2026 15:45

adamgreloch changed the title ~~libphoenix: make pthread attrs (more) POSIX-compliant~~ !libphoenix: make pthread attrs (more) POSIX-compliant Jun 23, 2026

gemini-code-assist Bot reviewed Jun 23, 2026

View reviewed changes

Comment thread pthread/pthread.c Outdated

adamgreloch added 8 commits June 23, 2026 18:03

pthread: return EINVAL on nonsensical priority in attr_setschedparam

f775639

TASK: RTOS-1353

pthread: don't assume SCHED_* ordering in attr_setschedpolicy, add nu…

47386f3

…llcheck TASK: RTOS-1353

pthread: implement pthread_attr_{set,get}scope

dd89867

TASK: RTOS-1353

pthread: use detachstate from user-provided attrs in pthread_create

5159c55

TASK: RTOS-1353

pthread: rename CEIL macro to ALIGN

e2e35c2

TASK: RTOS-1353

pthread: remove extern keyword prior to declarations

a089433

PS-C-REC-016 TASK: RTOS-1353

pthread: return positive errno on pthread_join failure

afc304a

POSIX requires either 0 or positive error number. TASK: RTOS-1353

pthread: don't mmap and use attrs->stackaddr directly if set

126d2be

TASK: RTOS-1353

adamgreloch force-pushed the adamgreloch/RTOS-1353 branch from 3688750 to 126d2be Compare June 23, 2026 16:05

adamgreloch requested a review from ziemleszcz June 23, 2026 16:05

Conversation

adamgreloch commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

github-actions Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

adamgreloch commented Jun 17, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ziemleszcz commented Jun 17, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamgreloch commented Jun 17, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

ziemleszcz commented Jun 18, 2026

Uh oh!

ziemleszcz Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

adamgreloch Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

ziemleszcz Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

adamgreloch Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

ziemleszcz commented Jun 22, 2026

Uh oh!

adamgreloch commented Jun 15, 2026 •

edited

Loading

github-actions Bot commented Jun 15, 2026 •

edited

Loading