Pthread implementation of condition variables and mutex uses system_clock which is non monotonic (IDFGH-11725)

[matheo-lucak]

Is your feature request related to a problem?

The pthread implementation of condition variables and mutexes uses only std::chrono::system_clock which is non monotonic.

CPP reference recommends the use of std::chrono::steady_clock instead here :

The standard recommends that a steady clock be used to measure the duration. This function may block for longer than timeout_duration due to scheduling or resource contention delays.

Same thing for mutexes here

The standard recommends that a std::steady_clock is used to measure the duration. If an implementation uses a std::system_clock instead, the wait time may also be sensitive to clock adjustments.

Having condition variables based on a non monotonic clock will affect waiting conditions if the clock comes to be updated.

We can find the use of the system clock for condition variable here

/// In <condition_variable> header

  /// condition_variable
  class condition_variable
  {
    using steady_clock = chrono::steady_clock;
    using system_clock = chrono::system_clock;
#ifdef _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT  /// Not defined
    using __clock_t = steady_clock;
#else
    using __clock_t = system_clock;         /// system clock is used here
#endif

and here

/// pthread/pthread_cond_var.c

int pthread_cond_timedwait(pthread_cond_t *cv, pthread_mutex_t *mut, const struct timespec *to)
{
    int ret;
    TickType_t timeout_ticks;

    int res = s_check_and_init_if_static(cv);
    if (res) {
        return res;
    }

    esp_pthread_cond_t *cond = (esp_pthread_cond_t *) *cv;

    if (to == NULL) {
        timeout_ticks = portMAX_DELAY;
    } else {
        struct timeval abs_time, cur_time, diff_time;
        long timeout_msec;
        gettimeofday(&cur_time, NULL);  /// gettimeofday uses the system clock
        ...

And for timed mutexes here

/// In <mutex> header

  template<typename _Derived>
    class __timed_mutex_impl
    {
    protected:
      template<typename _Rep, typename _Period>
    bool
    _M_try_lock_for(const chrono::duration<_Rep, _Period>& __rtime)
    {
#if _GLIBCXX_USE_PTHREAD_MUTEX_CLOCKLOCK          /// Not defined
      using __clock = chrono::steady_clock;
#else
      using __clock = chrono::system_clock;   /// System clock is used here
#endif

      auto __rt = chrono::duration_cast<__clock::duration>(__rtime);
      if (ratio_greater<__clock::period, _Period>())
        ++__rt;
      return _M_try_lock_until(__clock::now() + __rt);
    }

And also here

/// pthread/pthread.c

int IRAM_ATTR pthread_mutex_timedlock(pthread_mutex_t *mutex, const struct timespec *timeout)
{
    if (!mutex) {
        return EINVAL;
    }
    int res = pthread_mutex_init_if_static(mutex);
    if (res != 0) {
        return res;
    }

    struct timespec currtime;
    clock_gettime(CLOCK_REALTIME, &currtime);   /// CLOCK_REALTIME refers to the system clock
    TickType_t tmo = ((timeout->tv_sec - currtime.tv_sec)*1000 +
                     (timeout->tv_nsec - currtime.tv_nsec)/1000000)/portTICK_PERIOD_MS;

    res = pthread_mutex_lock_internal((esp_pthread_mutex_t *)*mutex, tmo);
    if (res == EBUSY) {
        return ETIMEDOUT;
    }
    return res;
}

I'm not an security expert, but knowing that condition variable and mutexes are based on system clock, and system clock is often affected by NTP updates on many devices. DNS hijacking and Man-In-The-Middle attacks on NTP could be a very easy attack vector.

Describe the solution you'd like.

Pthread should be configurable to use either the std::chrono::system_clock or the monotonic std::chrono::steady_clock using the menuconfig tool.

Describe alternatives you've considered.

No response

Additional context.

I'm using :

• esp-idf v5.0.1
• c++ 20

[SoucheSouche]

Hi @matheo-lucak, Thank you for reporting this. We will take a look at it and come back to you.

[MaximeVinzio]

Hello @ESP-Marius, we would be interested by this feature too as it is a blocker for our project where we expect periodic clock synchronization with SNTP and use the pthread and mutex functionalities.

Was this issue already prioritized? we consider it as well quite critical.

[0xjakob]

HI @MaximeVinzio, @matheo-lucak!

The C++ standard library used in ESP-IDF is the upstream library from GCC (libstdc++). As you already pointed out, it will call pthread_cond_timedwait() and pthread_mutex_timedlock() under the hood.

To std::chrono:steady_clock within the timed waiting functions, the GNU C++ standard library requires a set of non-standard functions (i.e., they are not in the POSIX standard). Note that they are only available on fairly recent versions of glibc, not in newlib, which ESP-IDF is using, neither on many other operating systems. However, the usefulness of having MONOTONIC clock available for synchronization primitives is acknowledged. Since we are already planning a refactoring of our POSIX portability layer, we might think of adding this feature in the future, but we are not certain yet. We'll keep you updated.

[0xjakob]

We've discussed this in our team and concluded that adding the API from Glibc is useful. Even though the current restriction to CLOCK_REALTIME is according to the POSIX standard, we plan to add this in the medium-term future.