[PATCH 1/7] Update to the SyncResponder to prevent free'd memory


Gary Molloy <GMolloy@...>
 

Testing

This is not OS dependant
This is not middleware dependant

 

This is a subtle race condition which could not be recreated directly with a MAMA application.

We were able to recreate it consistently with an enterprise application and can confirm that

The issue is resolved.

 

 

From d99fba4a10275c97f0c302149f05f5bc801107f7 Mon Sep 17 00:00:00 2001

From: Gary Molloy <gmolloy@...>

Date: Mon, 23 Jun 2014 11:18:26 +0100

Subject: [PATCH 1/7] Update to the SyncResponder to prevent free'd memory being read.

 

[OMAMA-271]

 

Description – When a sync request is received symbol names are copied retrieved from the transport list.

However this is a not a deep copy and if a subscription is removed from the list before the response is sent

The response can be corrupted.

Taking a deep copy of the symbol name(s) will prevent this.

 

Signed-off-by: Gary Molloy <gmolloy@...>

---

mama/c_cpp/src/c/syncresponder.c | 8 +++++++-

mama/c_cpp/src/c/transport.c     | 4 ++--

2 files changed, 9 insertions(+), 3 deletions(-)

 

diff --git a/mama/c_cpp/src/c/syncresponder.c b/mama/c_cpp/src/c/syncresponder.c

index 5141e7d..abec008 100644

--- a/mama/c_cpp/src/c/syncresponder.c

+++ b/mama/c_cpp/src/c/syncresponder.c

@@ -50,7 +50,7 @@ typedef struct

     mamaTimer        mTimer;

     int              mCurIndex;

     mama_u16_t       mTopicsPerMsg;

-    const char**     mTopics;

+    const char**     mTopics;                       /* Array of deep copies of the symbol names. */

     int              mTopicsLen;

     mama_i32_t*      mTypes;

     double           mResponseInterval;

@@ -163,6 +163,7 @@ setup (syncCommand* impl)

  * Invoked by CmManager once it finsishes its clean up */

void syncCommand_dtor(void *handle)

{

+    int ii = 0;

     syncCommand *impl = (syncCommand*)handle;

     mama_log (MAMA_LOG_LEVEL_FINE, "Destroying sync command.");

@@ -175,6 +176,11 @@ void syncCommand_dtor(void *handle)

         impl->mTimer = NULL;

     }

+    for (ii = 0; ii < impl->mTopicsLen; ++ii)

+    {

+        free ((void*)impl->mTopics [ii]);

+    }

+

     free ((void*)impl->mTopics);

     free (impl->mTypes);

diff --git a/mama/c_cpp/src/c/transport.c b/mama/c_cpp/src/c/transport.c

index 4743a12..339dbc0 100644

--- a/mama/c_cpp/src/c/transport.c

+++ b/mama/c_cpp/src/c/transport.c

@@ -1860,7 +1860,7 @@ struct topicsForSourceClosure

