From 8b7b32921e63c492fa7233d81b11ee4d7ba266de Mon Sep 17 00:00:00 2001 From: Tomas Mraz Date: Tue, 18 Jun 2019 16:41:48 +0200 Subject: [PATCH] Fix and document BIO_FLAGS_NONCLEAR_RST behavior on memory BIO The BIO_FLAGS_NONCLEAR_RST flag behavior was not properly documented and it also caused the length to be incorrectly set after the reset operation. Reviewed-by: Bernd Edlinger Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/9179) --- crypto/bio/bss_mem.c | 4 +--- doc/man3/BIO_s_mem.pod | 26 ++++++++++++++++++-------- test/bio_memleak_test.c | 40 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/crypto/bio/bss_mem.c b/crypto/bio/bss_mem.c index a7f2bfbae0..19a3bd88ba 100644 --- a/crypto/bio/bss_mem.c +++ b/crypto/bio/bss_mem.c @@ -259,9 +259,7 @@ static long mem_ctrl(BIO *b, int cmd, long num, void *ptr) bm = bbm->buf; if (bm->data != NULL) { if (!(b->flags & BIO_FLAGS_MEM_RDONLY)) { - if (b->flags & BIO_FLAGS_NONCLEAR_RST) { - bm->length = bm->max; - } else { + if (!(b->flags & BIO_FLAGS_NONCLEAR_RST)) { memset(bm->data, 0, bm->max); bm->length = 0; } diff --git a/doc/man3/BIO_s_mem.pod b/doc/man3/BIO_s_mem.pod index 6d9e747b25..42fc294417 100644 --- a/doc/man3/BIO_s_mem.pod +++ b/doc/man3/BIO_s_mem.pod @@ -41,9 +41,10 @@ If the BIO_CLOSE flag is set when a memory BIO is freed then the underlying BUF_MEM structure is also freed. Calling BIO_reset() on a read write memory BIO clears any data in it if the -flag BIO_FLAGS_NONCLEAR_RST is not set. On a read only BIO or if the flag -BIO_FLAGS_NONCLEAR_RST is set it restores the BIO to its original state and -the data can be read again. +flag BIO_FLAGS_NONCLEAR_RST is not set, otherwise it just restores the read +pointer to the state it was just after the last write was performed and the +data can be read again. On a read only BIO it similarly restores the BIO to +its original state and the read only data can be read again. BIO_eof() is true if no data is in the BIO. @@ -79,11 +80,11 @@ first, so the supplied area of memory must be unchanged until the BIO is freed. Writes to memory BIOs will always succeed if memory is available: that is their size can grow indefinitely. -Every read from a read write memory BIO will remove the data just read with -an internal copy operation, if a BIO contains a lot of data and it is -read in small chunks the operation can be very slow. The use of a read only -memory BIO avoids this problem. If the BIO must be read write then adding -a buffering BIO to the chain will speed up the process. +Every write after partial read (not all data in the memory buffer was read) +to a read write memory BIO will have to move the unread data with an internal +copy operation, if a BIO contains a lot of data and it is read in small +chunks intertwined with writes the operation can be very slow. Adding +a buffering BIO to the chain can speed up the process. Calling BIO_set_mem_buf() on a BIO created with BIO_new_secmem() will give undefined results, including perhaps a program crash. @@ -104,6 +105,15 @@ BIO is set to BIO_NOCLOSE, before freeing the BUF_MEM the data pointer in it must be set to NULL as the data pointer does not point to an allocated memory. +Calling BIO_reset() on a read write memory BIO with BIO_FLAGS_NONCLEAR_RST +flag set can have unexpected outcome when the reads and writes to the +BIO are intertwined. As documented above the BIO will be reset to the +state after the last completed write operation. The effects of reads +preceeding that write operation cannot be undone. + +Calling BIO_get_mem_ptr() prior to a BIO_reset() call with +BIO_FLAGS_NONCLEAR_RST set has the same effect as a write operation. + =head1 BUGS There should be an option to set the maximum size of a memory BIO. diff --git a/test/bio_memleak_test.c b/test/bio_memleak_test.c index fab5ce73cf..dab61f6f9a 100644 --- a/test/bio_memleak_test.c +++ b/test/bio_memleak_test.c @@ -181,6 +181,45 @@ finish: return ok; } +static int test_bio_nonclear_rst(void) +{ + int ok = 0; + BIO *bio = NULL; + char data[16]; + + bio = BIO_new(BIO_s_mem()); + if (!TEST_ptr(bio)) + goto finish; + if (!TEST_int_eq(BIO_puts(bio, "Hello World\n"), 12)) + goto finish; + + BIO_set_flags(bio, BIO_FLAGS_NONCLEAR_RST); + + if (!TEST_int_eq(BIO_read(bio, data, 16), 12)) + goto finish; + if (!TEST_mem_eq(data, 12, "Hello World\n", 12)) + goto finish; + if (!TEST_int_gt(BIO_reset(bio), 0)) + goto finish; + + if (!TEST_int_eq(BIO_read(bio, data, 16), 12)) + goto finish; + if (!TEST_mem_eq(data, 12, "Hello World\n", 12)) + goto finish; + + BIO_clear_flags(bio, BIO_FLAGS_NONCLEAR_RST); + if (!TEST_int_gt(BIO_reset(bio), 0)) + goto finish; + + if (!TEST_int_lt(BIO_read(bio, data, 16), 1)) + goto finish; + + ok = 1; + +finish: + BIO_free(bio); + return ok; +} int global_init(void) { @@ -196,5 +235,6 @@ int setup_tests(void) ADD_TEST(test_bio_new_mem_buf); ADD_TEST(test_bio_rdonly_mem_buf); ADD_TEST(test_bio_rdwr_rdonly); + ADD_TEST(test_bio_nonclear_rst); return 1; } -- 2.34.1