diff --git a/crypto/bio/bio_test.cc b/crypto/bio/bio_test.cc index 9328b4ead9..0c9f904e30 100644 --- a/crypto/bio/bio_test.cc +++ b/crypto/bio/bio_test.cc @@ -50,6 +50,8 @@ using Socket = int; #define INVALID_SOCKET (-1) static int closesocket(int sock) { return close(sock); } static std::string LastSocketError() { return strerror(errno); } +static const int kOpenReadOnlyBinary = O_RDONLY; +static const int kOpenReadOnlyText = O_RDONLY; #else using Socket = SOCKET; static std::string LastSocketError() { @@ -57,6 +59,8 @@ static std::string LastSocketError() { snprintf(buf, sizeof(buf), "%d", WSAGetLastError()); return buf; } +static const int kOpenReadOnlyBinary = _O_RDONLY | _O_BINARY; +static const int kOpenReadOnlyText = O_RDONLY | _O_TEXT; #endif class OwnedSocket { @@ -832,21 +836,87 @@ TEST(BIOTest, ExternalData) { } // Test that, on Windows, |BIO_read_filename| opens files in binary mode. -TEST(BIOTest, BinaryMode) { +TEST(BIOTest, FileMode) { if (SkipTempFileTests()) { GTEST_SKIP(); } - TemporaryFile file; - ASSERT_TRUE(file.Init("\r\n")); + TemporaryFile temp; + ASSERT_TRUE(temp.Init("hello\r\nworld")); - // Reading from the file should give back the exact bytes we put in. + auto expect_file_contents = [](BIO *bio, const std::string &str) { + // Read more than expected, to make sure we've reached the end of the file. + std::vector buf(str.size() + 100); + int len = BIO_read(bio, buf.data(), static_cast(buf.size())); + ASSERT_GT(len, 0); + EXPECT_EQ(Bytes(buf.data(), len), Bytes(str)); + }; + auto expect_binary_mode = [&](BIO *bio) { + expect_file_contents(bio, "hello\r\nworld"); + }; + auto expect_text_mode = [&](BIO *bio) { +#if defined(OPENSSL_WINDOWS) + expect_file_contents(bio, "hello\nworld"); +#else + expect_file_contents(bio, "hello\r\nworld"); +#endif + }; + + // |BIO_read_filename| should open in binary mode. bssl::UniquePtr bio(BIO_new(BIO_s_file())); ASSERT_TRUE(bio); - ASSERT_TRUE(BIO_read_filename(bio.get(), file.path().c_str())); - char buf[2]; - ASSERT_EQ(2, BIO_read(bio.get(), buf, 2)); - EXPECT_EQ(Bytes(buf, 2), Bytes("\r\n")); + ASSERT_TRUE(BIO_read_filename(bio.get(), temp.path().c_str())); + expect_binary_mode(bio.get()); + // |BIO_new_file| does not set |BIO_FP_TEXT|, so expect it to set the file's + // mode to binary in all cases. This odd behavior, but it's what OpenSSL does. + bio.reset(BIO_new_file(temp.path().c_str(), "rb")); + ASSERT_TRUE(bio); + expect_binary_mode(bio.get()); + bio.reset(BIO_new_file(temp.path().c_str(), "r")); + ASSERT_TRUE(bio); + // NOTE: Our behavior here aligns with OpenSSL which is to |_setmode| the file + // to binary. BoringSSL would |expect_text_mode| below because it respects + // default mode on Windows which is text and doesn't call |_setmode| (unless + // |BIO_FP_TEXT| is set, which is not the case here). + expect_binary_mode(bio.get()); + // |BIO_new_fp| will always set |_O_BINARY| if |BIO_FP_TEXT| is not set in the + // call to |BIO_new_fp|. + ScopedFILE file = temp.Open("rb"); + ASSERT_TRUE(file); + bio.reset(BIO_new_fp(file.get(), BIO_NOCLOSE)); + ASSERT_TRUE(bio); + expect_binary_mode(bio.get()); + file = temp.Open("r"); + ASSERT_TRUE(file); + bio.reset(BIO_new_fp(file.get(), BIO_NOCLOSE)); + ASSERT_TRUE(bio); + // NOTE: Our behavior here aligns with OpenSSL. BoringSSL would + // |expect_text_mode| below because they don't call |_setmode| unless + // |BIO_FP_TEXT| is set. + expect_binary_mode(bio.get()); + // However, |BIO_FP_TEXT| changes the file to be text mode, no matter how it + // was opened. + file = temp.Open("rb"); + ASSERT_TRUE(file); + bio.reset(BIO_new_fp(file.get(), BIO_NOCLOSE | BIO_FP_TEXT)); + ASSERT_TRUE(bio); + expect_text_mode(bio.get()); + file = temp.Open("r"); + ASSERT_TRUE(file); + bio.reset(BIO_new_fp(file.get(), BIO_NOCLOSE | BIO_FP_TEXT)); + ASSERT_TRUE(bio); + expect_text_mode(bio.get()); + // |BIO_new_fd| inherits the FD's existing mode. + ScopedFD fd = temp.OpenFD(kOpenReadOnlyBinary); + ASSERT_TRUE(fd.is_valid()); + bio.reset(BIO_new_fd(fd.get(), BIO_NOCLOSE)); + ASSERT_TRUE(bio); + expect_binary_mode(bio.get()); + fd = temp.OpenFD(kOpenReadOnlyText); + ASSERT_TRUE(fd.is_valid()); + bio.reset(BIO_new_fd(fd.get(), BIO_NOCLOSE)); + ASSERT_TRUE(bio); + expect_text_mode(bio.get()); } // Run through the tests twice, swapping |bio1| and |bio2|, for symmetry. diff --git a/crypto/bio/file.c b/crypto/bio/file.c index 91bdf1b81f..9a520f2a5e 100644 --- a/crypto/bio/file.c +++ b/crypto/bio/file.c @@ -206,9 +206,13 @@ static long file_ctrl(BIO *b, int cmd, long num, void *ptr) { b->init = 1; #if defined(OPENSSL_WINDOWS) // Windows differentiates between "text" and "binary" file modes, so set - // the file to text mode if caller specifies BIO_FP_TEXT flag. + // the file to text mode if caller specifies BIO_FP_TEXT flag. If + // |BIO_FP_TEXT| is not set, we still call |_setmode| with |_O_BINARY| to + // match OpenSSL's behavior. BoringSSL, on the other hand, does nothing in + // this case. // // https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/setmode?view=msvc-170#remarks + // https://github.com/google/boringssl/commit/5ee4e9512e9a99f97c4a3fad397034028b3457c2 _setmode(_fileno(b->ptr), num & BIO_FP_TEXT ? _O_TEXT : _O_BINARY); #endif break; @@ -299,8 +303,8 @@ int BIO_get_fp(BIO *bio, FILE **out_file) { return (int)BIO_ctrl(bio, BIO_C_GET_FILE_PTR, 0, (char *)out_file); } -int BIO_set_fp(BIO *bio, FILE *file, int close_flag) { - return (int)BIO_ctrl(bio, BIO_C_SET_FILE_PTR, close_flag, (char *)file); +int BIO_set_fp(BIO *bio, FILE *file, int flags) { + return (int)BIO_ctrl(bio, BIO_C_SET_FILE_PTR, flags, (char *)file); } int BIO_read_filename(BIO *bio, const char *filename) {