On 02/20/2018 05:48 AM, Nick Clifton wrote:
Hi Martin,
Since the class manages a resource it should ideally make sure it
doesn't try to release the same resource multiple times. I.e., its
copy constructor and assignment operators should either "do the right
thing" (whatever you think that is) or be made inaccessible (or declared
deleted in C++ 11).
For example:
{
escaped_string a;
a.escape ("foo\nbar");
escaped_string b (a);
// b destroys its m_str
// double free in a's destructor here
}
I am not sure that this can happen. First of the escaped_string
class does not have constructor that accepts a char* argument.
(Maybe in C++ this is done automatically ? My C++-fu is very weak).
It doesn't have a converting ctor and the above example doesn't
use one. It uses the implicitly defined copy constructor to
create a copy of A in B.
Secondly the m_owned private field is only set to true when
the m_str field is set to a string allocated by the particular
instance of the class, and memory is only freed by the destructor
if m_owned is true.
Right, and implicitly-defined copy constructor copies all the
fields so both the original and the copy wind up pointing to
the same allocated chunk and both with m_owned set to true.
The same thing happens when an object of the calss is assigned
to another, thanks to the implicitly-defined copy assignment
operator.
So even this should work:
{
escaped_string a,b;
a.escape ("foo\nbar");
b.escape ((const char *) a);
}
Yes, this works but this wouldn't:
{
escaped_string a, b;
a.escape ("foo\nbar");
b = a;
} // double free in a's dtor here
What also wouldn't work right is the converse:
{
escaped_string a, b;
a.escape ("foo\nbar");
a = b; // a.m_str leaks here (it's reset by the assignment)
}
Your patch works correctly because it doesn't use the copy
constructor or the copy assignment operator. So this is
only a hypothetical concern, if a change in the code led
to one of the copy functions being called.
Hope this makes sense.
Martin
The destructor for B will not free any memory, even though its
m_str field has been set to the same string as A, because its
m_owned field will be set to FALSE.
Cheers
Nick
