Merge "Fixed possible heap corruption in EffectDesc"

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)

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;
     };