From a9c3630d1bb2760546470845c34fcc1dc04bb561 Mon Sep 17 00:00:00 2001 From: dominicpoeschko <45942148+dominicpoeschko@users.noreply.github.com> Date: Fri, 7 Feb 2020 17:59:11 +0100 Subject: [PATCH 1/2] Properly handling SPDLOG_PREVENT_CHILD_FD Using the SPDLOG_PREVENT_CHILD_FD option there where still a race when a other thread was using fork and exec in between the call to fopen and fcntl. Using open and O_CLOEXEC when possible prevents this race. I have no idea if this problem exists on Windows. --- include/spdlog/details/os-inl.h | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/include/spdlog/details/os-inl.h b/include/spdlog/details/os-inl.h index 8473eb09..b306e318 100644 --- a/include/spdlog/details/os-inl.h +++ b/include/spdlog/details/os-inl.h @@ -153,10 +153,26 @@ SPDLOG_INLINE bool fopen_s(FILE **fp, const filename_t &filename, const filename *fp = ::_fsopen((filename.c_str()), mode.c_str(), _SH_DENYNO); #endif #else // unix +#if defined(SPDLOG_PREVENT_CHILD_FD) && (defined(_POSIX_VERSION) && _POSIX_VERSION >= 200809L) + // prevent child processes from inheriting log file descriptors direcly on open + // so there is no race with a possible fork and exec + const int mode_flag = mode == SPDLOG_FILENAME_T("ab") ? O_APPEND : O_TRUNC; + const int fd = ::open((filename.c_str()), O_CREAT | O_WRONLY | O_CLOEXEC | mode_flag, mode_t(0644)); + if(fd == -1) + { + return false; + } + *fp = ::fdopen(fd, mode.c_str()); + if (*fp == nullptr) + { + ::close(fd); + } +#else *fp = ::fopen((filename.c_str()), mode.c_str()); #endif +#endif -#ifdef SPDLOG_PREVENT_CHILD_FD +#if defined(SPDLOG_PREVENT_CHILD_FD) && !(defined(_POSIX_VERSION) && _POSIX_VERSION >= 200809L) // prevent child processes from inheriting log file descriptors if (*fp != nullptr) { From 033fe9f133092dd8d13cdd52a882a7c291a4a48d Mon Sep 17 00:00:00 2001 From: dominic Date: Sat, 8 Feb 2020 12:15:05 +0100 Subject: [PATCH 2/2] Properly handling SPDLOG_PREVENT_CHILD_FD Removed check for posix version so that missing O_CLOEXEC leads to compiler error. Removed extra function since it hat no real purpose anymore. Error behavior between Windows and Unix now equivalent. --- include/spdlog/details/os-inl.h | 39 ++++++++++----------------------- include/spdlog/details/os.h | 4 ---- 2 files changed, 12 insertions(+), 31 deletions(-) diff --git a/include/spdlog/details/os-inl.h b/include/spdlog/details/os-inl.h index b306e318..ccc53ff2 100644 --- a/include/spdlog/details/os-inl.h +++ b/include/spdlog/details/os-inl.h @@ -126,23 +126,6 @@ SPDLOG_INLINE std::tm gmtime() SPDLOG_NOEXCEPT return gmtime(now_t); } -#ifdef SPDLOG_PREVENT_CHILD_FD -SPDLOG_INLINE void prevent_child_fd(FILE *f) -{ -#ifdef _WIN32 - auto file_handle = reinterpret_cast(_get_osfhandle(::_fileno(f))); - if (!::SetHandleInformation(file_handle, HANDLE_FLAG_INHERIT, 0)) - SPDLOG_THROW(spdlog_ex("SetHandleInformation failed", errno)); -#else - auto fd = ::fileno(f); - if (::fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) - { - SPDLOG_THROW(spdlog_ex("fcntl with FD_CLOEXEC failed", errno)); - } -#endif -} -#endif // SPDLOG_PREVENT_CHILD_FD - // fopen_s on non windows for writing SPDLOG_INLINE bool fopen_s(FILE **fp, const filename_t &filename, const filename_t &mode) { @@ -152,10 +135,19 @@ SPDLOG_INLINE bool fopen_s(FILE **fp, const filename_t &filename, const filename #else *fp = ::_fsopen((filename.c_str()), mode.c_str(), _SH_DENYNO); #endif +#if defined(SPDLOG_PREVENT_CHILD_FD) + if (*fp != nullptr) + { + auto file_handle = reinterpret_cast(_get_osfhandle(::_fileno(*fp))); + if (!::SetHandleInformation(file_handle, HANDLE_FLAG_INHERIT, 0)) + { + :fclose(*fp); + *fp = nullptr; + } + } +#endif #else // unix -#if defined(SPDLOG_PREVENT_CHILD_FD) && (defined(_POSIX_VERSION) && _POSIX_VERSION >= 200809L) - // prevent child processes from inheriting log file descriptors direcly on open - // so there is no race with a possible fork and exec +#if defined(SPDLOG_PREVENT_CHILD_FD) const int mode_flag = mode == SPDLOG_FILENAME_T("ab") ? O_APPEND : O_TRUNC; const int fd = ::open((filename.c_str()), O_CREAT | O_WRONLY | O_CLOEXEC | mode_flag, mode_t(0644)); if(fd == -1) @@ -172,13 +164,6 @@ SPDLOG_INLINE bool fopen_s(FILE **fp, const filename_t &filename, const filename #endif #endif -#if defined(SPDLOG_PREVENT_CHILD_FD) && !(defined(_POSIX_VERSION) && _POSIX_VERSION >= 200809L) - // prevent child processes from inheriting log file descriptors - if (*fp != nullptr) - { - prevent_child_fd(*fp); - } -#endif return *fp == nullptr; } diff --git a/include/spdlog/details/os.h b/include/spdlog/details/os.h index 0894a6c0..8a038b26 100644 --- a/include/spdlog/details/os.h +++ b/include/spdlog/details/os.h @@ -38,10 +38,6 @@ static const char folder_sep = '\\'; SPDLOG_CONSTEXPR static const char folder_sep = '/'; #endif -#ifdef SPDLOG_PREVENT_CHILD_FD -void prevent_child_fd(FILE *f); -#endif - // fopen_s on non windows for writing bool fopen_s(FILE **fp, const filename_t &filename, const filename_t &mode);