Skip to content

Commit

Permalink
Improve documentation for BIO_s_mem
Browse files Browse the repository at this point in the history
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 #21724)

(cherry picked from commit 66d1658)
  • Loading branch information
nhorman authored and t8m committed Aug 16, 2023
1 parent 1958d26 commit cbf7b6b
Showing 1 changed file with 20 additions and 0 deletions.
20 changes: 20 additions & 0 deletions doc/man3/BIO_s_mem.pod
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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

Expand Down

0 comments on commit cbf7b6b

Please sign in to comment.