why string become invalid in memory after realloc?

Question

I have wrote class myString which override some operators. The result is wrong if it run in vs2013(Microsoft Virtual Studio 2013). But the result is right if it run in devcpp. why?

By the way, I have debug, and find the problem is realloc. The data which was existed in memory become invalid After realloc in vs2013.

all my code:

#include <iostream>
#include<string>
#include <stdlib.h>
#include <string.h>
using namespace std;


class myString{
public:
    myString() :_str(NULL), length(0){};
    myString(char* str);

    friend myString& operator+(myString& str,char* s);
    friend myString& operator+(myString& str, myString& s);
    friend myString& operator+(myString& str, int num);
    friend ostream& operator<<(ostream& out,myString& str);
    void operator=(char* s);
    void operator=(myString& s);
private:
    char* _str;
    int length;
};

myString::myString(char* str){
    if (str == NULL){
        _str = NULL;
        length = 0;
        return;
    }

    length = strlen(str);
    _str = (char*)malloc(sizeof(char)*length);
    strcpy(_str, str);
}

myString& operator+(myString& str, char* s){
    if (s == NULL)
        return str;

    char* temp = (char*)realloc((void*)str._str, sizeof(char)*strlen(s));
    if (temp == NULL){
        cout << "error" << endl;
        return str;
    }

    str.length += strlen(s);
    strcat(temp, s);
    str._str = temp;
    return str;
}

myString& operator+(myString& str, myString& s){
    if (s.length == 0 || s._str == NULL)
        return str;

    char* temp = (char*)realloc((void*)str._str, sizeof(char)*strlen(s._str));
    if (temp == NULL){
        cout << "error" << endl;
        return str;
    }

    str.length += strlen(s._str);
    strcat(temp, s._str);
    str._str = temp;
    return str;
}

myString& operator+(myString& str, int num){
    char* s = (char*)malloc(sizeof(char) * 100);
    itoa(num, s, 10);


    /******problem! in vs2013, after realloc str._str become invalid.
          But in devcpp everything is right. Why???******/
    char* temp = (char*)realloc((void*)str._str, sizeof(char)*strlen(s));
    if (temp == NULL){
        cout << "error" << endl;
        return str;
    }

    str.length += strlen(s);
    strcat(temp, s);
    str._str = temp;
    return str;
}

void myString::operator=(char* s){
    if (s == NULL)
        return;

    this->length += strlen(s);
    this->_str = (char*)malloc(sizeof(char)*strlen(s));
    strcpy(this->_str, s);
}

void myString::operator=(myString& s){
    this->length = s.length;
    this->_str = s._str;

}

ostream& operator<<(ostream& out, myString& str){
    out << "string is:" <<str._str<<", length is:"<<str.length;
    return out;
}

int main(){
    myString str;
    str = "hello world";
    cout << str << endl;
    str = str+ 123;
    cout << str << endl;
    system("pause");
    return 0;
}

