Strange memory management issues in C++ (from a beginner, at least)

Question

I'm new to C++, I have lots of Objective-C experience.

I'm trying to have an array of c-strings (that is char **) as an instance variable in my class, which gets allocated and filled in my constructor, and then in another member function I want to print out the whole "grid".

The allocation works, I fill up my array with strings (just "aaaaaaa" and so on for now). Checking at the end of my constructor, I see that each line has successfully been created and filled as expected.

However, I then call my printGrid() function, and then things go strange. If I've got 25 lines to print, say, the first 12 or so will print garbage, then the remaining 13 print out as expected. So it seems like I'm trampling over memory somewhere, and I'm not sure where.

My code might look a little messy because I've been trying different things, so I'll try to make it look as cohesive as possible.

main.cpp: Where I'm calling the functions

#include <iostream>
#include "Bitmap.h"

using namespace std;
int main (int argc, char * const argv[]) {

    Bitmap bitmap(15, 25);
    bitmap.printBitmap();

    return 0;
}

Bitmap.h: header for my class

class Bitmap {
private:
    char **_bitmap;
        void printLine(char const*lineString);
    int _width;
    int _height;
public:
    Bitmap();
        Bitmap(int width, int height);
    void printBitmap();
};

Bitmap.cpp: Where the action happens

#include <iostream>
#include "Bitmap.h"

using namespace std;
Bitmap::Bitmap() {
    // allocate space for the bitmap
    int numRows = 20;
    int numColumns = 30;

    Bitmap(numRows, numColumns); // Can I even safely do this? I'm not using the default constructor in my main() but I'm still curious.
}

Bitmap::Bitmap(int width, int height) {
    _width = width;
    _height = height;

    _bitmap = (char **)malloc(sizeof(char*) * height); // FIXED this line (used to be char, now it's char *).
    for (int currentRow = 0; currentRow < height; currentRow++) {
        _bitmap[currentRow] = (char *)malloc((sizeof(char) * width));
        snprintf(_bitmap[currentRow], width, "%s", "1");

        for (int currentColumn = 0; currentColumn < width; currentColumn++) {
            _bitmap[currentRow] = strcat(_bitmap[currentRow], "a");
        }
        printf("currentRow %0d: %s\n",currentRow, _bitmap[currentRow]); // Each row prints out FINE here, as expected
    }
}

void Bitmap::printBitmap() {
    int numColumns =_width;
    int numRows = _height;

    if (NULL == _bitmap)
        return;

    // iterate over the bitmap, line by line and print it out
    for (int currentRow = 0; currentRow < numRows; currentRow++) {

        // If there are, say, 25 lines, the first 12 or so will be garbage, then the remaining will print as expected
        printLine((char const *)_bitmap[currentRow]);
    }
}

void Bitmap::printLine(char const*lineString) {
    printf(":%s\n", lineString);    
}

This is for school and the prof isn't allowing C++ vectors or strings. Otherwise, yes I know I should be using those. Thanks for all the suggestions.

