char array corruption in C++

Question

I'm working on a project that makes me store an array of objects whose constructor is

Item(char* item, int itemType){
char temp[200];
    for(int i = 0; i < 200; i++){
        temp[i] = '\0';
        if(item[i] != '\0'){
            temp[i] = item[i];
        }
    }
    _item = item;
    _itemType = itemType;
    _tweetIDs = NULL;
}

Don't worry about _tweetIDs, that's another functional part of my program and isn't related to my problem.

This array is stored within a class:

ItemList()

How this works is that the functional part of my program parses a line of input and puts it into the Item(char*, int) object. This is how it adds the line:

int addItem(char* item, int type){
    char temp1[200];
    for(int i = 0; i < 200; i++){
        temp1[i] = '\0';
    }
    int j = 0;
    while(item[j] != '\0'){
        temp1[j] = item[j];
        j++;
    }
    _items[_size] = Item(temp1, type);
    _size++;
    return _size;
}

Where _items is the Item() array and _size is a field that is incremented every time an Item() is added.

My issue comes when I have to print the contents of the list.

I have a method that does that:

void printList(){

    for(int i = 0; i < 500; i++){
        if(_items[i] != NULL){
            cout << "[" << i << "] ";
        _items[i]->printContents();
        }
    }
}

I tested printContents() in the constructor of Item() and tested printList in the addItem method and they both work when called within the class itself. The issue comes when I have to call the print method outside the class body.

In the main method, I create a List object:

List itemList;

The default constructor sets all members of the Item() array to NULL and initializes _size.

After adding a few Item() objects into the array (Which I confirmed is increasing in size through the debugger), I tried to print it out. When I call:

itemList.printList();

It gives me the right amount of indexes (And lines), but the char array is just a bunch of garbage. I used the debugger to try and find out where it went wrong. In the addItem() method, I called printList to check the array, and the output from that is fine. Then, I called itemList.printList() right after the last addItem() call, and it gave me garbage. In between the addItem() and itemList.printList(), the char array is lost or something along those lines.

Any idea what's going wrong? I'll give you any more code if you need it.

You probably wanted to write `memcpy(_item, item, 200);` and declare `_item` as `char _item[200]` in class declaration, so that it will live longer than the instance of constructor. — ruslik, Oct 10 '11 at 01:34
@ruslik I put the temp arrays into my class fields and it works great now. Thanks for the help! — SkylineAddict, Oct 10 '11 at 01:55
Please read [What are the rules about using an underscore in a C++ identifier?](http://stackoverflow.com/q/228783/2509). It will save you some surprises later on. — dmckee --- ex-moderator kitten, Oct 10 '11 at 02:45

Nate · Accepted Answer · 2011-10-10T01:45:23.923

2

In your Item constructor, you are setting what I presume is a member _item as such:

_item = item;

This just assigns the pointer value of the location pointed to by item into _item. It does not actually copy the string!

The next time you go to read this location, it might be valid - chances are, though, it will be garbage, as you are seeing.

What you are looking for is a function like strcpy (as a side note, there's no need to do quite so much manual copying - just pass that pointer around and copy it once - in the Item constructor).

EDIT, to address your comment:

strcpy made your program crash because you are using it on unallocated memory.

You have to allocate memory for an array using new[] in c++

edited Oct 10 '11 at 01:45

answered Oct 10 '11 at 01:37

Nate

12,499
5
45
60

strcpy made my program crash every time I used it. I also tried manually copying the char array, but that didn't help.' – SkylineAddict Oct 10 '11 at 01:41
You'll need to *allocate* the memory before you `strcpy()` into it. Why don't you use the `string` and `list` classes from the standard library? – johnsyweb Oct 10 '11 at 01:42
I'm only allowed to use iostream and cstring. – SkylineAddict Oct 10 '11 at 01:46
@Johnsyweb - c++ has a standard library? I thought it was just c with classes ;^) – Nate Oct 10 '11 at 01:47
@SkylineAddict: If you have such a restriction, you should state it (and the reason for it) in your question. – johnsyweb Oct 10 '11 at 01:49
@Nate: It seems the OP's professor is under a similar misconception. – johnsyweb Oct 10 '11 at 01:52
My professor is one who enjoys doing things from scratch (And making his students do so). But, I fixed the issue. I kept my manual copying, the only thing I had to change was putting the temporary char arrays into the fields of the classes, just to change the scope. After that, it works fine. – SkylineAddict Oct 10 '11 at 01:57
@SkylineAddict: I suspect (hope) that your professor purely wants you to understand how to do things from first principles. Just as long as you know not to re-invent wheels when you start work! – johnsyweb Oct 10 '11 at 02:08

score 1 · Answer 2 · answered Oct 10 '11 at 01:38

1

Take note on the lifetime of a variable. If you declare temp1 as static array, then it will be destroyed immediately by the end of function addItem.

At the end, all object that refers to this memory location will be invalid.

And .... If you want to pass a reference to an array do it this way:

Item(char** item, int itemType)

answered Oct 10 '11 at 01:38

dip

530
3
7

The lifetime was the main issue. Putting my temp arrays in the field of the class and not declaring it in the function fixed it. Thank you! – SkylineAddict Oct 10 '11 at 01:55

danadam · Answer 3 · 2011-10-10T02:02:29.643

In Item constructor you create local array char temp[200], you copy there what is pointed by char * item and then you don't use temp[200] any more. What's the point of doing that?

Later you assign passed pointer to _item member. The pointer points to local variable char temp1[200] in addItem(). When addItem() finishes then temp1 is destroyed and so _item in Item class points to garbage.

What you probably need to do is to allocate memory either statically in _item definition or dynamically using new (and then not forget to release it). I think the first solution will be safer for you. In the latter case you would also have to take care of copy constructor and assign operator. So, you need to change _item definition from char * _item to char _item[200], and then you can use strncpy:

Item(char* item, int itemType) {
    strncpy(_item, item, 200);
}

The way I did it was pretty much exactly what my professor wanted me to do. The only difference is, he wanted me to use strcpy to do the copying and not manually. I found strcpy to be more bothersome to me than helpful so I ended up not using it. — SkylineAddict, Oct 10 '11 at 01:50

score 0 · Answer 4 · answered Oct 10 '11 at 01:59

I'm imagining your definition of class Item minimally looks like this:

class Item
{
Item(char* item, int itemType);
private:
    char *_item;
};

Your constructor must allocate memory for _item in order to make a copy of what gets passed in via the constructor. Failure to do that will inevitable result in memory problems and exceptions. Alternatively, you can use something like a vector of char.

char array corruption in C++

4 Answers4