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>
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 @@
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 @@
{"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 @@
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));
- 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));
+ // 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())));
+ };
- // 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)));
+ {
+ 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());
+ }
+
+ 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));
+ }
+
+ // 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 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';
diff --git a/include/openssl/bio.h b/include/openssl/bio.h
index 01ea69c..abe7aec 100644
--- a/include/openssl/bio.h
+++ b/include/openssl/bio.h
@@ -107,14 +107,14 @@
// bytes read, zero on EOF, or a negative number on error.
OPENSSL_EXPORT int BIO_read(BIO *bio, void *data, int len);
-// BIO_gets "reads a line" from |bio| and puts at most |size| bytes into |buf|.
-// It returns the number of bytes read or a negative number on error. The
-// phrase "reads a line" is in quotes in the previous sentence because the
-// exact operation depends on the BIO's method. For example, a digest BIO will
-// return the digest in response to a |BIO_gets| call.
+// BIO_gets reads a line from |bio| and writes at most |size| bytes into |buf|.
+// It returns the number of bytes read or a negative number on error. This
+// function's output always includes a trailing NUL byte, so it will read at
+// most |size - 1| bytes.
//
-// TODO(fork): audit the set of BIOs that we end up needing. If all actually
-// return a line for this call, remove the warning above.
+// If the function read a complete line, the output will include the newline
+// character, '\n'. If no newline was found before |size - 1| bytes or EOF, it
+// outputs the bytes which were available.
OPENSSL_EXPORT int BIO_gets(BIO *bio, char *buf, int size);
// BIO_write writes |len| bytes from |data| to |bio|. It returns the number of
