Avoid another NULL+0 in BIO_s_mem
That NULL+0 is forbidden is still an awful language bug in C (fixed in
C++), but this particular instance could have been written without
pointer arithmetic. While I'm here, tidy pointers a bit:
- No need to cast pointers to char* when we're writing to void* anyway
- BIO_C_GET_BUF_MEM_PTR is technically a strict aliasing violation. The
pointer points to a BUF_MEM*, not a char*, so we should not write to
it as a char**.
- C casts from void* freely and we've usually omitted the cast in that
case. (Though if we ever move libcrypto to C++, that'll all have to
change.)
Bug: b:286384999
Change-Id: I16d7da675d61f726f259fc9a3cc4a6fce2d6d1fd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60605
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
diff --git a/crypto/bio/bio_mem.c b/crypto/bio/bio_mem.c
index 1ee148b..5d7534c 100644
--- a/crypto/bio/bio_mem.c
+++ b/crypto/bio/bio_mem.c
@@ -206,7 +206,6 @@
static long mem_ctrl(BIO *bio, int cmd, long num, void *ptr) {
long ret = 1;
- char **pptr;
BUF_MEM *b = (BUF_MEM *)bio->ptr;
@@ -232,8 +231,8 @@
case BIO_CTRL_INFO:
ret = (long)b->length;
if (ptr != NULL) {
- pptr = (char **)ptr;
- *pptr = (char *)&b->data[0];
+ char **pptr = ptr;
+ *pptr = b->data;
}
break;
case BIO_C_SET_BUF_MEM:
@@ -243,8 +242,8 @@
break;
case BIO_C_GET_BUF_MEM_PTR:
if (ptr != NULL) {
- pptr = (char **)ptr;
- *pptr = (char *)b;
+ BUF_MEM **pptr = ptr;
+ *pptr = b;
}
break;
case BIO_CTRL_GET_CLOSE:
@@ -294,15 +293,15 @@
}
long BIO_get_mem_data(BIO *bio, char **contents) {
- return BIO_ctrl(bio, BIO_CTRL_INFO, 0, (char *) contents);
+ return BIO_ctrl(bio, BIO_CTRL_INFO, 0, contents);
}
int BIO_get_mem_ptr(BIO *bio, BUF_MEM **out) {
- return (int)BIO_ctrl(bio, BIO_C_GET_BUF_MEM_PTR, 0, (char *) out);
+ return (int)BIO_ctrl(bio, BIO_C_GET_BUF_MEM_PTR, 0, out);
}
int BIO_set_mem_buf(BIO *bio, BUF_MEM *b, int take_ownership) {
- return (int)BIO_ctrl(bio, BIO_C_SET_BUF_MEM, take_ownership, (char *) b);
+ return (int)BIO_ctrl(bio, BIO_C_SET_BUF_MEM, take_ownership, b);
}
int BIO_set_mem_eof_return(BIO *bio, int eof_value) {
diff --git a/crypto/bio/bio_test.cc b/crypto/bio/bio_test.cc
index 6610be9..8827ce6 100644
--- a/crypto/bio/bio_test.cc
+++ b/crypto/bio/bio_test.cc
@@ -287,11 +287,24 @@
bssl::UniquePtr<BIO> bio(BIO_new(BIO_s_mem()));
ASSERT_TRUE(bio);
+ auto check_bio_contents = [&](Bytes b) {
+ const uint8_t *contents;
+ size_t len;
+ ASSERT_TRUE(BIO_mem_contents(bio.get(), &contents, &len));
+ EXPECT_EQ(Bytes(contents, len), b);
+
+ char *contents_c;
+ long len_l = BIO_get_mem_data(bio.get(), &contents_c);
+ ASSERT_GE(len_l, 0);
+ EXPECT_EQ(Bytes(contents_c, len_l), b);
+
+ BUF_MEM *buf;
+ ASSERT_EQ(BIO_get_mem_ptr(bio.get(), &buf), 1);
+ EXPECT_EQ(Bytes(buf->data, buf->length), b);
+ };
+
// It is initially empty.
- const uint8_t *contents;
- size_t len;
- ASSERT_TRUE(BIO_mem_contents(bio.get(), &contents, &len));
- EXPECT_EQ(Bytes(contents, len), Bytes(""));
+ check_bio_contents(Bytes(""));
EXPECT_EQ(BIO_eof(bio.get()), 1);
// Reading from it should default to returning a retryable error.
@@ -310,44 +323,38 @@
// Writes append to the buffer.
ASSERT_EQ(BIO_write(bio.get(), "abcdef", 6), 6);
- ASSERT_TRUE(BIO_mem_contents(bio.get(), &contents, &len));
- EXPECT_EQ(Bytes(contents, len), Bytes("abcdef"));
+ check_bio_contents(Bytes("abcdef"));
EXPECT_EQ(BIO_eof(bio.get()), 0);
// Writes can include embedded NULs.
ASSERT_EQ(BIO_write(bio.get(), "\0ghijk", 6), 6);
- ASSERT_TRUE(BIO_mem_contents(bio.get(), &contents, &len));
- EXPECT_EQ(Bytes(contents, len), Bytes("abcdef\0ghijk", 12));
+ check_bio_contents(Bytes("abcdef\0ghijk", 12));
EXPECT_EQ(BIO_eof(bio.get()), 0);
// Do a partial read.
int ret = BIO_read(bio.get(), buf, 4);
ASSERT_GT(ret, 0);
EXPECT_EQ(Bytes(buf, ret), Bytes("abcd"));
- ASSERT_TRUE(BIO_mem_contents(bio.get(), &contents, &len));
- EXPECT_EQ(Bytes(contents, len), Bytes("ef\0ghijk", 8));
+ check_bio_contents(Bytes("ef\0ghijk", 8));
EXPECT_EQ(BIO_eof(bio.get()), 0);
// Reads and writes may alternate.
ASSERT_EQ(BIO_write(bio.get(), "lmnopq", 6), 6);
- ASSERT_TRUE(BIO_mem_contents(bio.get(), &contents, &len));
- EXPECT_EQ(Bytes(contents, len), Bytes("ef\0ghijklmnopq", 14));
+ check_bio_contents(Bytes("ef\0ghijklmnopq", 14));
EXPECT_EQ(BIO_eof(bio.get()), 0);
// Reads may consume embedded NULs.
ret = BIO_read(bio.get(), buf, 4);
ASSERT_GT(ret, 0);
EXPECT_EQ(Bytes(buf, ret), Bytes("ef\0g", 4));
- ASSERT_TRUE(BIO_mem_contents(bio.get(), &contents, &len));
- EXPECT_EQ(Bytes(contents, len), Bytes("hijklmnopq"));
+ check_bio_contents(Bytes("hijklmnopq"));
EXPECT_EQ(BIO_eof(bio.get()), 0);
// The read buffer exceeds the |BIO|, so we consume everything.
ret = BIO_read(bio.get(), buf, sizeof(buf));
ASSERT_GT(ret, 0);
EXPECT_EQ(Bytes(buf, ret), Bytes("hijklmnopq"));
- ASSERT_TRUE(BIO_mem_contents(bio.get(), &contents, &len));
- EXPECT_EQ(Bytes(contents, len), Bytes(""));
+ check_bio_contents(Bytes(""));
EXPECT_EQ(BIO_eof(bio.get()), 1);
// The |BIO| is now empty.
