Repository: trafficserver Updated Branches: refs/heads/master aa4a9e86e -> f5d62b5c7
TS-3097 Fix double free of SSL_CTX instances. This closes #126. Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/f5d62b5c Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/f5d62b5c Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/f5d62b5c Branch: refs/heads/master Commit: f5d62b5c70de64b6fb82066661e2ae13346779a1 Parents: aa4a9e8 Author: shinrich <[email protected]> Authored: Fri Sep 26 16:15:05 2014 -0500 Committer: Alan M. Carroll <[email protected]> Committed: Fri Sep 26 17:57:12 2014 -0500 ---------------------------------------------------------------------- iocore/net/SSLCertLookup.cc | 30 +++++++++++++++++++----------- iocore/net/test_certlookup.cc | 4 ++++ lib/ts/Vec.h | 8 +++++++- 3 files changed, 30 insertions(+), 12 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/f5d62b5c/iocore/net/SSLCertLookup.cc ---------------------------------------------------------------------- diff --git a/iocore/net/SSLCertLookup.cc b/iocore/net/SSLCertLookup.cc index c7ba9ee..47c70df 100644 --- a/iocore/net/SSLCertLookup.cc +++ b/iocore/net/SSLCertLookup.cc @@ -113,8 +113,6 @@ private: /// Add a context to the clean up list. /// @return The index of the added context. int store(SSLCertContext const& cc); - /// Remove last added context - void unstore(); }; @@ -234,10 +232,25 @@ SSLContextStorage::SSLContextStorage() { } +bool +SSLCtxCompare(SSLCertContext const & cc1, SSLCertContext const & cc2) +{ + // Either they are both real ctx pointers and cc1 has the smaller pointer + // Or only cc2 has a non-null pointer + return cc1.ctx < cc2.ctx; +} + SSLContextStorage::~SSLContextStorage() { + // First sort the array so we can efficiently detect duplicates + // and avoid the double free + this->ctx_store.qsort(SSLCtxCompare); + SSL_CTX *last_ctx = NULL; for (unsigned i = 0; i < this->ctx_store.length(); ++i) { - SSLReleaseContext(this->ctx_store[i].ctx); + if (this->ctx_store[i].ctx != last_ctx) { + last_ctx = this->ctx_store[i].ctx; + SSLReleaseContext(this->ctx_store[i].ctx); + } } ink_hash_table_destroy(this->hostnames); @@ -251,18 +264,12 @@ SSLContextStorage::store(SSLCertContext const& cc) return idx; } -void -SSLContextStorage::unstore() -{ - this->ctx_store.drop(); -} - int SSLContextStorage::insert(const char* name, SSLCertContext const& cc) { int idx = this->store(cc); idx = this->insert(name, idx); - if (idx < 0) this->unstore(); + if (idx < 0) this->ctx_store.drop(); return idx; } @@ -286,6 +293,7 @@ SSLContextStorage::insert(const char* name, int idx) } ref = new ContextRef(idx); + int ref_idx = (*ref).idx; inserted = this->wildcards.Insert(reversed, ref, 0 /* rank */, -1 /* keylen */); if (!inserted) { ContextRef * found; @@ -305,7 +313,7 @@ SSLContextStorage::insert(const char* name, int idx) } Debug("ssl", "%s wildcard certificate for '%s' as '%s' with SSL_CTX %p [%d]", - idx >= 0 ? "index" : "failed to index", name, reversed, this->ctx_store[(*ref).idx].ctx, (*ref).idx); + idx >= 0 ? "index" : "failed to index", name, reversed, this->ctx_store[ref_idx].ctx, ref_idx); } else { InkHashTableValue value; http://git-wip-us.apache.org/repos/asf/trafficserver/blob/f5d62b5c/iocore/net/test_certlookup.cc ---------------------------------------------------------------------- diff --git a/iocore/net/test_certlookup.cc b/iocore/net/test_certlookup.cc index f6374aa..418d06a 100644 --- a/iocore/net/test_certlookup.cc +++ b/iocore/net/test_certlookup.cc @@ -59,6 +59,10 @@ REGRESSION_TEST(SSLCertificateLookup)(RegressionTest* t, int /* atype ATS_UNUSED assert(all_com != NULL); box.check(lookup.insert("www.foo.com", foo_cc) >= 0, "insert host context"); + // Insert the same SSL_CTX instance under another name too + // Should be ok, but also need to make sure that the cleanup does not + // double free the SSL_CTX + box.check(lookup.insert("www.foo2.com", foo_cc) >= 0, "insert host context"); box.check(lookup.insert("*.wild.com", wild_cc) >= 0, "insert wildcard context"); box.check(lookup.insert("*.notwild.com", notwild_cc) >= 0, "insert wildcard context"); box.check(lookup.insert("*.b.notwild.com", b_notwild_cc) >= 0, "insert wildcard context"); http://git-wip-us.apache.org/repos/asf/trafficserver/blob/f5d62b5c/lib/ts/Vec.h ---------------------------------------------------------------------- diff --git a/lib/ts/Vec.h b/lib/ts/Vec.h index 9dcf933..cc233f5 100644 --- a/lib/ts/Vec.h +++ b/lib/ts/Vec.h @@ -112,6 +112,7 @@ class Vec { int write(int fd); int read(int fd); void qsort(bool (*lt)(C,C)); + void qsort(bool (*lt)(const C &, const C &)); private: void move_internal(Vec<C,A,S> &v); @@ -794,7 +795,7 @@ inline void qsort_Vec(C *left, C *right, bool (*lt)(C,C)) { } template <class C> -inline void qsort_VecRef(C *left, C *right, bool (*lt)(C&,C&)) { +inline void qsort_VecRef(C *left, C *right, bool (*lt)(const C &, const C &)) { Lagain: if (right - left < 5) { for (C *y = right - 1; y > left; y--) { @@ -835,6 +836,11 @@ inline void Vec<C,A,S>::qsort(bool (*lt)(C,C)) { qsort_Vec<C>(&v[0], end(), lt); } +template <class C, class A, int S> +inline void Vec<C,A,S>::qsort(bool (*lt)(const C &, const C &)) { + if (n) + qsort_VecRef<C>(&v[0], end(), lt); +} void test_vec(); #endif