You didn't initialize the buffer allocated in `myString::myString(char* str)`, so its contents is indeterminate and using the contents will invoke *undefined behavior*. Also don't forget to allocate space for terminating null-character. — MikeCAT, Jun 05 '16 at 16:11
Please construct a [minimal test-case](http://stackoverflow.com/help/mcve). — Oliver Charlesworth, Jun 05 '16 at 16:12
Besides that, the realloc() is completely wrong. If the existing string has three characters, and `char *s` is a 5-character string, the `realloc()` will allocate 5 characters for a combined string. Hello, buffer offerrun! Not to mention that 5 gets added to `str.length` not once, but twice. This is not one of the things that you want to do twice, just to make sure it gets done... This code will benefit from a [long conversation with a rubber duck](https://en.wikipedia.org/wiki/Rubber_duck_debugging). — Sam Varshavchik, Jun 05 '16 at 16:12
Why do your operators call `strlen()` instead of using `length`? Surely `char* temp = (char*)realloc((void*)str._str, sizeof(char)*strlen(s._str));` should be `realloc(str._str, length + s.length + 1)`? — kfsone, Jun 05 '16 at 17:42

score 1 · Accepted Answer · edited May 23 '17 at 12:08

You are missing a copy constructor for myString input, and a destructor. And your copy constructor for char* input is not copying the char data.

Your operator+ operators are implemented wrong. operator+ is not supposed to modify the input parameters, but you are. You need to return a new merged copy of the parameter data, which you are not doing.

Your operator= operators are implemented wrong, too. The char* operator is ignoring NULL input, leaving the target string unmodified instead of clearing it. And the same operator is incrementing length instead of re-assigning it. The myString operator is taking ownership of the input data instead of copying it, leaving two myString objects pointing at the same physical memory block. Both operators also have a memory leak, they are not freeing the existing data before replacing it with the new data. And they both need to return a reference to the myString that was modified.

Your operators should be utilizing the copy-and-swap idiom for safety.

Try something more like this instead:

#include <iostream>
#include <cstring>
#include <cstdlib>
#include <algorithm>

class myString
{
public:
  myString();
  myString(const myString &src);
  myString(const char* src);
  myString(const int src);
  ~myString();

  myString& operator=(const myString& rhs);
  myString& operator+=(const myString& rhs);

  friend myString operator+(const myString& lhs, const myString& rhs);
  friend std::ostream& operator<<(std::ostream& out, const myString& str);

private:
  char* _str;
  int length;
};

myString::myString()
  : _str(NULL), length(0)
{
}

myString::myString(const myString& src)
  : _str(NULL), length(0)
{
  if ((src._str != NULL) && (src.length > 0))
  {
    length = src.length;
    _str = new char[length + 1];
    std::copy(src._str, src._str + length, _str);
    _str[length] = 0;
  }
}

myString::myString(const char* src)
  : _str(NULL), length(0)
{
  if ((src != NULL) && (*src != 0))
  {
    length = std::strlen(src);
    _str = new char[length + 1];
    std::copy(src, src + length, _str);
    _str[length] = 0;
  }
}

myString::myString(const int src)
  : _str(NULL), length(0)
{
  char temp[16] = {0};
  std::itoa(src, temp, 10);
  length = std::strlen(temp);
  _str = new char[length + 1];
  std::copy(temp, &temp[length], _str);
  _str[length] = 0;
}

myString::~myString()
{
  delete[] _str;
}

myString& myString::operator=(const myString& rhs)
{
  myString temp(rhs);
  std::swap(_str, temp._str);
  std::swap(length, temp.length);
  return *this;
}

myString& myString::operator+=(const myString& rhs)
{
  myString temp;
  temp.length = length + rhs.length;
  temp._str = new char[temp.length + 1];
  std::copy(_str, _str + length, temp._str);
  std::copy(rhs._str, rhs._str + rhs.length, temp._str + length);
  temp._str[temp.length] = 0;
  std::swap(_str, temp._str);
  std::swap(length, temp.length);
  return *this;
}

myString operator+(const myString& lhs, const myString& rhs)
{
  myString temp;
  temp.length = lhs.length + rhs.length;
  temp._str = new char[temp.length + 1];
  std::copy(lhs._str, lhs._str + lhs.length, temp._str);
  std::copy(rhs._str, rhs._str + rhs.length, temp._str + lhs.length);
  temp._str[temp.length] = 0;
  return temp;
}

std::ostream& operator<<(std::ostream& out, const myString& str)
{
  out << "string is: " << str._str << ", length is: " << str.length;
  return out;
}

int main()
{
  myString str;
  str = "hello world";
  std::cout << str << std::endl;
  str = str + 123;
  std::cout << str << std::endl;
  std::system("pause");
  return 0;
}

I mean my parameters is a reference, a reference cannot be modify? — ranger lcy, Jun 06 '16 at 02:45
@rangerlcy: If a parameter is not `const` then it can be modified. But "*can be modified*" is a different issue than "*should be modified*". `operator+` *must not* modify its parameters, like your original code is doing. It must return a merged copy of the two input values, leaving the originals untouched. — Remy Lebeau, Jun 06 '16 at 03:31

score 0 · Answer 2 · answered Jun 05 '16 at 16:15

As I understand the action of realloc it may move the memory to a different place - specifically it may invalidate (deallocate) the old memory location if it needs more space.

After this the memory is no longer 'yours' and may change unpredictably. So it seems to me that VS2013 behavior is within the definition of what realloc does - it only guarantees the returned memory is valid (i.e your temp ) not the old memory addresses will remain valid.

tldr: realloc can destroy the old memory location; the fact that it works on some platforms as is; is a consequence of the implementation and won't be reliably under many memory conditions.

@rangerIcy Because are not managing the memory correctly at all. There are many bugs in your code that need fixing. — Remy Lebeau, Jun 05 '16 at 17:45

why string become invalid in memory after realloc?

2 Answers2