Fixed possible heap corruption in EffectDesc

"EffectDesc *effect = new EffectDesc(*effects[i]);" was relying on the
default copy constructor for EffectDesc, but the default copy constructor
does a member-by-member copy.  This works OK for mUuid, but a member
copy of mName and mParams shares pointers.  This could result in heap
corruption later on due to a double free.  Changed to add an explicit
copy constructor that does a deep copy of both mName and mParams.

A malloc() and strdup() were being freed by delete, but the correct
matching API for these is free().  Fortunately our current memory runtime
implementation ignores the difference. Changed to use free().

EffectDesc and InputSourceDesc member fields were being torn down by
the code that does delete.  Changed to do the tear-down in ~EffectDesc()
and ~InputSourceDesc().

Added constructor EffectDesc() with name and UUID parameters, rather
than having caller fill in the object after construction.

Made ~EffectDesc() and ~InputSourceDesc() non-virtual to save memory,
since they have no subclasses.

Change-Id: Ibb5cc2e6760d72e0c4cf537068ac4432c717bafd

2 files changed, 35 insertions, 22 deletions

diff --git a/services/audioflinger/AudioPolicyService.cpp b/services/audioflinger/AudioPolicyService.cpp
index 041b5a869b0a..364fd2275694 100644
--- a/services/audioflinger/AudioPolicyService.cpp
+++ b/services/audioflinger/AudioPolicyService.cpp
@@ -116,19 +116,7 @@ AudioPolicyService::~AudioPolicyService()
 
     // release audio pre processing resources
     for (size_t i = 0; i < mInputSources.size(); i++) {
-        InputSourceDesc *source = mInputSources.valueAt(i);
-        Vector <EffectDesc *> effects = source->mEffects;
-        for (size_t j = 0; j < effects.size(); j++) {
-            delete effects[j]->mName;
-            Vector <effect_param_t *> params = effects[j]->mParams;
-            for (size_t k = 0; k < params.size(); k++) {
-                delete params[k];
-            }
-            params.clear();
-            delete effects[j];
-        }
-        effects.clear();
-        delete source;
+        delete mInputSources.valueAt(i);
     }
     mInputSources.clear();
 
@@ -1243,7 +1231,7 @@ AudioPolicyService::InputSourceDesc *AudioPolicyService::loadInputSource(
             node = node->next;
             continue;
         }
-        EffectDesc *effect = new EffectDesc(*effects[i]);
+        EffectDesc *effect = new EffectDesc(*effects[i]);   // deep copy
         loadEffectParameters(node, effect->mParams);
         ALOGV("loadInputSource() adding effect %s uuid %08x", effect->mName, effect->mUuid.timeLow);
         source->mEffects.add(effect);
@@ -1294,11 +1282,7 @@ AudioPolicyService::EffectDesc *AudioPolicyService::loadEffect(cnode *root)
         ALOGW("loadEffect() invalid uuid %s", node->value);
         return NULL;
     }
-    EffectDesc *effect = new EffectDesc();
-    effect->mName = strdup(root->name);
-    memcpy(&effect->mUuid, &uuid, sizeof(effect_uuid_t));
-
-    return effect;
+    return new EffectDesc(root->name, uuid);
 }
 
 status_t AudioPolicyService::loadEffects(cnode *root, Vector <EffectDesc *>& effects)
diff --git a/services/audioflinger/AudioPolicyService.h b/services/audioflinger/AudioPolicyService.h
index fdaf57644e40..679fd30df02a 100644
--- a/services/audioflinger/AudioPolicyService.h
+++ b/services/audioflinger/AudioPolicyService.h
@@ -233,8 +233,33 @@ private:
 
     class EffectDesc {
     public:
-        EffectDesc() {}
-        virtual ~EffectDesc() {}
+        EffectDesc(const char *name, const effect_uuid_t& uuid) :
+                        mName(strdup(name)),
+                        mUuid(uuid) { }
+        EffectDesc(const EffectDesc& orig) :
+                        mName(strdup(orig.mName)),
+                        mUuid(orig.mUuid) {
+                            // deep copy mParams
+                            for (size_t k = 0; k < orig.mParams.size(); k++) {
+                                effect_param_t *origParam = orig.mParams[k];
+                                // psize and vsize are rounded up to an int boundary for allocation
+                                size_t origSize = sizeof(effect_param_t) +
+                                                  ((origParam->psize + 3) & ~3) +
+                                                  ((origParam->vsize + 3) & ~3);
+                                effect_param_t *dupParam = (effect_param_t *) malloc(origSize);
+                                memcpy(dupParam, origParam, origSize);
+                                // This works because the param buffer allocation is also done by
+                                // multiples of 4 bytes originally. In theory we should memcpy only
+                                // the actual param size, that is without rounding vsize.
+                                mParams.add(dupParam);
+                            }
+                        }
+        /*virtual*/ ~EffectDesc() {
+            free(mName);
+            for (size_t k = 0; k < mParams.size(); k++) {
+                free(mParams[k]);
+            }
+        }
         char *mName;
         effect_uuid_t mUuid;
         Vector <effect_param_t *> mParams;
@@ -243,7 +268,11 @@ private:
     class InputSourceDesc {
     public:
         InputSourceDesc() {}
-        virtual ~InputSourceDesc() {}
+        /*virtual*/ ~InputSourceDesc() {
+            for (size_t j = 0; j < mEffects.size(); j++) {
+                delete mEffects[j];
+            }
+        }
         Vector <EffectDesc *> mEffects;
     };