Fix BIO_find_type with BIO chains This fixes a regression from https://boringssl-review.googlesource.com/c/boringssl/+/87707, which mixed up bio and impl variables. Fix this by just calling the accessors that do the same thing. Change-Id: Ic4e78970da8825433acf8a84fd0149ccb97a95f5 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/92908 Auto-Submit: David Benjamin <davidben@google.com> Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Rudolf Polzer <rpolzer@google.com> Commit-Queue: Rudolf Polzer <rpolzer@google.com>
diff --git a/crypto/bio/bio.cc b/crypto/bio/bio.cc index d02847a..d98be33 100644 --- a/crypto/bio/bio.cc +++ b/crypto/bio/bio.cc
@@ -387,19 +387,14 @@ } BIO *BIO_find_type(BIO *bio, int type) { - auto *impl = FromOpaque(bio); - - int method_type, mask; - if (!bio) { return nullptr; } - mask = type & 0xff; + int mask = type & 0xff; do { - if (impl->method != nullptr) { - method_type = impl->method->type; - + if (FromOpaque(bio)->method != nullptr) { + int method_type = BIO_method_type(bio); if (!mask) { if (method_type & type) { return bio; @@ -408,7 +403,7 @@ return bio; } } - bio = impl->next_bio; + bio = BIO_next(bio); } while (bio != nullptr); return nullptr;
diff --git a/crypto/bio/bio_test.cc b/crypto/bio/bio_test.cc index bd8e333..afe35dd 100644 --- a/crypto/bio/bio_test.cc +++ b/crypto/bio/bio_test.cc
@@ -13,6 +13,7 @@ // limitations under the License. #include <algorithm> +#include <optional> #include <string> #include <utility> @@ -985,5 +986,92 @@ EXPECT_EQ(1, BIO_free(bio1.release())); } +TEST(BIOTest, BIOChain) { + auto make_bio_method = + [](int mask) -> std::optional<std::pair<int, const BIO_METHOD *>> { + int index = BIO_get_new_index(); + if (index < 0) { + return std::nullopt; + } + int type = index | mask; + BIO_METHOD *meth = BIO_meth_new(type, "test"); + if (meth == nullptr) { + return std::nullopt; + } + return std::pair(type, meth); + }; + + // There are a limited of BIO_METHOD indices per process. Allocate these + // statically so the test can be repeated safely. + static auto method1 = make_bio_method(BIO_TYPE_FILTER); + ASSERT_TRUE(method1); + static auto method2 = make_bio_method(BIO_TYPE_FILTER); + ASSERT_TRUE(method2); + static auto method3 = make_bio_method(BIO_TYPE_FILTER | BIO_TYPE_SOURCE_SINK); + ASSERT_TRUE(method3); + static auto method4 = make_bio_method(BIO_TYPE_SOURCE_SINK); + ASSERT_TRUE(method4); + + // Make a chain of BIOs, one from each method. + UniquePtr<BIO> bio(BIO_new(method1->second)); + ASSERT_TRUE(bio); + for (const auto &method : {method2, method3, method4}) { + UniquePtr<BIO> next(BIO_new(method->second)); + ASSERT_TRUE(next); + BIO_push(bio.get(), next.release()); + } + + // Check the |BIO_next| chain is what we expect. + auto expect_bio_chain = [](BIO *b, const std::vector<int> &types) { + for (int type : types) { + ASSERT_TRUE(b); + EXPECT_EQ(BIO_method_type(b), type); + b = BIO_next(b); + } + EXPECT_FALSE(b); + }; + expect_bio_chain(bio.get(), {method1->first, method2->first, method3->first, + method4->first}); + + // |BIO_find_type| should find all of them. + for (const auto &method : {method1, method2, method3, method4}) { + SCOPED_TRACE(method->first); + BIO *found = BIO_find_type(bio.get(), method->first); + ASSERT_TRUE(found); + EXPECT_EQ(BIO_method_type(found), method->first); + } + + // |BIO_find_type| can also look by mask. + BIO *found = BIO_find_type(bio.get(), BIO_TYPE_FILTER); + ASSERT_TRUE(found); + EXPECT_EQ(BIO_method_type(found), method1->first); + + found = BIO_find_type(bio.get(), BIO_TYPE_SOURCE_SINK); + ASSERT_TRUE(found); + EXPECT_EQ(BIO_method_type(found), method3->first); + + found = BIO_find_type(bio.get(), BIO_TYPE_DESCRIPTOR | BIO_TYPE_SOURCE_SINK); + ASSERT_TRUE(found); + EXPECT_EQ(BIO_method_type(found), method3->first); + + found = BIO_find_type(bio.get(), BIO_TYPE_FILTER | BIO_TYPE_SOURCE_SINK); + ASSERT_TRUE(found); + EXPECT_EQ(BIO_method_type(found), method1->first); + + // Not found, by exact match and by mask. + EXPECT_FALSE(BIO_find_type(bio.get(), BIO_TYPE_MEM)); + EXPECT_FALSE(BIO_find_type(bio.get(), BIO_TYPE_DESCRIPTOR)); + EXPECT_FALSE(BIO_find_type(bio.get(), 0)); + + // Pop the front of the chain. + UniquePtr<BIO> rest(BIO_pop(bio.get())); + + // bio is now a free-floating BIO. + expect_bio_chain(bio.get(), {method1->first}); + // The remainder was returned. + expect_bio_chain(rest.get(), + {method2->first, method3->first, method4->first}); +} + } // namespace BSSL_NAMESPACE_END
diff --git a/include/openssl/bio.h b/include/openssl/bio.h index 825c8bf..5c3dda0 100644 --- a/include/openssl/bio.h +++ b/include/openssl/bio.h
@@ -255,6 +255,12 @@ // BIO_find_type walks a chain of BIOs and returns the first that matches // |type|, which is one of the |BIO_TYPE_*| values. +// +// If |type & 0xff| is non-zero, i.e. |type| is a complete type and contains an +// "index" component, the function looks for an exact match. If |type & 0xff| is +// zero, i.e. |type| just specifies |BIO_TYPE_DESCRIPTOR|, |BIO_TYPE_FILTER|, +// and |BIO_TYPE_SOURCE_SINK| bits, the function looks for any BIO whose type +// contains at least one of those bits. OPENSSL_EXPORT BIO *BIO_find_type(BIO *bio, int type); // BIO_copy_next_retry sets the retry flags and |retry_reason| of |bio| from @@ -640,7 +646,8 @@ // Consumers can create custom |BIO|s by filling in a |BIO_METHOD| and using // low-level control functions to set state. -// BIO_get_new_index returns a new "type" value for a custom |BIO|. +// BIO_get_new_index returns a new "type" value for a custom |BIO|, or -1 on +// error. OPENSSL_EXPORT int BIO_get_new_index(void); // BIO_meth_new returns a newly-allocated |BIO_METHOD| or NULL on allocation