Windows: Made `net::download()` use CRT file descriptor explicitly. This is an edge case where a third-party library (libcurl) requires a CRT integer file descriptor. Thus we explicitly allocate one via `crt()`, which requires that we also manually close it via `_close()`, not `os::close()`.
Review: https://reviews.apache.org/r/66433 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b061fb14 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b061fb14 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b061fb14 Branch: refs/heads/master Commit: b061fb14638f306491d16cc79442ca5bc46bbde6 Parents: d0b055b Author: Andrew Schwartzmeyer <[email protected]> Authored: Tue Apr 3 22:33:39 2018 -0700 Committer: Andrew Schwartzmeyer <[email protected]> Committed: Tue May 1 18:36:04 2018 -0700 ---------------------------------------------------------------------- 3rdparty/stout/include/stout/net.hpp | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/b061fb14/3rdparty/stout/include/stout/net.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/stout/include/stout/net.hpp b/3rdparty/stout/include/stout/net.hpp index 52fa09b..10666ed 100644 --- a/3rdparty/stout/include/stout/net.hpp +++ b/3rdparty/stout/include/stout/net.hpp @@ -167,19 +167,32 @@ inline Try<int> download( curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, nullptr); curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, true); - // We don't bother introducing a `os::fdopen` since this is the only place - // we use `fdopen` in the entire codebase as of writing this comment. + // We don't bother introducing a `os::fdopen()` since this is the + // only place we use `fdopen()` in the entire codebase as of writing + // this comment. #ifdef __WINDOWS__ + // This explicitly allocates a CRT integer file descriptor, which + // when closed, also closes the underlying handle, so we do not call + // `CloseHandle()` (or `os::close()`). + const int crt = fd->crt(); // We open in "binary" mode on Windows to avoid line-ending translation. - FILE* file = ::_fdopen(fd->crt(), "wb"); + FILE* file = ::_fdopen(crt, "wb"); + if (file == nullptr) { + curl_easy_cleanup(curl); + // NOTE: This is not `os::close()` because we allocated a CRT int + // fd earlier. + ::_close(crt); + return ErrnoError("Failed to open file handle of '" + path + "'"); + } #else FILE* file = ::fdopen(fd.get(), "w"); -#endif if (file == nullptr) { curl_easy_cleanup(curl); os::close(fd.get()); return ErrnoError("Failed to open file handle of '" + path + "'"); } +#endif // __WINDOWS__ + curl_easy_setopt(curl, CURLOPT_WRITEDATA, file); if (stall_timeout.isSome()) { @@ -195,7 +208,9 @@ inline Try<int> download( CURLcode curlErrorCode = curl_easy_perform(curl); if (curlErrorCode != 0) { curl_easy_cleanup(curl); - fclose(file); + // NOTE: `fclose()` also closes the associated file descriptor, so + // we do not call `close()`. + ::fclose(file); return Error(curl_easy_strerror(curlErrorCode)); } @@ -203,7 +218,7 @@ inline Try<int> download( curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &code); curl_easy_cleanup(curl); - if (fclose(file) != 0) { + if (::fclose(file) != 0) { return ErrnoError("Failed to close file handle of '" + path + "'"); }
