Overwriting memory ?Using malloc in C, when calling structure

Question

im having problem with allocating memory for my following structure:

    typedef struct level {
    char* raw_map;      // original string representing the level map
    char* name;         // level name
    char* description;  // level description
    char* password;     // level password
    struct level *next; // pointer to the next level
} LEVEL;

My function looks like this:

LEVEL* parse_level(char* line){

    LEVEL *level =(LEVEL*)malloc(sizeof(LEVEL));

    int i=0;

    level->name = (char*)malloc(sizeof(level->name));
    while(line[i] != ';'){
       level->name[i]= line[i];
       i++;
    }
    level->password =(char*)malloc(sizeof(level->password));
    i++;
    while(line[i] != ';'){
       level->password[i]= line[i];
       i++;
    }
    return level;

So far, i have only set first two types of structure.

I call funciton like that:

int main(){

    LEVEL *first = (LEVEL*)malloc(sizeof(LEVEL));
    first =  parse_level("nameoffirstlevelwillbehere;mandragoramo5000");
    printf("name: %s pass: %s\n",first->name, first->password);
}

Im not sure that issue somewhere in malloc structure because im using pointers for first time in functions so i'm not sure they are placed right. When i have code like that i will have in first->name only 12 characters instead of full string until first ';'. Same happened in level->password. Thanks for help.

David Heffernan · Accepted Answer · 2014-04-19T20:18:29.897

level->name

is a pointer. So

sizeof(level->name)

returns the size of a pointer on your platform. That's known at compile time. In fact that's something to bear in mind. The sizeof operator is evaluated at compile time. So a priori it cannot be used to size the allocation of dynamically sized memory.

Instead you need to allocate enough memory to hold the length of string you require, including the null terminator.

You'll need to walk over line first to find the length of the string. Just as you presently do, looking for semi-colon separators. Then you can allocate the string's memory and finally copy it.

Do beware that your current code assumes that the string in line is well formed. If it does not have sufficient number of semi-colons you will read off the end of the buffer. It would be prudent to check for that eventuality.

The malloc function returns void* which is assignable to any pointer type. So casting is not necessary and it is generally best that you avoid casting.

Just one more thing, can i use something like level->name = malloc(strlen(line)) or it is bad because line in function is pointer. — user3476256, Apr 19 '14 at 21:11
Needs to be strlen(line)+1 for null terminator. But is too long because line contains multiple fields separated by semi-colons. — David Heffernan, Apr 19 '14 at 21:13

score 0 · Answer 2 · edited May 23 '17 at 12:20

0

This is your problem

level->name = (char*)malloc(sizeof(level->name));

Here level->name is a char pointer. So sizeof returns just the size of pointer (generally 4 bytes). You need to allocate sufficient memory for it.

level->name = malloc(sizeof(char)*100);

Do same for level->password And remember casting malloc is bad

edited May 23 '17 at 12:20

Community

1
1

answered Apr 19 '14 at 20:04

GoldRoger

1,263
7
10

1

How do you know that 100 will be enough? – David Heffernan Apr 19 '14 at 20:16

score 0 · Answer 3 · answered Apr 19 '14 at 20:05

0

Shouldn't level->name = (char*)malloc(sizeof(level->name)); be

level->name = (char*)malloc(strlen(line->name)+1);

answered Apr 19 '14 at 20:05

ASKASK

657
8
16

No it should not. For a start line is a string, and the terminator is a semi colon. – David Heffernan Apr 19 '14 at 20:16

nik_kgp · Answer 4 · 2014-04-20T09:49:14.550

We need to calculate length of name and password before allocating memory for it, or you may choose a default MAX_SIZE for it.

LEVEL* parse_level(char* line){

    LEVEL *level =(LEVEL*)malloc(sizeof(LEVEL));
    if (level == NULL) return NULL;

    int i=0, name_length=0;

    //calculate name length first, if MAX_SIZE is known then it can be dropped.        

    while(line[i] != ';') {
       i++;
    }

    level->name = (char*)malloc(i+1);
    if (level->name == NULL) {
        free(level);
        return NULL;
    }
    name_length = i;

    i = 0;
    while(line[i] != ';'){
       level->name[i]= line[i];
       i++;
    }
    level->name[i] = '\0';   

    // Calculating password length
    i++;
    while(line[i] != ';'){
       i++;
    }

    level->password = (char*)malloc(i - name_length);
    if (level->password == NULL) {
        free(level->name);
        free(level);
        return NULL;
    }

    i = name_length + 1;

    while(line[i] != ';') {
      level->password[i] = line[i];
      i++;
    }
    level->password[i] = '\0';


    return level;
  }

No. Not the malloc error checking. You'd missed that too. You allocate strings but fail to null terminate them. Also sizeof(char) is, by definition, 1. — David Heffernan, Apr 20 '14 at 08:47

Overwriting memory ?Using malloc in C, when calling structure

4 Answers4