Skip to content

Commit

Permalink
Bring in testing changes from upstream commit 5ee4e95 (#2048)
Browse files Browse the repository at this point in the history
Upstream commit [`5ee4e95`][1]'s source changes are almost identical to
our
addition of `BIO_FP_TEXT` in [PR 1153][2]. The only difference in
behavior is
that we will call `_setmode` w/ `_O_BINARY` on windows when no flags are
specified, where BoringSSL will simply no-op and leave the underlying
file's
mode alone.

Upstream's commit has some nice tests, though, so we crib those and
change a
single line of expectation to account for our difference in behavior.

[1]:
google/boringssl@5ee4e95
[2]: #1153
[3]:
https://github.com/openssl/openssl/blob/dc10ffc2834e0d2f5ebc1c3e29bd97f1f43a0ead/crypto/bio/bss_file.c#L235-L237
  • Loading branch information
WillChilds-Klein authored Dec 13, 2024
1 parent 02ea4c4 commit 637b5d2
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 11 deletions.
86 changes: 78 additions & 8 deletions crypto/bio/bio_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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<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| 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.
Expand Down
10 changes: 7 additions & 3 deletions crypto/bio/file.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 637b5d2

Please sign in to comment.