From 2f39c020d4a5bfd228c7868c732f9379ee0ebb40 Mon Sep 17 00:00:00 2001 From: Ezra Chung Date: Mon, 31 Jul 2023 16:27:11 -0500 Subject: [PATCH 1/5] Revert "CDRIVER-4679 Prefer XSI-compliant strerror_r when available (#1350)" This reverts commit ac436db8c1c0ee45a663a469e0783d50713e16e3. --- CMakeLists.txt | 2 +- src/libbson/CMakeLists.txt | 38 +++------------------------ src/libbson/src/bson/bson-config.h.in | 9 ------- src/libbson/src/bson/bson-error.c | 8 +++--- 4 files changed, 7 insertions(+), 50 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 476cc030470..c7219a071d3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -394,7 +394,7 @@ endif () # Enable POSIX features up to POSIX.1-2008 plus the XSI extension and BSD-derived definitions. # Both _BSD_SOURCE and _DEFAULT_SOURCE are defined for backwards-compatibility with glibc 2.19 and earlier. # _BSD_SOURCE and _DEFAULT_SOURCE are required by `getpagesize`, `h_errno`, etc. -# _XOPEN_SOURCE=700 is required by `strnlen`, `strerror_r`, etc. +# _XOPEN_SOURCE=700 is required by `strnlen`, etc. add_definitions (-D_XOPEN_SOURCE=700 -D_BSD_SOURCE -D_DEFAULT_SOURCE) list (APPEND CMAKE_REQUIRED_DEFINITIONS -D_XOPEN_SOURCE=700 -D_BSD_SOURCE -D_DEFAULT_SOURCE) diff --git a/src/libbson/CMakeLists.txt b/src/libbson/CMakeLists.txt index 1cf803ecbcb..49307f2488a 100644 --- a/src/libbson/CMakeLists.txt +++ b/src/libbson/CMakeLists.txt @@ -14,14 +14,12 @@ include (MongoC-Warnings) set (BSON_OUTPUT_BASENAME "bson" CACHE STRING "Output bson library base name") -include (CheckCSourceCompiles) include (CheckFunctionExists) include (CheckIncludeFile) -include (CheckIncludeFiles) include (CheckStructHasMember) include (CheckSymbolExists) -include (InstallRequiredSystemLibraries) include (TestBigEndian) +include (InstallRequiredSystemLibraries) set (CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${PROJECT_SOURCE_DIR}/build/cmake) @@ -85,6 +83,8 @@ else () set (BSON_OS 1) endif () +include (CheckIncludeFiles) + CHECK_INCLUDE_FILE (strings.h BSON_HAVE_STRINGS_H) if (NOT BSON_HAVE_STRINGS_H) set (BSON_HAVE_STRINGS_H 0) @@ -119,38 +119,6 @@ else () set (BSON_BYTE_ORDER 1234) endif () -# 1. Normally, `defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 600` should be enough -# to detect if an XSI-compliant `strerror_r` is available. -# 2. However, GNU provides an incompatible `strerror_r` as an extension, -# requiring an additional test for `!defined(_GNU_SOURCE)`. -# 3. However, musl does not support the GNU extension even when `_GNU_SOURCE` is -# defined, preventing `!defined(_GNU_SOURCE)` from being a portable -# XSI-compliance test. -# 4. However, musl does not provide a feature test macro to detect compilation -# with musl, and workarounds that use internal glibc feature test macros such -# as `defined(__USE_GNU)` are explicitly forbidden by glibc documentation. -# 5. Therefore, give up on using feature test macros: use `CheckCSourceCompiles` -# to test if the current `strerror_r` is XSI-compliant if present. -if (NOT WIN32) - CHECK_C_SOURCE_COMPILES( - [[ - #include - int test(int errnum, char *buf, size_t buflen) { - return strerror_r (errnum, buf, buflen); - } - int main(void) {} - ]] - BSON_HAVE_XSI_STRERROR_R - FAIL_REGEX "int-conversion" - ) -endif () - -if (BSON_HAVE_XSI_STRERROR_R) - set(BSON_HAVE_XSI_STRERROR_R 1) -else() - set(BSON_HAVE_XSI_STRERROR_R 0) -endif () - configure_file ( "${PROJECT_SOURCE_DIR}/src/bson/bson-config.h.in" "${PROJECT_BINARY_DIR}/src/bson/bson-config.h" diff --git a/src/libbson/src/bson/bson-config.h.in b/src/libbson/src/bson/bson-config.h.in index 8c55eadfdb7..2849d0206a2 100644 --- a/src/libbson/src/bson/bson-config.h.in +++ b/src/libbson/src/bson/bson-config.h.in @@ -122,13 +122,4 @@ # undef BSON_HAVE_STRLCPY #endif - -/* - * Define to 1 if you have an XSI-compliant strerror_r on your platform. - */ -#define BSON_HAVE_XSI_STRERROR_R @BSON_HAVE_XSI_STRERROR_R@ -#if BSON_HAVE_XSI_STRERROR_R != 1 -# undef BSON_HAVE_XSI_STRERROR_R -#endif - #endif /* BSON_CONFIG_H */ diff --git a/src/libbson/src/bson/bson-error.c b/src/libbson/src/bson/bson-error.c index 7d2352805d0..d16b5faefcf 100644 --- a/src/libbson/src/bson/bson-error.c +++ b/src/libbson/src/bson/bson-error.c @@ -109,14 +109,12 @@ bson_strerror_r (int err_code, /* IN */ if (strerror_s (buf, buflen, err_code) != 0) { ret = buf; } -#elif defined(BSON_HAVE_XSI_STRERROR_R) +#elif defined(__GNUC__) && defined(_GNU_SOURCE) + ret = strerror_r (err_code, buf, buflen); +#else /* XSI strerror_r */ if (strerror_r (err_code, buf, buflen) == 0) { ret = buf; } -#elif defined(_GNU_SOURCE) - ret = strerror_r (err_code, buf, buflen); -#else - #error "Unable to find a supported strerror_r candidate" #endif if (!ret) { From d53fb91609f7bf7ee5acaac73f8b4ce221b65c7e Mon Sep 17 00:00:00 2001 From: Ezra Chung Date: Mon, 31 Jul 2023 14:37:24 -0500 Subject: [PATCH 2/5] Depend on XSI-compliant strerror_l instead of strerror_r --- CMakeLists.txt | 2 +- src/libbson/src/bson/bson-error.c | 45 +++++++++++++++++++++++++------ 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c7219a071d3..658a5b24c37 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -394,7 +394,7 @@ endif () # Enable POSIX features up to POSIX.1-2008 plus the XSI extension and BSD-derived definitions. # Both _BSD_SOURCE and _DEFAULT_SOURCE are defined for backwards-compatibility with glibc 2.19 and earlier. # _BSD_SOURCE and _DEFAULT_SOURCE are required by `getpagesize`, `h_errno`, etc. -# _XOPEN_SOURCE=700 is required by `strnlen`, etc. +# _XOPEN_SOURCE=700 is required by `strnlen`, `strerror_l`, etc. add_definitions (-D_XOPEN_SOURCE=700 -D_BSD_SOURCE -D_DEFAULT_SOURCE) list (APPEND CMAKE_REQUIRED_DEFINITIONS -D_XOPEN_SOURCE=700 -D_BSD_SOURCE -D_DEFAULT_SOURCE) diff --git a/src/libbson/src/bson/bson-error.c b/src/libbson/src/bson/bson-error.c index d16b5faefcf..6f710f62a63 100644 --- a/src/libbson/src/bson/bson-error.c +++ b/src/libbson/src/bson/bson-error.c @@ -25,6 +25,11 @@ #include "bson-string.h" #include "bson-types.h" +// See `bson_strerror_r()` definition below. +#if !defined(_WIN32) && !defined(__APPLE__) +#include // uselocale() +#endif + /* *-------------------------------------------------------------------------- @@ -98,23 +103,47 @@ bson_set_error (bson_error_t *error, /* OUT */ */ char * -bson_strerror_r (int err_code, /* IN */ - char *buf, /* IN */ - size_t buflen) /* IN */ +bson_strerror_r (int err_code, /* IN */ + char *buf BSON_MAYBE_UNUSED, /* IN */ + size_t buflen BSON_MAYBE_UNUSED) /* IN */ { static const char *unknown_msg = "Unknown error"; char *ret = NULL; #if defined(_WIN32) + // Windows does not provide `strerror_l` or `strerror_r`, but it does + // unconditionally provide `strerror_s`. if (strerror_s (buf, buflen, err_code) != 0) { ret = buf; } -#elif defined(__GNUC__) && defined(_GNU_SOURCE) - ret = strerror_r (err_code, buf, buflen); -#else /* XSI strerror_r */ - if (strerror_r (err_code, buf, buflen) == 0) { - ret = buf; +#elif defined(__APPLE__) + // Apple does not provide `strerror_l`, but it does unconditionally provide + // the XSI-compliant `strerror_r`, but only when compiling with Apple Clang. + // GNU extensions may still be a problem if we are being compiled with GCC on + // Apple. Avoid the compatibility headaches with GNU extensions and the musl + // library by assuming QoI will not cause UB when reading the error message + // string even when `strerror_r` fails. + (void) strerror_r (err_code, buf, buflen); +#elif defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 700 + // Avoid `strerror_r` compatibility headaches with GNU extensions and the + // musl library by using `strerror_l` instead. Furthermore, `strerror_r` is + // scheduled to be marked as obsolete in favor of `strerror_l` in the + // upcoming POSIX Issue 8 (see: + // https://www.austingroupbugs.net/view.php?id=655). + // + // POSIX Spec: since strerror_l() is required to return a string for some + // errors, an application wishing to check for all error situations should + // set errno to 0, then call strerror_l(), then check errno. + errno = 0; + ret = strerror_l (err_code, uselocale ((locale_t) 0)); + if (errno != 0) { + ret = NULL; } +#elif defined(_GNU_SOURCE) + // Unlikely, but continue supporting use of GNU extension in cases where the + // C Driver is being built without _XOPEN_SOURCE=700. + ret = strerror_r (err_code, buf, buflen); +#error "Unable to find a supported strerror_r candidate" #endif if (!ret) { From f9bc8c29265437dce1a1f3621036baf9312d376d Mon Sep 17 00:00:00 2001 From: Ezra Chung Date: Thu, 3 Aug 2023 12:36:53 -0500 Subject: [PATCH 3/5] Fix missing #else to guard #error --- src/libbson/src/bson/bson-error.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libbson/src/bson/bson-error.c b/src/libbson/src/bson/bson-error.c index 6f710f62a63..a8bfae1f549 100644 --- a/src/libbson/src/bson/bson-error.c +++ b/src/libbson/src/bson/bson-error.c @@ -143,6 +143,7 @@ bson_strerror_r (int err_code, /* IN */ // Unlikely, but continue supporting use of GNU extension in cases where the // C Driver is being built without _XOPEN_SOURCE=700. ret = strerror_r (err_code, buf, buflen); +#else #error "Unable to find a supported strerror_r candidate" #endif From 58096c09677003bde2dde8e9e2940935e562d334 Mon Sep 17 00:00:00 2001 From: Ezra Chung Date: Thu, 3 Aug 2023 13:10:11 -0500 Subject: [PATCH 4/5] Ensure a valid locale is passed to strerror_l --- src/libbson/src/bson/bson-error.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/libbson/src/bson/bson-error.c b/src/libbson/src/bson/bson-error.c index a8bfae1f549..6552a945390 100644 --- a/src/libbson/src/bson/bson-error.c +++ b/src/libbson/src/bson/bson-error.c @@ -125,6 +125,19 @@ bson_strerror_r (int err_code, /* IN */ // string even when `strerror_r` fails. (void) strerror_r (err_code, buf, buflen); #elif defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 700 + // The behavior (of `strerror_l`) is undefined if the locale argument to + // `strerror_l()` is the special locale object LC_GLOBAL_LOCALE or is not a + // valid locale object handle. + locale_t locale = uselocale ((locale_t) 0); + // No need to test for error (it can only be [EINVAL]). + if (locale == LC_GLOBAL_LOCALE) { + // Only use our own locale if a thread-local locale was not already set. + // This is just to satisfy `strerror_l`. We do NOT want to unconditionally + // set a thread-local locale. + locale = newlocale (LC_MESSAGES_MASK, "C", (locale_t) 0); + } + BSON_ASSERT (locale != LC_GLOBAL_LOCALE); + // Avoid `strerror_r` compatibility headaches with GNU extensions and the // musl library by using `strerror_l` instead. Furthermore, `strerror_r` is // scheduled to be marked as obsolete in favor of `strerror_l` in the @@ -134,10 +147,18 @@ bson_strerror_r (int err_code, /* IN */ // POSIX Spec: since strerror_l() is required to return a string for some // errors, an application wishing to check for all error situations should // set errno to 0, then call strerror_l(), then check errno. - errno = 0; - ret = strerror_l (err_code, uselocale ((locale_t) 0)); - if (errno != 0) { - ret = NULL; + if (locale != (locale_t) 0) { + errno = 0; + ret = strerror_l (err_code, locale); + + if (errno != 0) { + ret = NULL; + } + + freelocale (locale); + } else { + // Could not obtain a valid `locale_t` object to satisfy `strerror_l`. + // Fallback to `bson_strncpy` below. } #elif defined(_GNU_SOURCE) // Unlikely, but continue supporting use of GNU extension in cases where the From c71941ae526ee9a83cf7f9e03d87bd9f2c1e3bff Mon Sep 17 00:00:00 2001 From: Ezra Chung Date: Thu, 3 Aug 2023 16:31:42 -0500 Subject: [PATCH 5/5] Improve clarity of documentation --- src/libbson/src/bson/bson-error.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libbson/src/bson/bson-error.c b/src/libbson/src/bson/bson-error.c index 6552a945390..c64b87a5301 100644 --- a/src/libbson/src/bson/bson-error.c +++ b/src/libbson/src/bson/bson-error.c @@ -121,8 +121,10 @@ bson_strerror_r (int err_code, /* IN */ // the XSI-compliant `strerror_r`, but only when compiling with Apple Clang. // GNU extensions may still be a problem if we are being compiled with GCC on // Apple. Avoid the compatibility headaches with GNU extensions and the musl - // library by assuming QoI will not cause UB when reading the error message - // string even when `strerror_r` fails. + // library by assuming the implementation will not cause UB when reading the + // error message string even when `strerror_r` fails, as encouraged (but not + // required) by the POSIX spec (see: + // https://pubs.opengroup.org/onlinepubs/9699919799/functions/strerror.html#tag_16_574_08). (void) strerror_r (err_code, buf, buflen); #elif defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 700 // The behavior (of `strerror_l`) is undefined if the locale argument to