malloc: *** error for object 0x7fff5fbff350: pointer being freed was not allocated

Question

I have seen many pages for this error, but I'm not able to grasp what I'm doing wrong with my code, so I was hoping if I posted it, someone could shed some light.

This is a c++ class project. The goal is to write a function that takes two C strings (char*) and returns them concatenated together in a new. What I have here compiles, except for when I try to delete copycat, I get the malloc error in the title of this question.

How can I delete copycat?

I think I should be also deleting the "unused" news I created in the cat2 function (p, q), but that gives me the same error. What am I missing here with deallocating memory?

char* cat2(char* dest1, char* str2)
{
    char* p = new char[100];
    char* q = new char[100];
    char* rvalue = new char[100];

    for (p = dest1; *p != 0; p++)
    {
        ;
    }


    for (q = str2; *q != 0; p++, q++ )
    {
        *p = *q;
    }

    *p = 0; /* set the last character to 0 */

    rvalue = dest1;

    return rvalue;
}

void main()
{
    char s1[] = "Hello";
    char s2[] = ", World!";

    char* copycat = cat2(s1, s2);
    cout << copycat; 

    delete copycat;
}

This is a c++ class project. Looks like mostly C and very little C++. — PaulMcKenzie
Yikes, memory leak central, and all those pointers, and delete instead of delete[]! Are you really being taught to write C++ in this manner? — Lightness Races in Orbit
It's true! I'm in the into class and it's mostly presented as if everyone is familiar with C (which I'm not). The other part of this project is to use vectors to do the same thing, which from how it's described seems much simpler than what I have here. — Tim Elhajj
Nobody should write code like this any more. This leaks memory and will break if the strings are more than 99 characters. If this is what your teacher expects you to do, find a new teacher. In any case, the problem is this line: rvalue = dest1 You therefore try to delete what comes from s2 in main. If you made your parameters as follows: (const char* dest1, const char* str2) the bad code would not have compiled. — Neil Kirk
@ Lightness Races in Orbit. I think you're saying to use delete[] instead of delete? But that gives the same error. — Tim Elhajj

M.M M.M · Accepted Answer · 2015-02-20T02:19:17

rvalue = dest1; makes rvalue point to what dest1 is pointing to. This is the local buffer s1 inside main.

So calling delete copycat; is the same as trying delete s1; which fails because it was not allocated via new. (and because it should be delete[]).

You make the same mistake with p = dest1; and q = str2;. These make p and q point to those buffers. You leaked all the memory you allocated via new, and caused a buffer overflow by writing characters past the end of s1.

I guess you intended to copy characters into the buffers you allocated via new. However, p and q buffers are useless anyway; you should be copying characters into the buffer pointed to by rvalue. To do that you will have to work with *rvalue and so on (which means: the location being pointed to by rvalue, instead of making rvalue point somewhere totally different and leaking memory).

malloc: *** error for object 0x7fff5fbff350: pointer being freed was not allocated

1 Answers