Skip to content

pthread: implement rwlocks#505

Open
adamgreloch wants to merge 3 commits into
masterfrom
adamgreloch/RTOS-1389
Open

pthread: implement rwlocks#505
adamgreloch wants to merge 3 commits into
masterfrom
adamgreloch/RTOS-1389

Conversation

@adamgreloch

Copy link
Copy Markdown
Member

TASK: RTOS-1389

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: (add PR link here).
  • Tested by hand on: (list targets here).

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.

@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 read-write locks (pthread_rwlock_t and pthread_rwlockattr_t) and adds the pthread_testcancel cancellation point. The review feedback highlights several critical issues and improvement opportunities: standard POSIX functions should return positive error codes instead of negative ones, and local variable shadowing should be resolved in pthread_rwlock_lazy_init. Additionally, missing NULL checks in pthread_rwlockattr_setpshared, pthread_rwlock_destroy, and timespec_is_valid should be added to prevent potential crashes. The initialized field in pthread_rwlock_t should be declared as volatile int to ensure thread safety, and the return type of timespec_is_valid should be changed from time_t to int.

Comment thread pthread/pthread.c
Comment thread pthread/pthread.c
Comment thread pthread/pthread.c
Comment thread include/sys/types.h
Comment thread pthread/pthread.c Outdated
@adamgreloch
adamgreloch force-pushed the adamgreloch/RTOS-1389 branch 2 times, most recently from 3adc0f2 to b8f7aa0 Compare July 13, 2026 14:38
@adamgreloch
adamgreloch force-pushed the adamgreloch/RTOS-1364 branch from 85344ca to 2ca5da8 Compare July 13, 2026 14:39
@adamgreloch
adamgreloch force-pushed the adamgreloch/RTOS-1389 branch from b8f7aa0 to 972b2ee Compare July 13, 2026 14:51
@adamgreloch
adamgreloch changed the base branch from adamgreloch/RTOS-1364 to master July 20, 2026 13:10
@adamgreloch
adamgreloch force-pushed the adamgreloch/RTOS-1389 branch from 972b2ee to 668d57b Compare July 20, 2026 13:10
@adamgreloch
adamgreloch marked this pull request as ready for review July 20, 2026 13:11
@adamgreloch
adamgreloch requested a review from a team July 20, 2026 13:11
Comment thread include/pthread.h
#define PTHREAD_SCOPE_SYSTEM 1

#define PTHREAD_MUTEX_INITIALIZER { 0, 0 }
#define PTHREAD_RWLOCK_INITIALIZER { 0 }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[clang-format-pr] reported by reviewdog 🐶
suggested fix

Suggested change
#define PTHREAD_RWLOCK_INITIALIZER { 0 }
#define PTHREAD_RWLOCK_INITIALIZER \
{ \
0 \
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

?

@adamgreloch
adamgreloch force-pushed the adamgreloch/RTOS-1389 branch from 668d57b to 905aea8 Compare July 20, 2026 13:13
@adamgreloch

Copy link
Copy Markdown
Member Author

/gemini review

@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 read-write locks (pthread_rwlock_t and pthread_rwlockattr_t) and adds support for thread cancellation via pthread_testcancel. The review feedback highlights two critical issues: a potential segmentation fault in timespec_is_valid due to a missing NULL check on the time pointer, and a concurrency bug in the cancellation logic where cleanup handlers and key data lists can be executed or freed twice, leading to double-free or use-after-free vulnerabilities.

Comment thread pthread/pthread.c
Comment thread pthread/pthread.c
@adamgreloch
adamgreloch marked this pull request as draft July 20, 2026 13:33
@adamgreloch
adamgreloch force-pushed the adamgreloch/RTOS-1389 branch from 905aea8 to 3b81e22 Compare July 20, 2026 14:17
@adamgreloch
adamgreloch force-pushed the adamgreloch/RTOS-1389 branch from 3b81e22 to 3a3e9ee Compare July 20, 2026 14:25
@adamgreloch
adamgreloch marked this pull request as ready for review July 20, 2026 14:25
@github-actions

Copy link
Copy Markdown

Unit Test Results

11 307 tests   10 600 ✅  52m 55s ⏱️
   690 suites     707 💤
     1 files         0 ❌

Results for commit 3a3e9ee.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant