Add BIO_FP_TEXT

This CL allows us to reduce the patch set on CPython.

BIO_new_fp is the FILE* analog of BIO_new_fd. However, it behaves very
strangely w.r.t. Windows file translation modes. Instead of simply
inheriting the FILE* as the caller constructed it, it unconditionally
overrides the file's translation mode!

This is surprising. Moreover, if you change the mode without flushing
the file, weird things happen, as Windows documentation discusses:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/setmode?view=msvc-170

This leaks all the way up to calling code, because callers need to pass
a matching BIO_FP_TEXT to the FILE* they made. To be source-compatible
with such callers, notably CPython, we need to at least provide
BIO_FP_TEXT.

I first tried to fully match OpenSSL's semantics, but OpenSSL's
semantics are quite dangerous. Code tested on POSIX, calling
BIO_new_fp(some_file, BIO_NOCLOSE), without much thought, is subtly
broken on Windows. It will change the mode of any file passed into it to
binary!

Our own code runs into this. BIO_new_file internally calls BIO_new_fp.
In OpenSSL, they need to re-parse the mode string and figure out the
right flag. ASN1_STRING_print_ex_fp doesn't even know which is the right
one. In OpenSSL, they actually call fwrite manually. We wrap it in a BIO
and then use the BIO version, because it makes no sense to not use the
abstraction we already have lying around. But that is incompatible with
OpenSSL's semantics.

So instead I've opted to make BIO_FP_TEXT switch the mode, but no flag
just leaves the mode alone. This is slightly OpenSSL-incompatible
because this code will work in OpenSSL, but continue to not work in
BoringSSL:

  // Oops, I actually wanted binary but forgot to use "rb"
  FILE *f = fopen("blah", "r");
  // But bio fixed it for me!
  BIO *bio = BIO_new_fp(f, BIO_NOCLOSE);

But callers should have passed "rb" if they wanted binary. This is also
preexisting and no one has noticed. I think it's far more likely that
applications *aren't* expecting BIO_new_fp to secretly change the input
FILE's mode. If we ever need to, we can adopt OpenSSL's semantics and
then add BIO_FP_LEAVE_MY_FILE_ALONE. But those are worse defaults.

Change-Id: I2905673c523eb24312c15d3000cbe34a66602700
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66809
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Auto-Submit: David Benjamin <[email protected]>

crypto/bio/bio_test.cc

