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

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;

    _bitma开发者_C百科p = (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.

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.

_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).

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);

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

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.

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.

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

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.