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)

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).
include/threads: expose min,max prio values to userspace 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 52m 23s ⏱️ -35s
670 suites ±0 670 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit e2a03e2. ± 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

PS-C-REC-016 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 · 2026-06-17T14:42:16Z

 int sched_get_priority_max(int policy)
 {
 	if (policy == SCHED_RR)
-		return 7;
+		return PH_MAX_PRIO;

 	return EINVAL;
 }


According to POSIX, sched_get_priority_max should return -1 and set errno to EINVAL if the policy is invalid or unsupported, rather than returning EINVAL directly as a positive integer.

int sched_get_priority_max(int policy) { if (policy == SCHED_RR) { return PH_MAX_PRIO; } errno = EINVAL; return -1; }

gemini-code-assist · 2026-06-17T14:42:16Z

 int sched_get_priority_min(int policy)
 {
 	if (policy == SCHED_RR)
-		return 0;
+		return PH_MIN_PRIO;

 	return EINVAL;
 }


According to POSIX, sched_get_priority_min should return -1 and set errno to EINVAL if the policy is invalid or unsupported, rather than returning EINVAL directly as a positive integer.

int sched_get_priority_min(int policy) { if (policy == SCHED_RR) { return PH_MIN_PRIO; } errno = EINVAL; return -1; }

gemini-code-assist · 2026-06-17T14:42:16Z

+	if (attrs->stacksize > SIZE_MAX - attrs->guardsize || attrs->stacksize + attrs->guardsize > SIZE_MAX - PAGE_SIZE) {
+		return EINVAL;
+	}
+
+	size_t stacksize = ALIGN(attrs->stacksize + attrs->guardsize, PAGE_SIZE);
+	/* FIXME: don't mmap when attrs->stackaddr is not null and just use that address as a stack */
+	void *stack = mmap(attrs->stackaddr, stacksize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);


According to POSIX, if the stackaddr attribute has been set (i.e., the caller is allocating and managing their own thread stacks), the guardsize attribute must be ignored and no guard area should be created. Currently, guardsize is added to stacksize regardless of whether stackaddr is NULL or not.

