Skip to content

CDRIVER-4679 Revert BSON_HAVE_XSI_STRERROR_R and use strerror_l instead #1370

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 3, 2023
Merged

CDRIVER-4679 Revert BSON_HAVE_XSI_STRERROR_R and use strerror_l instead #1370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f39c02
Revert "CDRIVER-4679 Prefer XSI-compliant strerror_r when available (…
eramongodb Jul 31, 2023
d53fb91
Depend on XSI-compliant strerror_l instead of strerror_r
eramongodb Jul 31, 2023
f9bc8c2
Fix missing #else to guard #error
eramongodb Aug 3, 2023
58096c0
Ensure a valid locale is passed to strerror_l
eramongodb Aug 3, 2023
c71941a
Improve clarity of documentation
eramongodb Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`, `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)

Expand Down
38 changes: 3 additions & 35 deletions src/libbson/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 <string.h>
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"
Expand Down
9 changes: 0 additions & 9 deletions src/libbson/src/bson/bson-config.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
65 changes: 58 additions & 7 deletions src/libbson/src/bson/bson-error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@
#include "bson-string.h"
#include "bson-types.h"

// See `bson_strerror_r()` definition below.
#if !defined(_WIN32) && !defined(__APPLE__)
#include <locale.h> // uselocale()
#endif


/*
*--------------------------------------------------------------------------
Expand Down Expand Up @@ -98,25 +103,71 @@ 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(BSON_HAVE_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 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);
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider using this code path for the final #else case instead of raising a build error of "Unable to find a supported strerror_r candidate"?

I'll note that PHP core does that (see: php/php-src#11624 (comment)). IMO it's not the ideal approach (I disagree with PHP doing it for all cases, since they never bother to detect the XSI variant), but it generally works. And given that errno translation probably isn't a critical component of libbson, I think a user would be more inclined to receive "Unknown error" at runtime than a build error.

Looking at uses of bson_strerror_r() throughout libbson and libmongoc, I see that the err_code param is only sometimes included in the logged error message. That seems like something that could be addressed independently. Rather than hunt through all of those occurrences, a compromise might be to incorporate it into the returned string for !ret at the end of this function (e.g. "Unknown error (errno: %d)").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we consider using this code path for the final #else case instead of raising a build error of "Unable to find a supported strerror_r candidate"?

I elected to have the #else -> #error branch for more informative error messages should any of the expectations accounted for by the prior conditions are not met (even if that shouldn't be the case). The explicit error message would be more informative than a compilation error should these expectations ever be broken (i.e. future maintenance changing the build configuration).

And given that errno translation probably isn't a critical component of libbson

The concern isn't with the criticality of bson_strerror_r usage, but the possibility of undefined behavior when the caller reads the error message string when strerror_r fails and strerror_r does not write to the buffer on failure (implementations are encouraged, but not required to, write to the buffer on failure).

For example, bson_json_reader_new_from_file's errmsg_buf buffer is uninitialized. If strerror_r fails internally and does not write a failure message to the buffer, the successful result of bson_strerror_r would point to the uninitialized buffer as the error message string, and any subsequent reads of that error message string would be undefined behavior.

The reason for including the note about "assuming QoI will not cause UB" is to document this potential scenario.

Copy link
Member

Choose a reason for hiding this comment

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

For example, bson_json_reader_new_from_file's errmsg_buf buffer is uninitialized. If strerror_r fails internally and does not write a failure message to the buffer, the successful result of bson_strerror_r would point to the uninitialized buffer as the error message string, and any subsequent reads of that error message string would be undefined behavior.

Ah, very good point. I was confused by jumping back and forth between libbson and PHP, which allocates the buffer in https://github.com/php/php-src/blob/3d832da8f53d2c2f93d4ffbd1c807d9a5d66536b/Zend/zend.c#L1631-L1641 (where the worst case scenario there is just an empty string).

SGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where the worst case scenario there is just an empty string

FYI the linked function is stil susceptible to the same potential undefined behavior as described for bson_json_reader_new_from_file: the buf array is allocated but its elements are left uninitialized. Subsequent read of the buf elements in zend_error_noreturn may be undefined behavior if strerror_r failed and did not write a message to buf. The risk could be reduced by initializing the elements of the buf array, but it would still be a risk nevertheless (reliance on QoI to always write something to buf even on failure).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right you are again. The buf is declared within the function and unallocated. I was thinking of a static/global decl being allocated with zeroes.

#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
// 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.
if (locale != (locale_t) 0) {
errno = 0;
ret = strerror_l (err_code, locale);

if (errno != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I had a fleeting thought of "what if we wanted to translate errno here? We can't possibly use strerror_l()!"

:laugh-cry:

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
// 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"
#error "Unable to find a supported strerror_r candidate"
#endif

if (!ret) {
Expand Down