Don't consume the newline in BIO_gets for fds

We support BIO_gets on three BIOs. They're all slightly different. File
BIOs have the NUL truncation bug. fd BIOs swallow the embedded newline.
This CL fixes the second issue and updates the BIO_gets test to cover
all three.

See also upstream's https://github.com/openssl/openssl/pull/3442

Update-Note: BIO_gets on an fd BIO now returns the newline, to align
with memory and file BIOs. BIO_gets is primarily used in the PEM parser,
which tries to tolerate both cases, but this reduces the risk of some
weird bug that only appears in fd BIOs.

Change-Id: Ia8ffb8c67b6981d6ef7144a1522f8605dc01d525
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58552
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

2 files changed, 61 insertions, 21 deletions

diff --git a/crypto/bio/bio_test.cc b/crypto/bio/bio_test.cc
index 28b9c6d..99cdcbf 100644
--- a/crypto/bio/bio_test.cc
+++ b/crypto/bio/bio_test.cc
@@ -364,7 +364,7 @@ TEST(BIOTest, MemWritable) {
   EXPECT_EQ(BIO_eof(bio.get()), 1);
 }
 
-TEST(BIOTest, MemGets) {
+TEST(BIOTest, Gets) {
   const struct {
     std::string bio;
     int gets_len;
@@ -386,8 +386,6 @@ TEST(BIOTest, MemGets) {
       {"12345", 10, "12345"},
 
       // NUL bytes do not terminate gets.
-      // TODO(davidben): File BIOs don't get this right. It's unclear if it's
-      // even possible to use fgets correctly here.
       {std::string("abc\0def\nghi", 11), 256, std::string("abc\0def\n", 8)},
 
       // An output size of one means we cannot read any bytes. Only the trailing
@@ -403,22 +401,61 @@ TEST(BIOTest, MemGets) {
     SCOPED_TRACE(t.bio);
     SCOPED_TRACE(t.gets_len);
 
-    bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(t.bio.data(), t.bio.size()));
-    ASSERT_TRUE(bio);
+    auto check_bio_gets = [&](BIO *bio) {
+      std::vector<char> buf(t.gets_len, 'a');
+      int ret = BIO_gets(bio, buf.data(), t.gets_len);
+      ASSERT_GE(ret, 0);
+      // |BIO_gets| should write a NUL terminator, not counted in the return
+      // value.
+      EXPECT_EQ(Bytes(buf.data(), ret + 1),
+                Bytes(t.gets_result.data(), t.gets_result.size() + 1));
+
+      // The remaining data should still be in the BIO.
+      buf.resize(t.bio.size() + 1);
+      ret = BIO_read(bio, buf.data(), static_cast<int>(buf.size()));
+      ASSERT_GE(ret, 0);
+      EXPECT_EQ(Bytes(buf.data(), ret),
+                Bytes(t.bio.substr(t.gets_result.size())));
+    };
+
+    {
+      SCOPED_TRACE("memory");
+      bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(t.bio.data(), t.bio.size()));
+      ASSERT_TRUE(bio);
+      check_bio_gets(bio.get());
+    }
 
-    std::vector<char> buf(t.gets_len, 'a');
-    int ret = BIO_gets(bio.get(), buf.data(), t.gets_len);
-    ASSERT_GE(ret, 0);
-    // |BIO_gets| should write a NUL terminator, not counted in the return
-    // value.
-    EXPECT_EQ(Bytes(buf.data(), ret + 1),
-              Bytes(t.gets_result.data(), t.gets_result.size() + 1));
+    using ScopedFILE = std::unique_ptr<FILE, decltype(&fclose)>;
+    ScopedFILE file(tmpfile(), fclose);
+    ASSERT_TRUE(file);
+    if (!t.bio.empty()) {
+      ASSERT_EQ(1u,
+                fwrite(t.bio.data(), t.bio.size(), /*nitems=*/1, file.get()));
+      ASSERT_EQ(0, fseek(file.get(), 0, SEEK_SET));
+    }
 
-    // The remaining data should still be in the BIO.
-    const uint8_t *contents;
-    size_t len;
-    ASSERT_TRUE(BIO_mem_contents(bio.get(), &contents, &len));
-    EXPECT_EQ(Bytes(contents, len), Bytes(t.bio.substr(ret)));
+    // TODO(crbug.com/boringssl/585): If the line has an embedded NUL, file
+    // BIOs do not currently report the answer correctly.
+    if (t.bio.find('\0') == std::string::npos) {
+      SCOPED_TRACE("file");
+      bssl::UniquePtr<BIO> bio(BIO_new_fp(file.get(), BIO_NOCLOSE));
+      ASSERT_TRUE(bio);
+      check_bio_gets(bio.get());
+    }
+
+    ASSERT_EQ(0, fseek(file.get(), 0, SEEK_SET));
+
+    {
+      SCOPED_TRACE("fd");
+#if defined(OPENSSL_WINDOWS)
+      int fd = _fileno(file.get());
+#else
+      int fd = fileno(file.get());
+#endif
+      bssl::UniquePtr<BIO> bio(BIO_new_fd(fd, BIO_NOCLOSE));
+      ASSERT_TRUE(bio);
+      check_bio_gets(bio.get());
+    }
   }
 
   // Negative and zero lengths should not output anything, even a trailing NUL.
diff --git a/crypto/bio/fd.c b/crypto/bio/fd.c
index 2980b7d..9a2a650 100644
--- a/crypto/bio/fd.c
+++ b/crypto/bio/fd.c
@@ -241,15 +241,18 @@ static long fd_ctrl(BIO *b, int cmd, long num, void *ptr) {
 }
 
 static int fd_gets(BIO *bp, char *buf, int size) {
-  char *ptr = buf;
-  char *end = buf + size - 1;
-
   if (size <= 0) {
     return 0;
   }
 
-  while (ptr < end && fd_read(bp, ptr, 1) > 0 && ptr[0] != '\n') {
+  char *ptr = buf;
+  char *end = buf + size - 1;
+  while (ptr < end && fd_read(bp, ptr, 1) > 0) {
+    char c = ptr[0];
     ptr++;
+    if (c == '\n') {
+      break;
+    }
   }
 
   ptr[0] = '\0';