Fix the docs for and test BIO_free's return value

Also remove the TODO(fork) to drop BIO_free_all. Too much third-party
code relies on it.

Bug: 485657226
Change-Id: I470d90e76548cb43f514d607a0ba5a50511b0978
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/89707
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Xiangfei Ding <xfding@google.com>
Commit-Queue: Xiangfei Ding <xfding@google.com>

crypto/bio/bio_test.cc

diff --git a/crypto/bio/bio_test.cc b/crypto/bio/bio_test.cc
index 2de44e4..aedf3aa 100644
--- a/crypto/bio/bio_test.cc
+++ b/crypto/bio/bio_test.cc
@@ -923,5 +923,36 @@
 
 INSTANTIATE_TEST_SUITE_P(All, BIOPairTest, testing::Values(false, true));
 
+// |BIO_free| returns whether the input |BIO| was shared.
+TEST(BIOTest, BIOFreeReturnValue) {
+  BIO *bio = BIO_new_mem_buf(nullptr, 0);
+  ASSERT_TRUE(bio);
+  BIO_up_ref(bio);
+  BIO_up_ref(bio);
+
+  // |BIO_free| should return one when the last reference is dropped.
+  EXPECT_EQ(0, BIO_free(bio));
+  EXPECT_EQ(0, BIO_free(bio));
+  EXPECT_EQ(1, BIO_free(bio));
+
+  // |BIO_free| of nullptr vacuously returns one.
+  EXPECT_EQ(1, BIO_free(nullptr));
+}
+
+TEST(BIOTest, BIOFreeReturnValueChain) {
+  // We have no built-in filter BIOs, but |BIO_push| works with any |BIO|, so
+  // just chain memory |BIO|s, even though it does nothing.
+  UniquePtr<BIO> bio1(BIO_new_mem_buf(nullptr, 0));
+  ASSERT_TRUE(bio1);
+  UniquePtr<BIO> bio2(BIO_new_mem_buf(nullptr, 0));
+  ASSERT_TRUE(bio2);
+  BIO_push(bio1.get(), UpRef(bio2).release());
+
+  // |bio1| now owns a copy of |bio2|, but it is still shared with the |bio2|
+  // pointer. This causes |BIO_free| to return zero.
+  // TODO(crbug.com/485657226): It should return one.
+  EXPECT_EQ(0, BIO_free(bio1.release()));
+}
+
 }  // namespace
 BSSL_NAMESPACE_END

include/openssl/bio.h

diff --git a/include/openssl/bio.h b/include/openssl/bio.h
index b19607a..59ca6b8 100644
--- a/include/openssl/bio.h
+++ b/include/openssl/bio.h
@@ -42,10 +42,22 @@
 
 // BIO_free decrements the reference count of |bio|. If the reference count
 // drops to zero, it calls the destroy callback, if present, on the method and
-// frees |bio| itself. It then repeats that for the next BIO in the chain, if
-// any.
+// frees |bio| itself. If |bio| is part of a chain (see |BIO_push|), this will
+// also free the next |BIO| in the chain, and so on.
 //
-// It returns one on success or zero otherwise.
+// It returns one if |bio| was NULL or freed. It returns zero if |bio| was
+// shared and some other owner still owns a reference count to it.
+//
+// TODO(crbug.com/485657226): Currently, if any |BIO| in the chain is shared,
+// this function will also return zero. It should only return zero when the
+// input |BIO| is shared.
+//
+// WARNING: Do not use the return value. Returning zero is not a sign of an
+// error, nor an indication to retry the operation. |BIO| is a reference-counted
+// type. A given |BIO| object may be shared between multiple parts of an
+// application. To correctly track the reference count, without leaks or
+// use-after-free, each part of the application must release only the reference
+// counts it owns.
 OPENSSL_EXPORT int BIO_free(BIO *bio);
 
 // BIO_vfree performs the same actions as |BIO_free|, but has a void return
@@ -238,11 +250,6 @@
 // no such BIO.
 OPENSSL_EXPORT BIO *BIO_next(BIO *bio);
 
-// BIO_free_all calls |BIO_free|.
-//
-// TODO(fork): update callers and remove.
-OPENSSL_EXPORT void BIO_free_all(BIO *bio);
-
 // BIO_find_type walks a chain of BIOs and returns the first that matches
 // |type|, which is one of the |BIO_TYPE_*| values.
 OPENSSL_EXPORT BIO *BIO_find_type(BIO *bio, int type);
@@ -778,6 +785,10 @@
 
 // Deprecated functions.
 
+// BIO_free_all calls |BIO_free|. Code that targets BoringSSL does not need to
+// call a separate free function for |BIO|s that are part of a chain.
+OPENSSL_EXPORT void BIO_free_all(BIO *bio);
+
 typedef BIO_info_cb bio_info_cb;
 
 // BIO_f_base64 returns a filter |BIO| that base64-encodes data written into