Skip to content

Commit

Permalink
Fix memory leak and a serious bug (#23)
Browse files Browse the repository at this point in the history
User "tycho-kirchner" discovered a memory bug (issue-22) that was caused
when the same key is inserted multiple times into OrderedMap. This was
a minor oversight that caused a major bug in OrderedMaps's
functionality. When inserting an existing key in the map, I delete the
corresponding entry in the linked list using the iterator stored, and
re-insert the key to the end of the linked list. However, I did not add
back this new iterator and value into the hashmap (OMHash). This meant
that the the key was holding the old value and an invalid iterator.
Because the old iterator pointed to an entry in linked list which was
deleted/free'd, it resulted in a memory leak (valgrind would show this
memory as unreachable and hence "leaking").

Fix was to add the new value-iterator pair into OMHash for the key being
inserted.

Updated unit tests to test for this condition.
  • Loading branch information
mandeepsandhu authored Sep 4, 2019
1 parent 3222704 commit e1a26e3
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 0 deletions.
1 change: 1 addition & 0 deletions src/orderedmap.h
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ typename OrderedMap<Key, Value>::iterator OrderedMap<Key, Value>::insert(const K
QllIterator ioIter = insertOrder.insert(insertOrder.end(), key);
pair.first = value;
pair.second = ioIter;
data.insert(key, pair);
return iterator(ioIter, &data);
}

Expand Down
19 changes: 19 additions & 0 deletions tests/functional/testorderedmap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ private slots:
void takeTest();
void valueTest();
void valuesTest();
void singleKeyMultipleValuesTest();
void copyConstructorTest();
void opAssignTest();
#if (QT_VERSION >= 0x050200)
Expand Down Expand Up @@ -180,6 +181,14 @@ void TestOrderedMap::valueTest()
QVERIFY(om.value(1) == 1);
QVERIFY(om.value(0) == 0);
QVERIFY(om.value(2) == 2);

// overwrite with different values
om.insert(0, 10);
om.insert(2, 20);

QVERIFY(om.value(1) == 1);
QVERIFY(om.value(0) == 10);
QVERIFY(om.value(2) == 20);
}

void TestOrderedMap::valuesTest()
Expand All @@ -204,6 +213,16 @@ void TestOrderedMap::valuesTest()
}
}

void TestOrderedMap::singleKeyMultipleValuesTest()
{
OrderedMap<int, int> om;
for (int i=0; i<10; i++) {
om.insert(1, i);
}
// only last value is retained
QVERIFY(om.value(1) == 9);
}

void TestOrderedMap::copyConstructorTest()
{
OrderedMap<int, int> om1;
Expand Down

0 comments on commit e1a26e3

Please sign in to comment.