Closed
Bug 28960
Opened 25 years ago
Closed 25 years ago
Memory leak in JS_HashTableAdd (jshash.c)
Categories
(Core :: JavaScript Engine, defect, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: russell, Assigned: rogerl)
Details
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);
}
confirming so that someone with mozilla-coding experience will have a look at
this...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 2•25 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•