1

This is my code:

typedef struct bobTheBuilder{
    char *name;
    int fix;
    int max;
};

int main(void){

    struct bobTheBuilder bob;
    initBob(&bob);
    del(&bob);

    system("PAUSE");
    return (0);
}

void initBob(struct bobTheBuilder *currBob)
{
    char *n="bob";
    currBob->name = (char*)malloc(sizeof(char)*strlen(n));
    strcpy((*currBob).name, n);
    (*currBob).fix = 0;
    (*currBob).max = 3;
}

void del(struct bobTheBuilder *currBob)
{
    free(currBob->name);
}

Visual studio breaks at the free sentence.

What should i do? Is the problem with the free or the malloc?

Spikatrix
  • 20,225
  • 7
  • 37
  • 83
guy ribak
  • 125
  • 1
  • 1
  • 8
  • Providing functionality to deallocate a string in the "destructor" of your data structure is a bad idea: what if someone initializes `currBob->name` with a string literal or a `char` stack array? See [here](http://stackoverflow.com/questions/28896609/how-to-distinguish-between-strings-in-heap-or-literals) for a similar case. –  May 03 '15 at 12:54
  • And use `getch()` instead of `system("pause")`. – Jongware May 03 '15 at 13:11
  • @CoolGuy: I prefer C over `system(xx)` because it does not needlessly invokes the shell. (There are arguments against `getch` as well -- but I don't have a better recommendation, because I never have to somehow "stop" output as I do not run terminal commands under Windows, where this is an issue.) – Jongware May 03 '15 at 15:02
  • What do you mean with *breaks at the `free` sentence*? – Luis Colorado May 04 '15 at 05:40

1 Answers1

7

The line

currBob->name = (char*)malloc(sizeof(char)*strlen(n));

is wrong because

  1. You did not include the space for the NUL-terminator.
  2. You should not cast the result of malloc(and family) in C.

Fix the problems by using

currBob->name = malloc(strlen(n) + 1);

If you are wondering why I've removed sizeof(char), it is because sizeof(char) is guarenteed to be 1. So, it is not necessary.

As @EdHeal mentions in a comment, there is a function called strdup() that does malloc+strcpy. It is POSIX function. If it is available, you can shorten 
currBob->name = malloc(strlen(n) + 1);
strcpy((*currBob).name, n);

to

currBob->name = strdup(n);

by using this function. Also, note that 

(*currBob).fix = 0;
(*currBob).max = 3;

is equivalent to

currBob -> fix = 0;
currBob -> max = 3;

as @Edheal mentions in another comment.

Community
  • 1
  • 1
Spikatrix
  • 20,225
  • 7
  • 37
  • 83