Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit afe9833d authored by Glenn Kasten's avatar Glenn Kasten
Browse files

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
parent 9efedcbb

services/audioflinger/AudioPolicyService.cpp

+3 −19
Original line number Diff line number Diff line
@@ -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

+32 −3
Original line number Diff line number Diff line
@@ -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;

    };