DataDog / nginx-datadog

Enhance NGINX Observability and Security with Datadog's Module
https://www.datadoghq.com
Apache License 2.0
25 stars 10 forks source link

Override pthread_cond_init in glibc-compat #105

Closed Anilm3 closed 3 months ago

Anilm3 commented 3 months ago

pthread_cond_init in glibc is provided in two different versions: the "current" version mapped to __pthread_cond_init, and the legacy version mapped to __pthread_cond_init_2_0. When using this function through a musl-built binary, the dynamic linker opts to resolve the non-default legacy version, presumably for backwards compatibility.

However, the legacy API for condition variables forbids the use of clocks other than CLOCK_REALTIME, while in many cases CLOCK_MONOTONIC is preferred instead, resulting in an unexpected failure to initialise a condition variable.

The fix provided overrides pthread_cond_init with a weak symbol and then resolves the default pthread_cond_init provided in libc.so.6. Note that this is only a problem in x86_64.

The images will be updated once this has been merged.

Related Jira:APPSEC-54528

codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 67.64%. Comparing base (98b1d15) to head (73e5b44).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/DataDog/nginx-datadog/pull/105/graphs/tree.svg?width=650&height=150&src=pr&token=SZCZI1FAYU&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog)](https://app.codecov.io/gh/DataDog/nginx-datadog/pull/105?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog) ```diff @@ Coverage Diff @@ ## master #105 +/- ## ======================================= Coverage 67.64% 67.64% ======================================= Files 36 36 Lines 3533 3533 Branches 615 615 ======================================= Hits 2390 2390 Misses 818 818 Partials 325 325 ```
Anilm3 commented 3 months ago

LGTM! :shipit: How did you check if it does behave as intended?

Essentially validating that this works as expected, i.e. error == 0. When using the old symbol, pthread_cond_init results in errno being set to EINVAL (22).

pthread_condattr_init(&attr);
pthread_condattr_setclock(&attr, CLOCK_MONOTONIC);
error = pthread_cond_init(&cond_, &attr);

But this has also been indirectly validated through the PHP appsec CI and system tests, which were failing due to the remote configuration client failing to initialise, specifically due to boost io_context not being able to initialise the condition variable.