Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sys/pal/unix/sync/mutex: Fix Mutex::new() on NuttX #138400

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

no1wudi
Copy link
Contributor

@no1wudi no1wudi commented Mar 12, 2025

PTHREAD_MUTEX_INITIALIZER is highly configurable on NuttX, making it difficult to use a hardcoded initializer from libc crate in Rust way.

Instead, call pthread_mutex_init() to initialize the mutex with default attributes, ensuring consistent behavior with PTHREAD_MUTEX_INITIALIZER.

  • Added conditional compilation for NuttX target
  • Replaced PTHREAD_MUTEX_INITIALIZER with pthread_mutex_init() for NuttX
  • Ensured consistent mutex initialization behavior across platforms

PTHREAD_MUTEX_INITIALIZER is highly configurable on NuttX, making it difficult
to use a hardcoded initializer from libc crate in Rust way.

Instead, call pthread_mutex_init() to initialize the mutex with default attributes, ensuring consistent behavior
with PTHREAD_MUTEX_INITIALIZER.

* Added conditional compilation for NuttX target
* Replaced PTHREAD_MUTEX_INITIALIZER with pthread_mutex_init() for NuttX
* Ensured consistent mutex initialization behavior across platforms
@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2025

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 12, 2025
let mut mutex = Mutex { inner: UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER) };

unsafe {
libc::pthread_mutex_init(mutex.raw(), core::ptr::null());
Copy link
Member

Choose a reason for hiding this comment

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

NuttX allows moving a mutex after calling pthread_mutex_init? POSIX makes that UB as the mutex could eg be self-referential.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in this aspect, the situation is somewhat different. When initializing a mutex, NuttX only sets the flags:

int pthread_mutex_init(FAR pthread_mutex_t *mutex,
                       FAR const pthread_mutexattr_t *attr)
{
  int status;

  sinfo("mutex=%p attr=%p\n", mutex, attr);

  if (!mutex)
    {
      return EINVAL;
    }

  /* Initialize the mutex like a semaphore with initial count = 1 */

  status = mutex_init(&mutex->mutex);
  if (status < 0)
    {
      return -status;
    }

  /* Were attributes specified?  If so, use them */

#ifdef CONFIG_PTHREAD_MUTEX_TYPES
      mutex->type  = attr ? attr->type : PTHREAD_MUTEX_DEFAULT;
#endif
#ifndef CONFIG_PTHREAD_MUTEX_UNSAFE
      mutex->flink = NULL;
#  ifdef CONFIG_PTHREAD_MUTEX_BOTH
      mutex->flags = attr && attr->robust == PTHREAD_MUTEX_ROBUST ?
                     _PTHREAD_MFLAGS_ROBUST : 0;
#  else
      mutex->flags = 0;
#  endif
#endif

#if defined(CONFIG_PRIORITY_INHERITANCE) || defined(CONFIG_PRIORITY_PROTECT)
  if (attr)
    {
      status = mutex_set_protocol(&mutex->mutex, attr->proto);
      if (status < 0)
        {
          mutex_destroy(&mutex->mutex);
          return -status;
        }

#  ifdef CONFIG_PRIORITY_PROTECT
      if (attr->proto == PTHREAD_PRIO_PROTECT)
        {
          status = mutex_setprioceiling(&mutex->mutex, attr->ceiling, NULL);
          if (status < 0)
            {
              mutex_destroy(&mutex->mutex);
              return -status;
            }
        }
#  endif
    }
#endif

  return 0;
}

Copy link
Member

Choose a reason for hiding this comment

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

Is this a documented guarantee or does NuttX reserve the right to change the implementation in such a way that this becomes UB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I can't find such a document, I think I can add it to the test cases on the relevant side and incorporate it into the NuttX side to remind developers and prevent accidental changes to this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants