1

I have a doubly threaded linked list made up of winery elements, winery being a class with its own data fields. The problem that I appear to be having is with the cstring data fields in my winery element. Now I was receiving a "double free or corruption (out): 0x0000000000402940" error in Linux, so I disabled my winery element destructor:

winery::~winery()
{
    delete[] name;
    delete[] location;
}

and that fixed the error, in Linux. Although, I can't see anything wrong with my winery deconstructor. My Linux environment will just output the values of name * and location *, while Windows Visual Studio outputs the values of name * and location * followed by: ²²²²½½½½½½½½ϵ▮ϵ▮ϵ▮.

In order to keep this post short, I'm just going to include the functions responsible for taking in data for name * and the functions for printing name *. Here's a dropbox link to a zip of my entire project if you really want to run it.

So, for example, I'm not actually making this call:

char name[] = "Winery\0\0\0\0\0\0";
winery item;
item.setName(name);

Just assume that name[] is a character array with more memory slots than it needs.

Here's my setName() function from my winery.cpp file:

void winery::setName(const char name[])
{
    unsigned char count;
    for (count = 0; name[count] != '\0'; count++);
    this->name = new char[count];
    for (unsigned char i = 0; name[i] != '\0'; i++)
        this->name[i] = name[i];
}

Now I have an addWinery() function in my list.cpp file which is part of my list class, but I'm not going to include that because I don't think it's causing the issue.

For couting my values I overloaded the << operator:

std::ostream& operator<< (std::ostream& out, const char array[])
{
    for (unsigned char i = 0; array[i] != '\0'; i++)
    {
        out << array[i];
    }
    return out;
}

std::cout << "Name: " << head->item.getName() << std::endl; Prints this in Linux

Name: Winery

and this in Visual Studio:

Name: Winery²²²²½½½½½½½½ϵ▮ϵ▮ϵ▮

Now, I'm guessing this error is caused by my winery::setName() & winery::setLocation() functions although I can't see anything wrong with these two functions. I could be wrong about that.

Again, you guys may find this easier to test for yourselves, so here's a dropbox link.

If you could also explain what is wrong with my ~winery() deconstructor or why I don't need it for these dynamically allocated arrays (C-Style Strings) I would be so grateful.

Logan Kling
  • 569
  • 2
  • 6
  • 19
  • 2
    And the elephant in the room... Why a dynamic array? And why not a string? – user4581301 Oct 09 '15 at 23:41
  • http://stackoverflow.com/help/mcve – Benjamin Lindley Oct 09 '15 at 23:43
  • 1
    @user4581301 XD, I'm still learning C++. This is for a class which I'm not allowed to use the string library. If I was anything but a beginner & having problems with pointers, I would be ashamed of myself. – Logan Kling Oct 10 '15 at 00:02
  • And if you must hand-roll this, change `name` to `char *`; change `setName()` to `this->name = strdup(name);`, and change the destructor to `free(name);`. Ditto for `location`. But using a `std::string` would be much more to the point. Then you wouldn't need a destructor at all. – user207421 Oct 10 '15 at 00:11

1 Answers1

3

winery::setName() doesn't null-terminate the string.

