Improve documentation for BIO_s_mem

Recent leak discovered by valgrind:
==1007580== at 0x483C815: malloc (vg_replace_malloc.c:431)
==1007580== by 0x2C2689: CRYPTO_zalloc (in /home/vien/microedge-c/test)
==1007580== by 0x295A17: BUF_MEM_new (in /home/vien/microedge-c/test)
==1007580== by 0x295A78: BUF_MEM_new_ex (in /home/vien/microedge-c/test)
==1007580== by 0x28CACE: mem_new (in /home/vien/microedge-c/test)
==1007580== by 0x285EA8: BIO_new_ex (in /home/vien/microedge-c/test)
==1007580== by 0x231894: convert_pubkey_ECC (tpm2_driver.c:221)
==1007580== by 0x232B73: create_ephemeral_key (tpm2_driver.c:641)
==1007580== by 0x232E1F: tpm_gen_keypair (tpm2_driver.c:695)
==1007580== by 0x22D60A: gen_keypair (se_driver_api.c:275)
==1007580== by 0x21FF35: generate_keypair (dhkey.c:142)
==1007580== by 0x24D4C8: __test_dhkey (dhkey_test.c:55)

led me to find that BIO_get_mem_data is informative only, it does not
transer ownership of a BIO_s_mems data structure to the caller.
Additionally treating it as such leads to the above leak, or possibly
data corruption in the event that BIO_set_close(bio, BIO_NOCLOSE) is not
set properly prior to calling BIO_free.

Made an attempt to fix it in a minimally invasive manner in the 3.1
branch, but based on discussion, its just not safe to do in an API
compatible way, so just document the sematics a little more clearly
here, and fix it properly in a future release

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/21724)

diff --git a/doc/man3/BIO_s_mem.pod b/doc/man3/BIO_s_mem.pod
index 6b3cc6a2dae94395e3c94645ceb193d0b5c53d2b..31b2daf33d9ccb12f6e819bfab31b4867831c237 100644 (file)
--- a/doc/man3/BIO_s_mem.pod
+++ b/doc/man3/BIO_s_mem.pod
@@ -59,6 +59,8 @@ positive return value B<v> should be set to a negative value, typically -1.
 
 BIO_get_mem_data() sets *B<pp> to a pointer to the start of the memory BIOs data
 and returns the total amount of data available. It is implemented as a macro.
+Note the pointer returned by this call is informative, no transfer of ownership
+of this memory is implied.  See notes on BIO_set_close().
 
 BIO_set_mem_buf() sets the internal BUF_MEM structure to B<bm> and sets the
 close flag to B<c>, that is B<c> should be either BIO_CLOSE or BIO_NOCLOSE.
@@ -114,6 +116,10 @@ preceding 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.
 
+Calling BIO_set_close() with BIO_NOCLOSE orphans the BUF_MEM internal to the
+BIO, _not_ its actual data buffer. See the examples section for the proper
+method for claiming ownership of the data pointer for a deferred free operation.
+
 =head1 BUGS
 
 There should be an option to set the maximum size of a memory BIO.
@@ -151,6 +157,20 @@ Extract the BUF_MEM structure from a memory BIO and then free up the BIO:
  BIO_set_close(mem, BIO_NOCLOSE); /* So BIO_free() leaves BUF_MEM alone */
  BIO_free(mem);
 
+Extract the BUF_MEM ptr, claim ownership of the internal data and free the BIO
+and BUF_MEM structure:
+
+ BUF_MEM *bptr;
+ char *data;
+
+ BIO_get_mem_data(bio, &data);
+ BIO_get_mem_ptr(bio, &bptr);
+ BIO_set_close(mem, BIO_NOCLOSE); /* So BIO_free orphans BUF_MEM */
+ BIO_free(bio);
+ bptr->data = NULL; /* Tell BUF_MEM to orphan data */
+ BUF_MEM_free(bptr);
+ ...
+ free(data);
 
 =head1 COPYRIGHT