{

     int curIdx;

     const char* source;

-    const char** topics;

+    const char** topics;    /* Array of deep copies of the symbol names. */

     mama_i32_t*  types;

     /* This is index of the sub transport bridge that the subscription is

@@ -1897,7 +1897,7 @@ topicsForSourceIterator (wList list, void* element, void* c)

                 mamaSubscriptionType subscType = MAMA_SUBSC_TYPE_NORMAL;

                 mamaSubscription_getSubscriptionType (subsc->mSubscription, &subscType);

                 closure->types[closure->curIdx] = subscType;

-                closure->topics[closure->curIdx++] = topic;

+                closure->topics[closure->curIdx++] = strdup (topic);

             }

         }

     }

--

1.8.3.1

 


This message may contain confidential information and is intended for specific recipients unless explicitly noted otherwise. If you have reason to believe you are not an intended recipient of this message, please delete it and notify the sender. This message may not represent the opinion of IntercontinentalExchange Group, Inc. (ICE), NYSE Euronext or any of their subsidiaries or affiliates, and does not constitute a contract or guarantee. Unencrypted electronic mail is not secure and the recipient of this message is expected to provide safeguards from viruses and pursue alternate means of communication where privacy or a binding message is desired.


Damian Maguire <DMaguire@...>
 

Cheers Gary, raised as BZ131

D

From: Gary Molloy <GMolloy@...>
Date: Tuesday, June 24, 2014 10:52 AM
To: "openmama-dev@..." <openmama-dev@...>
Subject: [Openmama-dev] [PATCH 1/7] Update to the SyncResponder to prevent free'd memory

Testing

This is not OS dependant
This is not middleware dependant

 

This is a subtle race condition which could not be recreated directly with a MAMA application.

We were able to recreate it consistently with an enterprise application and can confirm that

The issue is resolved.

 

 

From d99fba4a10275c97f0c302149f05f5bc801107f7 Mon Sep 17 00:00:00 2001

From: Gary Molloy <gmolloy@...>

Date: Mon, 23 Jun 2014 11:18:26 +0100

Subject: [PATCH 1/7] Update to the SyncResponder to prevent free'd memory being read.

 

[OMAMA-271]

 

Description – When a sync request is received symbol names are copied retrieved from the transport list.

However this is a not a deep copy and if a subscription is removed from the list before the response is sent

The response can be corrupted.

Taking a deep copy of the symbol name(s) will prevent this.

 

Signed-off-by: Gary Molloy <gmolloy@...>

---

mama/c_cpp/src/c/syncresponder.c | 8 +++++++-

mama/c_cpp/src/c/transport.c     | 4 ++--

2 files changed, 9 insertions(+), 3 deletions(-)

 

diff --git a/mama/c_cpp/src/c/syncresponder.c b/mama/c_cpp/src/c/syncresponder.c

index 5141e7d..abec008 100644

--- a/mama/c_cpp/src/c/syncresponder.c

+++ b/mama/c_cpp/src/c/syncresponder.c

@@ -50,7 +50,7 @@ typedef struct

     mamaTimer        mTimer;

     int              mCurIndex;

     mama_u16_t       mTopicsPerMsg;

-    const char**     mTopics;

+    const char**     mTopics;                       /* Array of deep copies of the symbol names. */

     int              mTopicsLen;

     mama_i32_t*      mTypes;

     double           mResponseInterval;

@@ -163,6 +163,7 @@ setup (syncCommand* impl)

  * Invoked by CmManager once it finsishes its clean up */

void syncCommand_dtor(void *handle)

{

+    int ii = 0;

     syncCommand *impl = (syncCommand*)handle;

     mama_log (MAMA_LOG_LEVEL_FINE, "Destroying sync command.");

@@ -175,6 +176,11 @@ void syncCommand_dtor(void *handle)

         impl->mTimer = NULL;

     }

+    for (ii = 0; ii < impl->mTopicsLen; ++ii)

+    {

+        free ((void*)impl->mTopics [ii]);

+    }

+

     free ((void*)impl->mTopics);

     free (impl->mTypes);

diff --git a/mama/c_cpp/src/c/transport.c b/mama/c_cpp/src/c/transport.c

index 4743a12..339dbc0 100644

--- a/mama/c_cpp/src/c/transport.c

+++ b/mama/c_cpp/src/c/transport.c

@@ -1860,7 +1860,7 @@ struct topicsForSourceClosure

{

     int curIdx;

     const char* source;

-    const char** topics;

+    const char** topics;    /* Array of deep copies of the symbol names. */

     mama_i32_t*  types;

     /* This is index of the sub transport bridge that the subscription is

@@ -1897,7 +1897,7 @@ topicsForSourceIterator (wList list, void* element, void* c)

                 mamaSubscriptionType subscType = MAMA_SUBSC_TYPE_NORMAL;

                 mamaSubscription_getSubscriptionType (subsc->mSubscription, &subscType);

                 closure->types[closure->curIdx] = subscType;

-                closure->topics[closure->curIdx++] = topic;

+                closure->topics[closure->curIdx++] = strdup (topic);

             }

         }

     }

--

1.8.3.1

 


This message may contain confidential information and is intended for specific recipients unless explicitly noted otherwise. If you have reason to believe you are not an intended recipient of this message, please delete it and notify the sender. This message may not represent the opinion of IntercontinentalExchange Group, Inc. (ICE), NYSE Euronext or any of their subsidiaries or affiliates, and does not constitute a contract or guarantee. Unencrypted electronic mail is not secure and the recipient of this message is expected to provide safeguards from viruses and pursue alternate means of communication where privacy or a binding message is desired.

This message may contain confidential information and is intended for specific recipients unless explicitly noted otherwise. If you have reason to believe you are not an intended recipient of this message, please delete it and notify the sender. This message may not represent the opinion of Intercontinental Exchange, Inc. (ICE), Euronext or any of their subsidiaries or affiliates, and does not constitute a contract or guarantee. Unencrypted electronic mail is not secure and the recipient of this message is expected to provide safeguards from viruses and pursue alternate means of communication where privacy or a binding message is desired.