Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

fix potential memory leak in CVariant

  • Loading branch information...
commit bfd5673a698d2f80f6d32e3b5efdf47737d3260f 1 parent 31113d1
Sascha Montellese Montellese authored
Showing with 57 additions and 79 deletions.
  1. +54 −74 xbmc/utils/Variant.cpp
  2. +3 −5 xbmc/utils/Variant.h
128 xbmc/utils/Variant.cpp
View
@@ -41,18 +41,12 @@ CVariant::CVariant(VariantType type)
case VariantTypeBoolean:
m_data.boolean = false;
break;
- case VariantTypeString:
- m_data.string = new string();
- break;
case VariantTypeDouble:
m_data.dvalue = 0.0;
break;
+ case VariantTypeString:
case VariantTypeArray:
- m_data.array = new VariantArray();
- break;
case VariantTypeObject:
- m_data.map = new VariantMap();
- break;
default:
memset(&m_data, 0, sizeof(m_data));
break;
@@ -104,19 +98,19 @@ CVariant::CVariant(bool boolean)
CVariant::CVariant(const char *str)
{
m_type = VariantTypeString;
- m_data.string = new string(str);
+ m_string = str;
}
CVariant::CVariant(const char *str, unsigned int length)
{
m_type = VariantTypeString;
- m_data.string = new string(str, length);
+ m_string = string(str, length);
}
CVariant::CVariant(const string &str)
{
m_type = VariantTypeString;
- m_data.string = new string(str);
+ m_string = str;
}
CVariant::CVariant(const CVariant &variant)
@@ -125,25 +119,6 @@ CVariant::CVariant(const CVariant &variant)
*this = variant;
}
-CVariant::~CVariant()
-{
- if (m_type == VariantTypeString && m_data.string)
- {
- delete m_data.string;
- m_data.string = NULL;
- }
- else if (m_type == VariantTypeArray && m_data.array)
- {
- delete m_data.array;
- m_data.array = NULL;
- }
- else if (m_type == VariantTypeObject && m_data.map)
- {
- delete m_data.map;
- m_data.map = NULL;
- }
-}
-
bool CVariant::isInteger() const
{
return m_type == VariantTypeInteger;
@@ -270,7 +245,7 @@ bool CVariant::asBoolean(bool fallback) const
case VariantTypeDouble:
return (bool)m_data.dvalue;
case VariantTypeString:
- if (m_data.string->empty() || m_data.string->compare("0") || m_data.string->compare("false"))
+ if (m_string.empty() || m_string.compare("0") == 0 || m_string.compare("false") == 0)
return false;
return true;
default:
@@ -285,7 +260,7 @@ std::string CVariant::asString(const std::string &fallback /* = "" */) const
switch (m_type)
{
case VariantTypeString:
- return *(m_data.string);
+ return m_string;
case VariantTypeBoolean:
return m_data.boolean ? "true" : "false";
case VariantTypeInteger:
@@ -313,19 +288,19 @@ CVariant &CVariant::operator[](const std::string &key)
if (m_type == VariantTypeNull)
{
m_type = VariantTypeObject;
- m_data.map = new VariantMap();
+ m_map.clear();
}
if (m_type == VariantTypeObject)
- return (*m_data.map)[key];
+ return m_map[key];
else
return ConstNullVariant;
}
const CVariant &CVariant::operator[](const std::string &key) const
{
- if (m_type == VariantTypeObject)
- return (*m_data.map)[key];
+ if (m_type == VariantTypeObject && isMember(key))
+ return m_map.at(key);
davilla Collaborator
davilla added a note

m_map.at(key) fails on osx

/Users/Shared/xbmc/xbmc-git/xbmc/utils/Variant.cpp:303:0 /Users/Shared/xbmc/xbmc-git/xbmc/utils/Variant.cpp:303: error: 'const class std::map, std::allocator > >' has no member named 'at'

Sascha Montellese Owner

Sorry about that. I knew that it wasn't part of the STL but as win32 and linux supported it I figured OSX would as well. In that case I need to drop the const version of operator[] as [] can not be used on a const std::map.

I'm on my phone right now so will fix when I arrive at the university.

davilla Collaborator
davilla added a note

k, osx still uses gcc-4.0, that will change next cycle when we can bump to 10.6sdk.

davilla Collaborator
davilla added a note

just an update, this also broke my natty build.
edit: my bad, ccache issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
else
return ConstNullVariant;
}
@@ -333,7 +308,7 @@ const CVariant &CVariant::operator[](const std::string &key) const
CVariant &CVariant::operator[](unsigned int position)
{
if (m_type == VariantTypeArray && size() > position)
- return (*m_data.array)[position];
+ return m_array[position];
else
return ConstNullVariant;
}
@@ -341,7 +316,7 @@ CVariant &CVariant::operator[](unsigned int position)
const CVariant &CVariant::operator[](unsigned int position) const
{
if (m_type == VariantTypeArray && size() > position)
- return (*m_data.array)[position];
+ return m_array.at(position);
else
return ConstNullVariant;
}
@@ -368,13 +343,14 @@ CVariant &CVariant::operator=(const CVariant &rhs)
m_data.dvalue = rhs.m_data.dvalue;
break;
case VariantTypeString:
- m_data.string = new string(rhs.m_data.string->c_str());
+ m_string = rhs.m_string;
break;
case VariantTypeArray:
- m_data.array = new VariantArray(rhs.m_data.array->begin(), rhs.m_data.array->end());
+ m_array.assign(rhs.m_array.begin(), rhs.m_array.end());
break;
case VariantTypeObject:
- m_data.map = new VariantMap(rhs.m_data.map->begin(), rhs.m_data.map->end());
+ m_map.clear();
+ m_map.insert(rhs.m_map.begin(), rhs.m_map.end());
break;
default:
break;
@@ -391,25 +367,18 @@ bool CVariant::operator==(const CVariant &rhs) const
{
case VariantTypeInteger:
return m_data.integer == rhs.m_data.integer;
- break;
case VariantTypeUnsignedInteger:
return m_data.unsignedinteger == rhs.m_data.unsignedinteger;
- break;
case VariantTypeBoolean:
return m_data.boolean == rhs.m_data.boolean;
- break;
case VariantTypeDouble:
return m_data.dvalue == rhs.m_data.dvalue;
- break;
case VariantTypeString:
- return (*m_data.string) == (*rhs.m_data.string);
- break;
+ return m_string == rhs.m_string;
case VariantTypeArray:
- return (*m_data.array) == (*rhs.m_data.array);
- break;
+ return m_array == rhs.m_array;
case VariantTypeObject:
- return (*m_data.map) == (*rhs.m_data.map);
- break;
+ return m_map == rhs.m_map;
default:
break;
}
@@ -423,11 +392,11 @@ void CVariant::push_back(const CVariant &variant)
if (m_type == VariantTypeNull)
{
m_type = VariantTypeArray;
- m_data.array = new VariantArray();
+ m_array.clear();
}
if (m_type == VariantTypeArray)
- m_data.array->push_back(variant);
+ m_array.push_back(variant);
}
void CVariant::append(const CVariant &variant)
@@ -438,7 +407,7 @@ void CVariant::append(const CVariant &variant)
const char *CVariant::c_str() const
{
if (m_type == VariantTypeString)
- return m_data.string->c_str();
+ return m_string.c_str();
else
return NULL;
}
@@ -447,18 +416,27 @@ void CVariant::swap(CVariant &rhs)
{
VariantType temp_type = m_type;
VariantUnion temp_data = m_data;
+ string temp_string = m_string;
+ VariantArray temp_array = m_array;
+ VariantMap temp_map = m_map;
m_type = rhs.m_type;
m_data = rhs.m_data;
+ m_string = rhs.m_string;
+ m_array = rhs.m_array;
+ m_map = rhs.m_map;
rhs.m_type = temp_type;
rhs.m_data = temp_data;
+ rhs.m_string = temp_string;
+ rhs.m_array = temp_array;
+ rhs.m_map = temp_map;
}
CVariant::iterator_array CVariant::begin_array()
{
if (m_type == VariantTypeArray)
- return m_data.array->begin();
+ return m_array.begin();
else
return iterator_array();
}
@@ -466,7 +444,7 @@ CVariant::iterator_array CVariant::begin_array()
CVariant::const_iterator_array CVariant::begin_array() const
{
if (m_type == VariantTypeArray)
- return m_data.array->begin();
+ return m_array.begin();
else
return const_iterator_array();
}
@@ -474,7 +452,7 @@ CVariant::const_iterator_array CVariant::begin_array() const
CVariant::iterator_array CVariant::end_array()
{
if (m_type == VariantTypeArray)
- return m_data.array->end();
+ return m_array.end();
else
return iterator_array();
}
@@ -482,7 +460,7 @@ CVariant::iterator_array CVariant::end_array()
CVariant::const_iterator_array CVariant::end_array() const
{
if (m_type == VariantTypeArray)
- return m_data.array->end();
+ return m_array.end();
else
return const_iterator_array();
}
@@ -490,7 +468,7 @@ CVariant::const_iterator_array CVariant::end_array() const
CVariant::iterator_map CVariant::begin_map()
{
if (m_type == VariantTypeObject)
- return m_data.map->begin();
+ return m_map.begin();
else
return iterator_map();
}
@@ -498,7 +476,7 @@ CVariant::iterator_map CVariant::begin_map()
CVariant::const_iterator_map CVariant::begin_map() const
{
if (m_type == VariantTypeObject)
- return m_data.map->begin();
+ return m_map.begin();
else
return const_iterator_map();
}
@@ -506,7 +484,7 @@ CVariant::const_iterator_map CVariant::begin_map() const
CVariant::iterator_map CVariant::end_map()
{
if (m_type == VariantTypeObject)
- return m_data.map->end();
+ return m_map.end();
else
return iterator_map();
}
@@ -514,7 +492,7 @@ CVariant::iterator_map CVariant::end_map()
CVariant::const_iterator_map CVariant::end_map() const
{
if (m_type == VariantTypeObject)
- return m_data.map->end();
+ return m_map.end();
else
return const_iterator_map();
}
@@ -522,11 +500,11 @@ CVariant::const_iterator_map CVariant::end_map() const
unsigned int CVariant::size() const
{
if (m_type == VariantTypeObject)
- return m_data.map->size();
+ return m_map.size();
else if (m_type == VariantTypeArray)
- return m_data.array->size();
+ return m_array.size();
else if (m_type == VariantTypeString)
- return m_data.string->size();
+ return m_string.size();
else
return 0;
}
@@ -534,11 +512,11 @@ unsigned int CVariant::size() const
bool CVariant::empty() const
{
if (m_type == VariantTypeObject)
- return m_data.map->empty();
+ return m_map.empty();
else if (m_type == VariantTypeArray)
- return m_data.array->empty();
+ return m_array.empty();
else if (m_type == VariantTypeString)
- return m_data.string->empty();
+ return m_string.empty();
else
return true;
}
@@ -546,9 +524,11 @@ bool CVariant::empty() const
void CVariant::clear()
{
if (m_type == VariantTypeObject)
- m_data.map->clear();
+ m_map.clear();
else if (m_type == VariantTypeArray)
- m_data.array->clear();
+ m_array.clear();
+ else if (m_type == VariantTypeString)
+ m_string.clear();
}
void CVariant::erase(const std::string &key)
@@ -556,10 +536,10 @@ void CVariant::erase(const std::string &key)
if (m_type == VariantTypeNull)
{
m_type = VariantTypeObject;
- m_data.map = new VariantMap();
+ m_map.clear();
}
else if (m_type == VariantTypeObject)
- m_data.map->erase(key);
+ m_map.erase(key);
}
void CVariant::erase(unsigned int position)
@@ -567,17 +547,17 @@ void CVariant::erase(unsigned int position)
if (m_type == VariantTypeNull)
{
m_type = VariantTypeArray;
- m_data.array = new VariantArray();
+ m_array.clear();
}
if (m_type == VariantTypeArray && position < size())
- m_data.array->erase(m_data.array->begin() + position);
+ m_array.erase(m_array.begin() + position);
}
bool CVariant::isMember(const std::string &key) const
{
if (m_type == VariantTypeObject)
- return m_data.map->find(key) != m_data.map->end();
+ return m_map.find(key) != m_map.end();
return false;
}
8 xbmc/utils/Variant.h
View
@@ -53,8 +53,6 @@ class CVariant
CVariant(const std::string &str);
CVariant(const CVariant &variant);
- ~CVariant();
-
bool isInteger() const;
bool isUnsignedInteger() const;
bool isBoolean() const;
@@ -124,13 +122,13 @@ class CVariant
uint64_t unsignedinteger;
bool boolean;
double dvalue;
- std::string *string;
- VariantArray *array;
- VariantMap *map;
};
VariantType m_type;
VariantUnion m_data;
+ std::string m_string;
+ VariantArray m_array;
+ VariantMap m_map;
static CVariant ConstNullVariant;
};

