28960 - Memory leak in JS_HashTableAdd (jshash.c)

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect, P3)

Description

[Russell Pflughaupt]

The JS_HashTableAdd checks for hash table keys and data already being present.  
If this is found to be true, the function re-uses the existing key/data and 
returns to the caller.  This results in memory leaks because the caller won't 
free its just-passed-in key/data because they're assuming that JS_HashTableAdd 
is holding onto it.

Here is a modified version of the funciton that correctly handles adding a key 
to the hash table, even if that key is already present:

JS_EXPORT_API(JSHashEntry *)
JS_HashTableAdd(JSHashTable *ht, const void *key, void *value)
{
    JSHashNumber keyHash;
    JSHashEntry *he, **hep;

    keyHash = (*ht->keyHash)(key);
    hep = JS_HashTableRawLookup(ht, keyHash, key);
    if ((he = *hep) != NULL) {
#ifdef ORIGINAL_VERSION
        // I just don't understand how this code could possibly
        // work.  The caller to PL_HashTableAdd might have just
        // allocated the passed-in key and value, and then this
        // code checks for the key/value for already being in
        // the table AND DOESN'T INFORM THE CALLER OF THIS!
        // It looks like this code was pulled from an environment
        // that has garbage collection.
        //
        // The only right thing to do is free the old key/value
        // and install the new ones, even if they're identical.

        /* Hit; see if values match */
        if ((*ht->valueCompare)(he->value, value)) {
            /* key,value pair is already present in table */
            return he;
        }
        if (he->value)
            (*ht->allocOps->freeEntry)(ht->allocPriv, he, HT_FREE_VALUE);
        he->value = value;
        return he;
#else
        JS_HashTableRawRemove(ht, hep, he);
        hep = NULL;
#endif
    }
    return JS_HashTableRawAdd(ht, hep, keyHash, key, value);
}

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

confirming so that someone with mozilla-coding experience will have a look at
this...

Comment 2

[rogerl (gone)]

Thanks for the suggested patch - as it turns out, the JS engine is using a GC 
and so doesn't need to handle the situation you're addressing here. We've not 
intended the individual engine components to be used other than in the context 
of an executing engine, so we wouldn't need to support this case.