trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From a..@apache.org
Subject git commit: TS-3097 Fix double free of SSL_CTX instances. This closes #126.
Date Fri, 26 Sep 2014 22:57:45 GMT
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 <shinrich@network-geographics.com>
Authored: Fri Sep 26 16:15:05 2014 -0500
Committer: Alan M. Carroll <amc@apache.org>
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


Mime
View raw message