CDRIVER-4679 Prefer XSI-compliant strerror_r when available

[eramongodb]

Attempts to resolve CDRIVER-4679 by preferring the XSI-compliant version of strerror_r when available by using a more accurate feature test macros condition. The GNU extension is used as a fallback instead. An additional #error was added to hopefully improve the resulting compilation error if a supported strerror_r implementation cannot be found.

Verified by this patch. Note: not yet verified on Alpine Linux as described in CDRIVER-4679 as the corresponding EVG distro is not available, so not entirely confident this actually addresses the originally reported issue.

[jmikola]

https://stackoverflow.com/a/65745713/162228 was a helpful reminder that undefined macros evaluate to zero.

What is the argument for not using the logic suggested in strerror_r(3)?

(_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && ! _GNU_SOURCE

I'm now familiar with what platforms define _POSIX_C_SOURCE and/or _XOPEN_SOURCE, but it would seem more flexible to me to allow either. Likewise, ! _GNU_SOURCE would also handle the case where _GNU_SOURCE is defined but zero (not sure how relevant that is).

---

Also, I'm not sure if this line from the man page is relevant:

If no feature test macros are explicitly defined, then (since glibc 2.4) _POSIX_SOURCE is defined by default with the value 200112L, so that the XSI-compliant version of strerror_r() is provided by default.

I don't see _POSIX_SOURCE referred to anywhere within libmongoc. It's mentioned once in the bundled zlib sources.

The versions of glibc mentioned in the man page predate 2011, so perhaps none of this is relevant to any supported platform.

[eramongodb]

What is the argument for not using the logic suggested in strerror_r(3)?

_XOPEN_SOURCE implies _POSIX_C_SOURCE per https://pubs.opengroup.org/onlinepubs/7908799/xsh/compilation.html: "Since this specification is aligned with the ISO C standard, and since all functionality enabled by _POSIX_C_SOURCE set greater than zero and less than or equal to 199506L should be enabled by _XOPEN_SOURCE set equal to 500, there should be no need to define either _POSIX_SOURCE or _POSIX_C_SOURCE if _XOPEN_SOURCE is so defined."

Likewise, !_GNU_SOURCE would also handle the case where _GNU_SOURCE is defined but zero (not sure how relevant that is).

Only the presence of _GNU_SOURCE matters, not its value, per https://man7.org/linux/man-pages/man7/feature_test_macros.7.html: "Defining this macro (with any value) implicitly defines [...]. In addition, various GNU-specific extensions are also exposed."

I don't see _POSIX_SOURCE referred to anywhere within libmongoc.

It is unconditionally defined via the definition of _XOPEN_SOURCE here:

mongo-c-driver/CMakeLists.txt

Line 398 in 6d2be25

add_definitions (-D_XOPEN_SOURCE=700 -D_BSD_SOURCE -D_DEFAULT_SOURCE)

All our requirements concerning POSIX and extensions are being handled by the (unconditional) definition of the macros listed in the link above. The conflict between the XSI-compliant version and GNU extension version of strerror_r is AFAIK a unique case (among the features being used by the C Driver).

[jmikola]

Thanks for the rigorous response! LGTM.

[kevinAlbs]

musl defines the XSI-conforming strerror_r when _GNU_SOURCE is defined.

Compiling with Alpine with docker results in an error:

docker run --rm -it --platform linux/amd64 -v `pwd`:/c-driver alpine

Configured with _GNU_SOURCE defined:

cmake \
    -DCMAKE_C_FLAGS="-D_GNU_SOURCE=1" \
    -DBUILD_VERSION="1.25.0-pre" \
    -DENABLE_SSL=OFF \
    -DENABLE_SRV=OFF \
    -S$(pwd) -B$(pwd)/cmake-build

Building selects the GNU extension:

/c-driver/src/libbson/src/bson/bson-error.c: In function 'bson_strerror_r':
/c-driver/src/libbson/src/bson/bson-error.c:120:8: error: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
  120 |    ret = strerror_r (err_code, buf, buflen);
      |        ^
[ 40%] Building C object src/libbson/CMakeFiles/bson_shared.dir/src/bson/bson-iter.c.o

Does this suggest a bug in musl? Is musl be expected to provide the GNU extension when _GNU_SOURCE is defined?

[eramongodb]

Does this suggest a bug in musl? Is musl be expected to provide the GNU extension when _GNU_SOURCE is defined?

This seems to be a deliberate choice by musl, not a bug. Support for _GNU_SOURCE (in a meaningful way) seems to be a hard "no", per this mailing thread:

I have found that _GNU_SOURCE is defined when I build snort on OpenWrt (OpenWrt uses musl). This causes the build to fail because snort expects strerror_r to return a char * since _GNU_SOURCE is defined.

Is this a bug in musl?

No, musl explicitly does not [support] the GNU interfaces that conflict with standard interfaces by the same name.

This issue seems to have also plagued Gentoo maintainers as documented here:

Should we blame musl for this? [...] Unfortunately the ugliness doesn’t just end here. It turns out g++/clang++ unconditionally predefines _GNU_SOURCE when compiling any C++ code on Linux, and that’s because libc++/libstd++ won’t work on Linux without this macro defined. So the perfect fix for this incompatibility between LLVM and musl should be fixing the C++ compiler itself, but that’s another big story…

and here:

This week I was trying to make libc++ work without _GNU_SOURCE predefined, which causes me some trouble when compiling LLVM against musl. As mentioned in my last post, g++/clang++ unconditionally predefines _GNU_SOURCE for any C++ code, because libstdc++/libc++ simply won’t work without it. This is an old and well-known issue [1], but unfortunately has never been fixed. This week I boldly tried to fix it for libc++, and failed 🙁

and here:

musl is very strict regarding standards-conformance compared to the widely used GNU C Library (glibc). This means that many of the GNU extensions, as well as much of the backwards compatibility provided by glibc is completely removed, and applications that use these will often fail to build. Here are some pointers on getting software to compile with musl.

In summary, combining musl and _GNU_SOURCE (with the expectation of _GNU_SOURCE enabling corresponding GNU extensions) seems like a forbidden recipe for trouble. 😕

We could carve out an exclusion to handle the musl case (despite no feature test macro to detect musl directly), where the XSI-compliant variant is present despite _GNU_SOURCE being set, which is normally not the case.

[jmikola]

@kevinAlbs: Thanks for catching that. That very issue was discussed in https://stackoverflow.com/q/41953104/162228, which I linked from CDRIVER-4679 and soon after forgot. Unfortunately, the Stack Overflow question was using C++ and ultimately worked around this using operator overloading.

[kevinAlbs]

We could carve out an exclusion to handle the musl case (despite no feature test macro to detect musl directly), where the XSI-compliant variant is present despite _GNU_SOURCE being set, which is normally not the case.

I vote for carving out an exclusion to handle musl. Given repeated requests for Alpine, I expect Alpine may eventually be a tested platform in the future (filed CDRIVER-4696 to consider testing with Alpine in Evergreen).

[eramongodb]

Given our findings above, which are now documented in the CMakeLists.txt file, gave up on using feature test macros and instead opted to use CMake's CheckCSourceCompiles module to obtain the definitive truth. Verified by this patch.

[jmikola]

gave up on using feature test macros and instead opted to use CMake's CheckCSourceCompiles module to obtain the definitive truth.

This does seem like the most reliable solution. Since CMake doesn't apply to the PHP driver, I created PHPC-2263 to port this to Autotools.

[kevinAlbs]

Confirmed latest changes build with Alpine in docker.