Commit 2b1b09ed authored by Phil Willoughby's avatar Phil Willoughby Committed by Facebook GitHub Bot

categorize errno-domain exceptions per platform

Summary:
This change introduces the function `errorCategoryForErrnoDomain` with the contract that it will return the "correct" domain object to use for errno-domain errors when constructing a `std::system_error` object. The current implementation simply forwards to `std::generic_category()` on Windows and `std::system_category()` elsewhere.

## Background
As we understand how the standard intended for this to work:

1. std::generic_category should be used for standard errno domain
2. std::system_category should be used for the system's error code domain

This creates an ambiguity on platforms which define their system error codes as extended errno values, because it's not obvious which category should be used. This applies to macos, Linux, etc.

Historically we decided to use std::system_category for all errno-domain exceptions because we actually can't tell if a given errno value is "standard" or not, and on most platforms there isn't any clear separation of domains.

However, on Windows it turns out this is well-defined, and our previous code is actively incorrect. This is because the system's error code domain is well-defined as "things that `GetLastError()` can return" and that domain is *not* an extension of `errno`. The values intersect with different meanings and in some circumstances the constructor of std::system_error will fail and crash the program if the category is std::system_category and the value comes from the errno-domain.

We chose not to use `std::generic_category()` on non-Windows platforms because the reasoning that led us to use `std::system_category()` historically still makes sense - that domain still contains non-standard-errno values on those platforms.

Reviewed By: simpkins

Differential Revision: D28327673