@@ -50,13 +50,17 @@ 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() {
   char buf[DECIMAL_SIZE(int) + 1];
   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 {
@@ -673,21 +677,16 @@ TEST(BIOTest, Gets) {
 
       {
         SCOPED_TRACE("fd");
-#if defined(OPENSSL_WINDOWS)
-        int open_flags = _O_RDONLY | _O_BINARY;
-#else
-        int open_flags = O_RDONLY;
-#endif
 
         // Test |BIO_NOCLOSE|.
-        ScopedFD fd = file.OpenFD(open_flags);
+        ScopedFD fd = file.OpenFD(kOpenReadOnlyBinary);
         ASSERT_TRUE(fd.is_valid());
         bssl::UniquePtr<BIO> bio(BIO_new_fd(fd.get(), BIO_NOCLOSE));
         ASSERT_TRUE(bio);
         check_bio_gets(bio.get());
 
         // Test |BIO_CLOSE|.
-        fd = file.OpenFD(open_flags);
+        fd = file.OpenFD(kOpenReadOnlyBinary);
         ASSERT_TRUE(fd.is_valid());
         bio.reset(BIO_new_fd(fd.get(), BIO_CLOSE));
         ASSERT_TRUE(bio);
@@ -706,22 +705,87 @@ TEST(BIOTest, Gets) {
   EXPECT_EQ(c, 'a');
 }
 
-// Test that, on Windows, |BIO_read_filename| opens files in binary mode.
-TEST(BIOTest, BinaryMode) {
+// Test that, on Windows, file BIOs correctly handle text vs binary mode.
+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<char> buf(str.size() + 100);
+    int len = BIO_read(bio, buf.data(), static_cast<int>(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(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| should use the specified mode.
+  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);
+  expect_text_mode(bio.get());
+
+  // |BIO_new_fp| inherits the file's existing mode by default.
+  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);
+  expect_text_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.

crypto/bio/file.c

@@ -73,6 +73,7 @@
 
 #include <openssl/bio.h>
 
+#include <assert.h>
 #include <errno.h>
 #include <stdio.h>
 #include <string.h>
@@ -82,6 +83,10 @@
 
 #include "../internal.h"
 
+#if defined(OPENSSL_WINDOWS)
+#include <fcntl.h>
+#include <io.h>
+#endif
 
 #define BIO_FP_READ 0x02
 #define BIO_FP_WRITE 0x04
@@ -122,14 +127,13 @@ BIO *BIO_new_file(const char *filename, const char *mode) {
   return ret;
 }
 
-BIO *BIO_new_fp(FILE *stream, int close_flag) {
+BIO *BIO_new_fp(FILE *stream, int flags) {
   BIO *ret = BIO_new(BIO_s_file());
-
   if (ret == NULL) {
     return NULL;
   }
 
-  BIO_set_fp(ret, stream, close_flag);
+  BIO_set_fp(ret, stream, flags);
   return ret;
 }
 
@@ -196,6 +200,17 @@ static long file_ctrl(BIO *b, int cmd, long num, void *ptr) {
       break;
     case BIO_C_SET_FILE_PTR:
       file_free(b);
+      static_assert((BIO_CLOSE & BIO_FP_TEXT) == 0,
+                    "BIO_CLOSE and BIO_FP_TEXT must not collide");
+#if defined(OPENSSL_WINDOWS)
+      // If |BIO_FP_TEXT| is not set, OpenSSL will switch the file to binary
+      // mode. BoringSSL intentionally diverges here because it means code
+      // tested under POSIX will inadvertently change the state of |FILE|
+      // objects when wrapping them in a |BIO|.
+      if (num & BIO_FP_TEXT) {
+        _setmode(_fileno(ptr), _O_TEXT);
+      }
+#endif
       b->shutdown = (int)num & BIO_CLOSE;
       b->ptr = ptr;
       b->init = 1;
@@ -287,8 +302,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) {

include/openssl/bio.h

@@ -473,22 +473,59 @@ OPENSSL_EXPORT int BIO_get_fd(BIO *bio, int *out_fd);
 OPENSSL_EXPORT const BIO_METHOD *BIO_s_file(void);
 
 // BIO_new_file creates a file BIO by opening |filename| with the given mode.
-// See the |fopen| manual page for details of the mode argument.
+// See the |fopen| manual page for details of the mode argument. On Windows,
+// files may be opened in either binary or text mode so, as in |fopen|, callers
+// must specify the desired option in |mode|.
 OPENSSL_EXPORT BIO *BIO_new_file(const char *filename, const char *mode);
 
-// BIO_new_fp creates a new file BIO that wraps the given |FILE|. If
-// |close_flag| is |BIO_CLOSE|, then |fclose| will be called on |stream| when
-// the BIO is closed.
-OPENSSL_EXPORT BIO *BIO_new_fp(FILE *stream, int close_flag);
+// BIO_FP_TEXT indicates the |FILE| should be switched to text mode on Windows.
+// It has no effect on non-Windows platforms.
+#define BIO_FP_TEXT 0x10
+
+// BIO_new_fp creates a new file BIO that wraps |file|. If |flags| contains
+// |BIO_CLOSE|, then |fclose| will be called on |file| when the BIO is closed.
+//
+// On Windows, if |flags| contains |BIO_FP_TEXT|, this function will
+// additionally switch |file| to text mode. This is not recommended, but may be
+// required for OpenSSL compatibility. If |file| was not already in text mode,
+// mode changes can cause unflushed data in |file| to be written in unexpected
+// ways. See |_setmode| in Windows documentation for details.
+//
+// Unlike OpenSSL, if |flags| does not contain |BIO_FP_TEXT|, the translation
+// mode of |file| is left as-is. In OpenSSL, |file| will be set to binary, with
+// the same pitfalls as above. BoringSSL does not do this so that wrapping a
+// |FILE| in a |BIO| will not inadvertently change its state.
+//
+// To avoid these pitfalls, callers should set the desired translation mode when
+// opening the file. If targeting just BoringSSL, this is sufficient. If
+// targeting both OpenSSL and BoringSSL, callers should set |BIO_FP_TEXT| to
+// match the desired state of the file.
+OPENSSL_EXPORT BIO *BIO_new_fp(FILE *file, int flags);
 
 // BIO_get_fp sets |*out_file| to the current |FILE| for |bio|. It returns one
 // on success and zero otherwise.
 OPENSSL_EXPORT int BIO_get_fp(BIO *bio, FILE **out_file);
 
-// BIO_set_fp sets the |FILE| for |bio|. If |close_flag| is |BIO_CLOSE| then
+// BIO_set_fp sets the |FILE| for |bio|. If |flags| contains |BIO_CLOSE| then
 // |fclose| will be called on |file| when |bio| is closed. It returns one on
 // success and zero otherwise.
-OPENSSL_EXPORT int BIO_set_fp(BIO *bio, FILE *file, int close_flag);
+//
+// On Windows, if |flags| contains |BIO_FP_TEXT|, this function will
+// additionally switch |file| to text mode. This is not recommended, but may be
+// required for OpenSSL compatibility. If |file| was not already in text mode,
+// mode changes can cause unflushed data in |file| to be written in unexpected
+// ways. See |_setmode| in Windows documentation for details.
+//
+// Unlike OpenSSL, if |flags| does not contain |BIO_FP_TEXT|, the translation
+// mode of |file| is left as-is. In OpenSSL, |file| will be set to binary, with
+// the same pitfalls as above. BoringSSL does not do this so that wrapping a
+// |FILE| in a |BIO| will not inadvertently change its state.
+//
+// To avoid these pitfalls, callers should set the desired translation mode when
+// opening the file. If targeting just BoringSSL, this is sufficient. If
+// targeting both OpenSSL and BoringSSL, callers should set |BIO_FP_TEXT| to
+// match the desired state of the file.
+OPENSSL_EXPORT int BIO_set_fp(BIO *bio, FILE *file, int flags);
 
 // BIO_read_filename opens |filename| for reading and sets the result as the
 // |FILE| for |bio|. It returns one on success and zero otherwise. The |FILE|