keithmo
  • 4,893
  • 1
  • 21
  • 17
  • 1
    But wouldn't `count` be high enough so that it creates `name *` with one extra slot for a `\0` character at the end? – Logan Kling Oct 09 '15 at 23:32
  • No -- you're counting the number of characters before the null. You'd be much better off doing something like `this->name = new char[strlen(name) + 1]; strcpy(this->name, name);`. – keithmo Oct 09 '15 at 23:37
  • @LarryK: No, your `count` ends up being too short by 1 and your copy loop also stops before copying the NUL terminator. One last thing, stop using `unsigned char` for `count`. That is a terrible bug if your string is longer than 255 characters. – Blastfurnace Oct 09 '15 at 23:40
  • 1
    @Blastfurnace Do you know why this was working in Linux? Do you know why I don't need the deconstructor? Or do I? Also, the array I'm passing into the function is 100 characters and I've set up my main execution file so that it handles an overflow error. – Logan Kling Oct 09 '15 at 23:43
  • It worked under Linux because you got lucky (for the moment) and the heap block just happened to have zeros just after the string. – keithmo Oct 09 '15 at 23:45
  • @LarryK: I suspect it only failed to crash in an obvious manner on Linux. Off by one errors and missing terminators can lurk without causing an immediate crash. You definitely need a destructor if your class owns a resource like allocated memory. You probably need a copy constructor and copy assignment operator as well... – Blastfurnace Oct 09 '15 at 23:46
  • 2
    @keithmo Better way to put it is he got unlucky. Crash on error is one of the best things that can happen. – user4581301 Oct 09 '15 at 23:50
  • 1
    If I got lucky/unlucky in Linux (and the last value in the array was `\0` like it was supposed to be), then why did the deconstructor throw a double free or corruption error? – Logan Kling Oct 09 '15 at 23:51
  • 1
    Is there a `do while` equivalent of a `for` loop? – Logan Kling Oct 09 '15 at 23:53
  • @user4581301 -- Good point. Better to crash early and obviously! LarryK -- That's hard to answer without seeing the rest of the code. – keithmo Oct 09 '15 at 23:55
  • 1
    @LarryK: A double free is likely due to you copying a winery object. You need to read about resource management so you don't end up with multiple objects holding pointers to the same memory. [Do you have a good C++ book?](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) – Blastfurnace Oct 09 '15 at 23:56
  • 2
    @LarryK: Your error with the destructor is likely unrelated to the error with the null-terminator. I suspect broken value semantics (i.e. improperly implemented copy constructor and/or assignment operator) – Benjamin Lindley Oct 09 '15 at 23:56
  • 1
    You should make you answer more specific. State that I need to place this comand: `this->name[count] = '\0';` after `this->name = new char[count];`. I've marked your answer as correct, though. – Logan Kling Oct 09 '15 at 23:59
  • 1
    No, that's not the correct fix; this will lead to more heap corruption. Instead, after you count the number of characters, increment count one more time to account for the terminator. This will make `new` allocate the correct size, and then your next loop will include the terminator while it's copying. – keithmo Oct 10 '15 at 00:08
  • 1
    It worked for me. I'm pretty sure `count`'s the right number and `i` is too, just not until the `for loop`'s terminating condition is true. The difference is that `count` exists outside of the loop. – Logan Kling Oct 10 '15 at 00:10
  • @LarryK: If your loop finds a NUL at `name[4]` and stops with `count` equal to 4 you are off by one. You need to allocated __5__ bytes to hold the string plus NUL terminator. – Blastfurnace Oct 10 '15 at 00:19
  • 1
    @Blastfurnace The Debugger said `char` was equal to five when I put a breakpoint at `this->name = new char[count];`. I may be somewhat new to C++, but I do know that cstrings need an extra space for a null character at the end (`\0` in this case). – Logan Kling Oct 10 '15 at 00:21
  • 1
    @LarryK: The code in your question doesn't work that way. [Link to example on ideone.com](http://ideone.com/F0aFQS) – Blastfurnace Oct 10 '15 at 00:26
  • @LarryK Your code counts the number of characters before the trailing null. Consider if the string is empty: the loop never iterates, so count is zero. You're off by one. In any case `strlen()` already exists, as does `strdup()`. – user207421 Oct 10 '15 at 00:32
  • 1
    @Blastfurnace That example is perfect because the first `char` is stored at `[0]` so for a four character array, the last character would be stored at `[3]`, right? – Logan Kling Oct 10 '15 at 00:34
  • @LarryK: Yes, the letters "meow" occupy array indices 0,1,2,3. Note that the last index is 3 but the total count is 4. Don't forget index 4 for the NUL which gives a final length of 5. You need to allocated 5 bytes. – Blastfurnace Oct 10 '15 at 00:36
  • 1
    @Blastfurnace If that's the case, should I even run this command: `this->name[count] = '\0';` or should I just do this: `this->name[count+1] = NULL;`? – Logan Kling Oct 10 '15 at 00:38
  • @LarryK: If `count` is off by one then you need to allocate `count+1` bytes of memory and copy `count+1` bytes from the old string to the new string (that should copy the NUL as well). I'll leave that as an exercise for you. – Blastfurnace Oct 10 '15 at 00:42
  • 1
    @Blastfurnace I am so sorry. I though `array[1]` would initialize an array with a `[0]` slot and a `[1]` slot. – Logan Kling Oct 10 '15 at 00:51
  • @LarryK: That's a common mistake, we humans don't start counting from `0` but you'll get used to it. – Blastfurnace Oct 10 '15 at 00:52