Just out of curiosity: If you're coding this in C++, why not use features available for you such as new/delete, etc? — Mike Bailey, Jan 20 '11 at 18:08
.....this is c++? I can see you've got a hodge podge of c-style c++ and c++ - consider investing in a good c++ book! ;) `malloc` is a *faux-pas* in c++ land, consider using `std::vector` — Nim, Jan 20 '11 at 18:10
@Sagekilla: Not `new`/`delete` here. `std::vector` is what the OP really should be using here. — Billy ONeal, Jan 20 '11 at 18:11
@chris that was a typo (that's the name it's actually called in my code but I figured it would be simpler to just say print. Fixed. — jbrennan, Jan 20 '11 at 18:15
Re: hodge-podgeness. This is for a class at school, and the prof isn't letting us use C++-isms quite yet (like new/delete, and none of the cpp strings, vectors, etc.). It's dumb, I know. — jbrennan, Jan 20 '11 at 18:16
@Billy This is for school and the prof isn't letting us use vectors here. Sadly. — jbrennan, Jan 20 '11 at 18:21
...here's a tip - `calloc()` will also initialze the block allocated to a character (what every thing in your inner loop is doing) - so with one line: `_bitmap[currentRow] = (char *)calloc('a', (sizeof(char) * width));` - NOTE: this will not be a null terminated string though, so you should still NUL terminate it correctly... — Nim, Jan 20 '11 at 18:24

score 6 · Accepted Answer · answered Jan 20 '11 at 18:10

6

Red flag:

_bitmap = (char **)malloc(sizeof(char) * height);

should be

_bitmap = (char **)malloc(sizeof(char*) * height);

You want a pointer to a char*, not a pointer to a char.

answered Jan 20 '11 at 18:10

chrisaycock

36,470
14
88
125

Wow. Duh, yeah haha. I wonder how I missed that. OK. Well now the first 3 lines are garbage, and the rest are fine. I must be stomping on memory somewhere still. – jbrennan Jan 20 '11 at 18:13
@jbrennan, after this fix it looks correct to me, works fine for me too. Try recompiling from stratch and testing once again. – Sergei Tachenov Jan 20 '11 at 18:53
1

@jbrennan, I take that back. See David's answer. You forgot the space for the null terminator. – Sergei Tachenov Jan 20 '11 at 19:08
@jbrennan Also look at Greg's answer regarding default arguments to a constructor. – chrisaycock Jan 20 '11 at 19:25

Joel · Answer 2 · 2011-01-20T18:37:17.493

5

_bitmap = (char **)malloc(sizeof(char) * height);

should be

_bitmap = (char **)malloc(sizeof(char*) * height);

and only if you're coding C.

Better is to use new/delete if you absolutely need the bitmap to be contiguous, and

Vector< Vector < char > >

if you don't.

Also, strcat seems an odd choice, given that you haven't initialized the memory yet. I.e. there is not necessarily a 0, so no end to the string. That may cause your memory stomp. Try using strcpy (or strncpy if you want to be safe).

edited Jan 20 '11 at 18:37

answered Jan 20 '11 at 18:10

Joel

5,618
1
20
19

This is for school and the prof won't let us use Vectors or other built-in C++ datastructures. It's a bummer. – jbrennan Jan 20 '11 at 18:18
@Matthieu No. I would have marked it as C except for the fact that, you know, I'm using objects. More specifically, Object constructors. My questions are partly based around not understanding how object lifecycle works in C++. – jbrennan Jan 20 '11 at 19:59

Greg Hewgill · Answer 3 · 2011-01-20T18:17:03.477

Related to this comment inside your default constructor:

Bitmap(numRows, numColumns); // Can I even safely do this? I'm not using
                             // the default constructor in my main() but
                             // I'm still curious.

This does not do what you think it does. This is not a call to the other constructor to do additional initialisation. Instead, this creates another temporary unnamed Bitmap object using numRows and numColumns, and then immediately calls its destructor. This statement acts like a local variable with no name.

In your case you can supply a default constructor by giving your one constructor default arguments:

public:
    Bitmap(int width = 20, int height = 30);

score 4 · Answer 4 · answered Jan 20 '11 at 18:28

This malloc isn't leaving room for a 0 byte at the end of the string:

    _bitmap[currentRow] = (char *)malloc((sizeof(char) * width));

Since "sizeof(char)" is 1 by definition, you can just do:

    _bitmap[currentRow] = (char *)malloc(width+1);

And in this construct:

    for (int currentColumn = 0; currentColumn < width; currentColumn++) {
        _bitmap[currentRow] = strcat(_bitmap[currentRow], "a");
    }

you don't really want to use strcat, just assign the char directly:

    for (int currentColumn = 0; currentColumn < width; currentColumn++) {
        _bitmap[currentRow][currentColumn] = 'a';
    }
    _bitmap[currentRow][width] = 0; // and don't forget to terminate the string

score 1 · Answer 5 · answered Jan 20 '11 at 18:17

In addition to all the other answers:

Bitmap::Bitmap() {
    // allocate space for the bitmap
    int numRows = 20;
    int numColumns = 30;

    Bitmap(numRows, numColumns); // Can I even safely do this? I'm not using the default constructor in my main() but I'm still curious.
}

No, you can't do this. Each constructor is independent and they cannot delegate to each other.

For memory management, use dedicated resource management classes that will automatically control the memory for you. The Standard provides an excellent series of classes and std::vector<std::string> shall serve the purpose in this case excellently.

Thanks for the info. Sadly I'm not allowed to use Vectors or C++ strings for this assignment (it's for school). — jbrennan, Jan 20 '11 at 18:20

score 0 · Answer 6 · answered Jan 20 '11 at 18:14

The following should allocate correctly (haven't tested it).

_bitmap = new char*[height];
for (int currentRow = 0; currentRow < height; currentRow++) 
{         
    _bitmap[currentRow] = new char[width];         
    snprintf(_bitmap[currentRow], width, "%s", "1");          
    for (int currentColumn = 0; currentColumn < width; currentColumn++) 
    {             
        _bitmap[currentRow] = strcat(_bitmap[currentRow], "a");         
    }                 // Each row prints out FINE here, as expected     

    printf("currentRow %0d: %s\n",currentRow, _bitmap[currentRow]); 
}

Also be sure to define a copy constructor, destructor, and assignment operator to ensure memory doesn't leak and the array gets deleted.

score 0 · Answer 7 · answered Jan 20 '11 at 18:14

here I think you want sizeof (char *) inside malloc

_bitmap = (char **)malloc(sizeof(char) * height);

Also, when you are filling the string with "a", you have to make sure you are not overwriting any memory: you allocated width character, you print "1" to it, then concatenate "a" width times, which would go 1 past the allocated memory (not to mention not leave any room for the nul-termination

score 0 · Answer 8 · answered Jan 20 '11 at 18:16

0

Your malloc() call doesn't look right to me, but perhaps I'm missing something.

What I should be seeing is one malloc() call for the storage for the array. If you want 10 C strings in it, that would be malloc(10 * sizeof (char *)). Then I should see some more malloc() calls that actually allocate the memory the 10 strings themselves use.

But I'm only seeing one malloc() call, that appears to think it is allocating string array memory, not string pointer array memory.

answered Jan 20 '11 at 18:16

T.E.D.

44,016
10
73
134

@jbrennan - Ah, so then perhaps the only issue was in fact the size you were allocating. Generally `sizeof(char)` is 1, while `sizeof(char *)` is 4 or 8. So you were allocating your array of string pointers way too small. – T.E.D. Jan 20 '11 at 22:18

Strange memory management issues in C++ (from a beginner, at least)

8 Answers8