size_t guardsize = (attrs->stackaddr != NULL) ? 0 : attrs->guardsize; if (attrs->stacksize > SIZE_MAX - guardsize || attrs->stacksize + guardsize > SIZE_MAX - PAGE_SIZE) { return EINVAL; } size_t stacksize = ALIGN(attrs->stacksize + guardsize, PAGE_SIZE); /* FIXME: don't mmap when attrs->stackaddr is not null and just use that address as a stack */ void *stack = mmap(attrs->stackaddr, stacksize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

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 · 2026-06-17T14:42:56Z

 int sched_get_priority_max(int policy)
 {
 	if (policy == SCHED_RR)
-		return 7;
+		return PH_MAX_PRIO;

 	return EINVAL;
 }


According to POSIX, sched_get_priority_max should return -1 and set errno to EINVAL on failure, rather than returning the error code directly.

int sched_get_priority_max(int policy) { if (policy == SCHED_RR) { return PH_MAX_PRIO; } errno = EINVAL; return -1; }

gemini-code-assist · 2026-06-17T14:42:56Z

 int sched_get_priority_min(int policy)
 {
 	if (policy == SCHED_RR)
-		return 0;
+		return PH_MIN_PRIO;

 	return EINVAL;
 }


According to POSIX, sched_get_priority_min should return -1 and set errno to EINVAL on failure, rather than returning the error code directly.

int sched_get_priority_min(int policy) { if (policy == SCHED_RR) { return PH_MIN_PRIO; } errno = EINVAL; return -1; }

gemini-code-assist · 2026-06-17T14:42:56Z

+	if (param->sched_priority > sched_get_priority_max(attr->schedpolicy) ||
+			param->sched_priority < sched_get_priority_min(attr->schedpolicy)) {
+		return ENOTSUP;
+	}


According to POSIX, pthread_attr_setschedparam should return EINVAL (not ENOTSUP) if the scheduling priority specified in param is invalid for the scheduling policy of attr.

if (param->sched_priority > sched_get_priority_max(attr->schedpolicy) || param->sched_priority < sched_get_priority_min(attr->schedpolicy)) { return EINVAL; }

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.

gemini-code-assist · 2026-06-17T14:44:01Z

+	if (attrs->stacksize > SIZE_MAX - attrs->guardsize || attrs->stacksize + attrs->guardsize > SIZE_MAX - PAGE_SIZE) {
+		return EINVAL;
+	}
+
+	size_t stacksize = ALIGN(attrs->stacksize + attrs->guardsize, PAGE_SIZE);
+	/* FIXME: don't mmap when attrs->stackaddr is not null and just use that address as a stack */
+	void *stack = mmap(attrs->stackaddr, stacksize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

 	if ((stack == MAP_FAILED) || (stack == NULL)) {
 		return EAGAIN;


The guard area is currently allocated as part of the stack with PROT_READ | PROT_WRITE permissions, which means it does not actually protect against stack overflow (it just acts as extra stack space). To comply with POSIX and provide actual stack overflow protection, the guard area should be protected with PROT_NONE using mprotect.

if (attrs->guardsize > SIZE_MAX - PAGE_SIZE || attrs->stacksize > SIZE_MAX - attrs->guardsize) { return EINVAL; } size_t guardsize = ALIGN(attrs->guardsize, PAGE_SIZE); if (attrs->stacksize > SIZE_MAX - guardsize || attrs->stacksize + guardsize > SIZE_MAX - PAGE_SIZE) { return EINVAL; } size_t stacksize = ALIGN(attrs->stacksize + guardsize, PAGE_SIZE); /* FIXME: don't mmap when attrs->stackaddr is not null and just use that address as a stack */ void *stack = mmap(attrs->stackaddr, stacksize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); if ((stack == MAP_FAILED) || (stack == NULL)) { return EAGAIN; } if (attrs->guardsize > 0) { if (mprotect(stack, guardsize, PROT_NONE) != 0) { munmap(stack, stacksize); return EAGAIN; } }

gemini-code-assist · 2026-06-17T14:44:01Z

 int sched_get_priority_max(int policy)
 {
 	if (policy == SCHED_RR)
-		return 7;
+		return PH_MAX_PRIO;

 	return EINVAL;
 }


According to POSIX, sched_get_priority_max should return -1 and set errno to EINVAL if the policy is invalid, rather than returning EINVAL directly.

int sched_get_priority_max(int policy) { if (policy == SCHED_RR) { return PH_MAX_PRIO; } errno = EINVAL; return -1; }

gemini-code-assist · 2026-06-17T14:44:01Z

 int sched_get_priority_min(int policy)
 {
 	if (policy == SCHED_RR)
-		return 0;
+		return PH_MIN_PRIO;

 	return EINVAL;
 }


According to POSIX, sched_get_priority_min should return -1 and set errno to EINVAL if the policy is invalid, rather than returning EINVAL directly.

int sched_get_priority_min(int policy) { if (policy == SCHED_RR) { return PH_MIN_PRIO; } errno = EINVAL; return -1; }

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 11 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

pthread: implement pthread_attr_{get,set}inheritsched

96942a6

TASK: RTOS-1353

pthread: implement pthread_attr_{get,set}guardsize

f0c2867

TASK: RTOS-1353

pthread: use PH_{MIN,MAX}_PRIO define in sched_get_priority_{min,max}

33cc575

TASK: RTOS-1353

pthread: disallow other schedpolicy than SCHED_RR in setschedparam

0f07f79

TASK: RTOS-1353

pthread: implement pthread_attr_{set,get}scope

81605e1

TASK: RTOS-1353

pthread: use detachstate from user-provided attrs in pthread_create

7e582a9

TASK: RTOS-1353

pthread: rename CEIL macro to ALIGN

eb5e454

TASK: RTOS-1353

pthread: remove extern keyword prior to declarations

e2a03e2

PS-C-REC-016 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

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!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

adamgreloch commented Jun 15, 2026 •

edited

Loading

github-actions Bot commented Jun 15, 2026 •

edited

Loading