4 comments on commit bfd5673

davilla

m_map.at(key) fails on osx

/Users/Shared/xbmc/xbmc-git/xbmc/utils/Variant.cpp:303:0 /Users/Shared/xbmc/xbmc-git/xbmc/utils/Variant.cpp:303: error: 'const class std::map, std::allocator > >' has no member named 'at'

Sascha Montellese

Sorry about that. I knew that it wasn't part of the STL but as win32 and linux supported it I figured OSX would as well. In that case I need to drop the const version of operator[] as [] can not be used on a const std::map.

I'm on my phone right now so will fix when I arrive at the university.

davilla

k, osx still uses gcc-4.0, that will change next cycle when we can bump to 10.6sdk.

davilla

just an update, this also broke my natty build.
edit: my bad, ccache issue.

Tobias Arrskog
Collaborator

Sorry for bringing this back up but what potential memory leak is fixed? Problem with the solution proposed here is that it adds at the very least 96bytes (on 32bit system) and most likely quite a bit more for each Variant in use. While this may be rather extreme to care I bring it up as CVariant was designed to be close to no overhead compared to using the explicit type. Before change the instance only ever used 64bits (if we assume it was no pointer), this made it quick to copy on the very basic types (bool, double, int) while obviously the more complex types needed larger copy. Now CVariant i(int) needs atleast 160bytes, which is quite much more :)

jmarshallnz

My guess is that it handles the case where an array or map type CVariant is assigned to some other CVariant - any data it contained previously is not free'd.

Sascha Montellese
Owner

There was a memory leak in the = operator and m_data.string which before was a pointer and the pointer was simply overwritten by the m_data.string pointer of the assigned value. The same applied to m_data.array and m_data.map. So we lost at least 4 bytes everytime a CVariant was assigned using the = operator.

This solution is based on a proposal/discussion with spiff.

Tobias Arrskog
Collaborator

A, yeah that would leak, did not think of that. I'll see if I can find some time to fix the operator= instead as that would be a bit better solution IMO. Unless someone beats me to it :)

Please sign in to comment.
Something went wrong with that request. Please try again.