fbshipit-source-id: d1a3a55b384e818eb4122d5cd03031bc7de90876
parent 58d7ddb2
...@@ -35,15 +35,22 @@ namespace folly { ...@@ -35,15 +35,22 @@ namespace folly {
// //
// The *Explicit functions take an explicit value for errno. // The *Explicit functions take an explicit value for errno.
// On linux and similar platforms the value of `errno` is a mixture of
// POSIX-`errno`-domain error codes and per-OS extended error codes. So the
// most appropriate category to use is `system_category`.
//
// On Windows `system_category` means codes that can be returned by Win32 API
// `GetLastError` and codes from the `errno`-domain must be reported as
// `generic_category`.
inline const std::error_category& errorCategoryForErrnoDomain() noexcept {
if (kIsWindows) {
return std::generic_category();
}
return std::system_category();
}
inline std::system_error makeSystemErrorExplicit(int err, const char* msg) { inline std::system_error makeSystemErrorExplicit(int err, const char* msg) {
// TODO: The C++ standard indicates that std::generic_category() should be return std::system_error(err, errorCategoryForErrnoDomain(), msg);
// used for POSIX errno codes.
//
// We should ideally change this to use std::generic_category() instead of
// std::system_category(). However, undertaking this change will require
// updating existing call sites that currently catch exceptions thrown by
// this code and currently expect std::system_category.
return std::system_error(err, std::system_category(), msg);
} }
template <class... Args> template <class... Args>
......
...@@ -21,9 +21,9 @@ ...@@ -21,9 +21,9 @@
#include <cstdint> #include <cstdint>
#include <system_error> #include <system_error>
#include <folly/Exception.h>
#include <folly/portability/SysMman.h> #include <folly/portability/SysMman.h>
#include <folly/portability/Unistd.h> #include <folly/portability/Unistd.h>
namespace folly { namespace folly {
namespace detail { namespace detail {
...@@ -55,7 +55,7 @@ class MMapAlloc { ...@@ -55,7 +55,7 @@ class MMapAlloc {
-1, -1,
0)); 0));
if (mem == reinterpret_cast<void*>(-1)) { if (mem == reinterpret_cast<void*>(-1)) {
throw std::system_error(errno, std::system_category()); throw std::system_error(errno, errorCategoryForErrnoDomain());
} }
#if !defined(MAP_POPULATE) && defined(MADV_WILLNEED) #if !defined(MAP_POPULATE) && defined(MADV_WILLNEED)
madvise(mem, size, MADV_WILLNEED); madvise(mem, size, MADV_WILLNEED);
......
...@@ -20,6 +20,7 @@ ...@@ -20,6 +20,7 @@
#include <system_error> #include <system_error>
#include <folly/Exception.h>
#include <folly/gen/String.h> #include <folly/gen/String.h>
namespace folly { namespace folly {
...@@ -41,7 +42,8 @@ class FileReader : public GenImpl<ByteRange, FileReader> { ...@@ -41,7 +42,8 @@ class FileReader : public GenImpl<ByteRange, FileReader> {
n = ::read(file_.fd(), buffer_->writableTail(), buffer_->capacity()); n = ::read(file_.fd(), buffer_->writableTail(), buffer_->capacity());
} while (n == -1 && errno == EINTR); } while (n == -1 && errno == EINTR);
if (n == -1) { if (n == -1) {
throw std::system_error(errno, std::system_category(), "read failed"); throw std::system_error(
errno, errorCategoryForErrnoDomain(), "read failed");
} }
if (n == 0) { if (n == 0) {
return true; return true;
...@@ -101,7 +103,7 @@ class FileWriter : public Operator<FileWriter> { ...@@ -101,7 +103,7 @@ class FileWriter : public Operator<FileWriter> {
} while (n == -1 && errno == EINTR); } while (n == -1 && errno == EINTR);
if (n == -1) { if (n == -1) {
throw std::system_error( throw std::system_error(
errno, std::system_category(), "write() failed"); errno, errorCategoryForErrnoDomain(), "write() failed");
} }
v.advance(size_t(n)); v.advance(size_t(n));
} }
......
...@@ -25,6 +25,7 @@ ...@@ -25,6 +25,7 @@
#include <boost/preprocessor/control/if.hpp> #include <boost/preprocessor/control/if.hpp>
#include <folly/Exception.h>
#include <folly/ExceptionWrapper.h> #include <folly/ExceptionWrapper.h>
#include <folly/Format.h> #include <folly/Format.h>
#include <folly/Portability.h> #include <folly/Portability.h>
...@@ -2020,7 +2021,8 @@ void AsyncSocket::cacheAddresses() { ...@@ -2020,7 +2021,8 @@ void AsyncSocket::cacheAddresses() {
cacheLocalAddress(); cacheLocalAddress();
cachePeerAddress(); cachePeerAddress();
} catch (const std::system_error& e) { } catch (const std::system_error& e) {
if (e.code() != std::error_code(ENOTCONN, std::system_category())) { if (e.code() !=
std::error_code(ENOTCONN, errorCategoryForErrnoDomain())) {
VLOG(2) << "Error caching addresses: " << e.code().value() << ", " VLOG(2) << "Error caching addresses: " << e.code().value() << ", "
<< e.code().message(); << e.code().message();
} }
......
...@@ -25,15 +25,15 @@ ...@@ -25,15 +25,15 @@
namespace folly { namespace folly {
namespace test { namespace test {
#define EXPECT_SYSTEM_ERROR(statement, err, msg) \ #define EXPECT_SYSTEM_ERROR(statement, err, msg) \
try { \ try { \
statement; \ statement; \
ADD_FAILURE() << "Didn't throw"; \ ADD_FAILURE() << "Didn't throw"; \
} catch (const std::system_error& e) { \ } catch (const std::system_error& e) { \
std::system_error expected(err, std::system_category(), msg); \ std::system_error expected(err, errorCategoryForErrnoDomain(), msg); \
EXPECT_STREQ(expected.what(), e.what()); \ EXPECT_STREQ(expected.what(), e.what()); \
} catch (...) { \ } catch (...) { \
ADD_FAILURE() << "Threw a different type"; \ ADD_FAILURE() << "Threw a different type"; \
} }
TEST(ExceptionTest, Simple) { TEST(ExceptionTest, Simple) {
...@@ -94,27 +94,27 @@ TEST(ExceptionTest, makeSystemError) { ...@@ -94,27 +94,27 @@ TEST(ExceptionTest, makeSystemError) {
errno = ENOENT; errno = ENOENT;
auto ex = makeSystemErrorExplicit(EDEADLK, "stuck"); auto ex = makeSystemErrorExplicit(EDEADLK, "stuck");
EXPECT_EQ(EDEADLK, ex.code().value()); EXPECT_EQ(EDEADLK, ex.code().value());
EXPECT_EQ(std::system_category(), ex.code().category()); EXPECT_EQ(errorCategoryForErrnoDomain(), ex.code().category());
EXPECT_TRUE(StringPiece{ex.what()}.contains("stuck")) EXPECT_TRUE(StringPiece{ex.what()}.contains("stuck"))
<< "what() string missing input message: " << ex.what(); << "what() string missing input message: " << ex.what();
ex = makeSystemErrorExplicit(EDOM, 300, " is bigger than max=", 255); ex = makeSystemErrorExplicit(EDOM, 300, " is bigger than max=", 255);
EXPECT_EQ(EDOM, ex.code().value()); EXPECT_EQ(EDOM, ex.code().value());
EXPECT_EQ(std::system_category(), ex.code().category()); EXPECT_EQ(errorCategoryForErrnoDomain(), ex.code().category());
EXPECT_TRUE(StringPiece{ex.what()}.contains("300 is bigger than max=255")) EXPECT_TRUE(StringPiece{ex.what()}.contains("300 is bigger than max=255"))
<< "what() string missing input message: " << ex.what(); << "what() string missing input message: " << ex.what();
errno = EINVAL; errno = EINVAL;
ex = makeSystemError("bad argument ", 1234, ": bogus"); ex = makeSystemError("bad argument ", 1234, ": bogus");
EXPECT_EQ(EINVAL, ex.code().value()); EXPECT_EQ(EINVAL, ex.code().value());
EXPECT_EQ(std::system_category(), ex.code().category()); EXPECT_EQ(errorCategoryForErrnoDomain(), ex.code().category());
EXPECT_TRUE(StringPiece{ex.what()}.contains("bad argument 1234: bogus")) EXPECT_TRUE(StringPiece{ex.what()}.contains("bad argument 1234: bogus"))
<< "what() string missing input message: " << ex.what(); << "what() string missing input message: " << ex.what();
errno = 0; errno = 0;
ex = makeSystemError("unexpected success"); ex = makeSystemError("unexpected success");
EXPECT_EQ(0, ex.code().value()); EXPECT_EQ(0, ex.code().value());
EXPECT_EQ(std::system_category(), ex.code().category()); EXPECT_EQ(errorCategoryForErrnoDomain(), ex.code().category());
EXPECT_TRUE(StringPiece{ex.what()}.contains("unexpected success")) EXPECT_TRUE(StringPiece{ex.what()}.contains("unexpected success"))
<< "what() string missing input message: " << ex.what(); << "what() string missing input message: " << ex.what();
} }
......
...@@ -217,11 +217,8 @@ CheckResult checkThrowErrno( ...@@ -217,11 +217,8 @@ CheckResult checkThrowErrno(
try { try {
fn(); fn();
} catch (const std::system_error& ex) { } catch (const std::system_error& ex) {
// TODO: POSIX errno values should use std::generic_category(), but // POSIX errno can use either std::generic_category() or
// folly/Exception.h incorrectly throws them using std::system_category() // std::system_category() on POSIX platforms.
// at the moment.
// For now we also accept std::system_category so that we will also handle
// exceptions from folly/Exception.h correctly.
if (ex.code().category() != std::generic_category() && if (ex.code().category() != std::generic_category() &&
ex.code().category() != std::system_category()) { ex.code().category() != std::system_category()) {
return CheckResult(false) return CheckResult(false)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment