Description:
In writing a storage engine with the 5.1.22-rc version, I tried using the String class defined in sql_string.h to return an object from a called method to the caller. This code fails as a result of heap corruption. The essential problem is that the copy constructor and the assignment operator fail to make a copy of the string owned by the source object when that string is in allocated memory. Of course, the copy constructor and assignment operator are frequently used in C++ as shown in the code fragment below to return an object itself from a called function/method to the caller. When the String copy constructor or assignment operator is passed a source object that has allocated the string memory, they need to allocate a new cell and place a pointer to that cell in the created/assigned object. Otherwise the new String object points to a piece of memory that is immediately deallocated at the end of the copy or assignment operation.
Obviously a workaround is to point only to memory allocated with thd_alloc, so it is not owned by the source String object.
How to repeat:
A sample code fragment looks something like this:
static String computeFilename () {
String s(10);
s.append("xyzzy");
return s;
}
...
String y = computeFilename();
...
Suggested fix:
The original code of the Copy constructor:
String(const String &str)
{
Ptr=str.Ptr ; str_length=str.str_length ;
Alloced_length=str.Alloced_length; alloced=0;
str_charset=str.str_charset;
}
should be replaced with:
String(const String &str)
{
alloced=0;
str_length=str.str_length ;
if (str.alloced && str.str_length) {
Ptr=0;
Alloced_length=0;
(void)copy(str.Ptr,str.str_length,str.str_charset);
} else {
Ptr=str.Ptr;
Alloced_length=str.Alloced_length;
str_charset=str.str_charset;
}
}
Similarly, the original code of the assignment operator:
inline String& operator = (const String &s)
{
if (&s != this)
{
/*
It is forbidden to do assignments like
some_string = substring_of_that_string
*/
DBUG_ASSERT(!s.uses_buffer_owned_by(this));
free();
Ptr=s.Ptr ; str_length=s.str_length ; Alloced_length=s.Alloced_length;
alloced=0;
}
return *this;
}
should be replaced with this:
inline String& operator = (const String &s)
{
if (&s != this)
{
/*
It is forbidden to do assignments like
some_string = substring_of_that_string
*/
DBUG_ASSERT(!s.uses_buffer_owned_by(this));
free();
alloced=0;
str_length=s.str_length;
if (s.alloced && s.str_length) {
Ptr=0;
Alloced_length=0;
(void)copy(s.Ptr,s.str_length,s.str_charset);
} else {
Ptr=s.Ptr;
Alloced_length=s.Alloced_length;
str_charset=s.str_charset;
}
}
return *this;
}
Of course, a more sophisticated solution could be implemented that involved keeping the strings in reference counted memory, so that the copy/assignment operators could point to the same memory and increment the use count, and the free() operator would decrement the use count and release the memory only when the count reaches zero. The quick fix above has the anomalous behavior of returning a String pointing to zero if the memory allocation in copy() fails.
Description: In writing a storage engine with the 5.1.22-rc version, I tried using the String class defined in sql_string.h to return an object from a called method to the caller. This code fails as a result of heap corruption. The essential problem is that the copy constructor and the assignment operator fail to make a copy of the string owned by the source object when that string is in allocated memory. Of course, the copy constructor and assignment operator are frequently used in C++ as shown in the code fragment below to return an object itself from a called function/method to the caller. When the String copy constructor or assignment operator is passed a source object that has allocated the string memory, they need to allocate a new cell and place a pointer to that cell in the created/assigned object. Otherwise the new String object points to a piece of memory that is immediately deallocated at the end of the copy or assignment operation. Obviously a workaround is to point only to memory allocated with thd_alloc, so it is not owned by the source String object. How to repeat: A sample code fragment looks something like this: static String computeFilename () { String s(10); s.append("xyzzy"); return s; } ... String y = computeFilename(); ... Suggested fix: The original code of the Copy constructor: String(const String &str) { Ptr=str.Ptr ; str_length=str.str_length ; Alloced_length=str.Alloced_length; alloced=0; str_charset=str.str_charset; } should be replaced with: String(const String &str) { alloced=0; str_length=str.str_length ; if (str.alloced && str.str_length) { Ptr=0; Alloced_length=0; (void)copy(str.Ptr,str.str_length,str.str_charset); } else { Ptr=str.Ptr; Alloced_length=str.Alloced_length; str_charset=str.str_charset; } } Similarly, the original code of the assignment operator: inline String& operator = (const String &s) { if (&s != this) { /* It is forbidden to do assignments like some_string = substring_of_that_string */ DBUG_ASSERT(!s.uses_buffer_owned_by(this)); free(); Ptr=s.Ptr ; str_length=s.str_length ; Alloced_length=s.Alloced_length; alloced=0; } return *this; } should be replaced with this: inline String& operator = (const String &s) { if (&s != this) { /* It is forbidden to do assignments like some_string = substring_of_that_string */ DBUG_ASSERT(!s.uses_buffer_owned_by(this)); free(); alloced=0; str_length=s.str_length; if (s.alloced && s.str_length) { Ptr=0; Alloced_length=0; (void)copy(s.Ptr,s.str_length,s.str_charset); } else { Ptr=s.Ptr; Alloced_length=s.Alloced_length; str_charset=s.str_charset; } } return *this; } Of course, a more sophisticated solution could be implemented that involved keeping the strings in reference counted memory, so that the copy/assignment operators could point to the same memory and increment the use count, and the free() operator would decrement the use count and release the memory only when the count reaches zero. The quick fix above has the anomalous behavior of returning a String pointing to zero if the memory allocation in copy() fails.