CODEWITHSUNDEEP
cstringpointersfor-loopstrcat
beremaran
beremaran

Reputation: 57

strcat crashes program (0xc0000005)

I need to draw a line of characters as long I want. So I wrote a function for that purpose:

void fDrawLine(int length)
{
    int i;
    char * compLine = (char *) malloc(WINDOW_WIDTH + 2);

    for(i = 0; i < length; i++)
        strcat(compLine, "-");

    fDrawSpacedMessage(compLine, -1, TRUE);
}

WINDOW_WIDTH defined as 80, fDrawSpacedMessage is another function to print texts centered etc.

It's building perfectly, no errors, no warnings. But in runtime, everything works but if fDrawLine executes, the program crashes and gives the error code 0xc0000005. I know it's about memory allocation but I already initialize the compLine string.

I've tried a couple of things; I thought another function caused it so I isolated fDrawLine, but crashing continued. Changing initialize with compLine[0] = 0;, compLine[WINDOW_WIDTH] = {0}; did not help.

It works well with my other machine, which runs Ubuntu, with latest gcc, but when using Code::Blocks (MinGW) on Windows it keeps crashing.

What's wrong with this code?

Upvotes: 0

Views: 501

Answers (3)

kdopen
kdopen

Reputation: 8215

There are a few problems with your code below

void fDrawLine(int length)
{
    int i;
    char * compLine = (char *) malloc(WINDOW_WIDTH + 2);

    for(i = 0; i < length; i++)
        strcat(compLine, "-");

    fDrawSpacedMessage(compLine, -1, TRUE);
}

First, the length parameter should at least be unsigned int as a negative length makes no sense. Ideally, you should use size_t. The same goes for i.

Next, you are not protecting yourself against invalid values for length. The implicit contract is that 0 <= length <= WINDOW_WIDTH - make it explicit.

Your use of dynamically allocated memory leads to a memory leak as you don't release it after calling fDrawSpacedMessage().

Finally, strcat is overkill to append a single character.

Putting all that together, here's an alternative implementation.

void fDrawLine(size_t length)
{
    size_t actual_length = length <= WINDOW_WIDTH ? length : WINDOW_WIDTH;
    char compLine[WINDOW_WIDTH+2];

    memset(compLine, '-', actual_length);
    compline[actual_length] = '\0';
    fDrawSpacedMessage(compLine, -1, TRUE);
}

I've left compline at WINDOW_WIDTH+2 as I'm guessing fDrawSpacedMessage adds a newline.

If it still crashes, the problem is in fDrawSpacedMessage

Upvotes: 0

Iharob Al Asimi
Iharob Al Asimi

Reputation: 53016

Don't declare compLine as a pointer, since you don't need that, and actually you have a memory leak in your function, first declare compLine this way

char compLine[1 + WINDOW_WIDTH] = {0}; // strings need an extra byte at the end to mark the end.

then use memset to set the '-' character like this

memset(compLine, '-', length);

of course, check that length <= WINDOW_WIDTH.

This is your function fixed, so you can try it

void fDrawLine(int length)
{
    char compLine[1 + WINDOW_WIDTH] = {0}; // initialized so that last byte is '\0'.
    if (length > WINDOW_WIDTH)
        length = WINDOW_WIDTH;
    memset(compLine, '-', length);        
    fDrawSpacedMessage(compLine, -1, TRUE);
}

besides using strcat that way is a bad idea, you can do it this 

char *compLine = malloc(1 + length); // the last extra '\0' byte.
if (compLine == NULL) // malloc returns NULL on failure to allocate memory
    return; // so we must abort this function in that case.
for(i = 0; i < length; i++)
    compLine[i] = '-';
compLine[length] = '\0';

fDrawSpacedMessage(compLine, -1, TRUE);
free(compLine);

you can also use memset in this case, and it is actually better.

Upvotes: 1

Thomas Padron-McCarthy
Thomas Padron-McCarthy

Reputation: 27652

The allocated memory starts containing garbage. Set it to an empty string, for example like this:

compLine[0] = '\0';

Upvotes: 1

Related Questions