How to use malloc and strtok properly?

Question

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

#define MAX_HEIGHT 5
#define MAX_WIDTH  9
#define MAX_DIRECT 30

typedef struct position_t position_t;

struct position_t {
        char *position;
        char *ptr;
};

int main(int argc, char *argv[])
{
        int i;
        FILE *fp;
        char a[50], b[50], c[50], d[50];
        position_t pos;

        pos.position = malloc(sizeof(char) * 20);

        for (i = 1; i < argc; i++) {
                fp = fopen(argv[i], "r");
                if (fp == NULL) {
                        fprintf(stderr, "cat: can't open %s\n", argv[i]);
                        continue;
                }

                fgets(a, 50, fp);
                fgets(b, 50, fp);
                fgets(c, 50, fp);
                fgets(d, 50, fp);

                fclose(fp);

                while (1) {
                        int j = 0;

                        pos.position = 0;
                        pos.ptr = strtok(a, ",.; ");

                        while (pos.ptr != NULL) {
                                pos.position[j] = *pos.ptr;
                                j++;
                                pos.ptr = strtok(NULL, ",.; ");
                        }

                        printf("%c", pos.position[j]);
                }

        }

        free(pos.position);
        return 0;
}

What I want to do is to read the first line from a file (the content of it is: START FOYER ELEVATOR) and split them by space through strtok, then store each of the strings in the malloc, pos.position and use it later wherever I want to use. Can someone please fix this code?

I recommend you read [this question about casting the return of `malloc`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). — Some programmer dude, Apr 20 '17 at 14:23

Some programmer dude · Answer 1 · 2017-04-20T14:27:33.833

With

pos.position = (char*)malloc(sizeof(char)*20);

you allocate 20 bytes of memory, and makes pos.position point to that memory.

But later you do

pos.position = 0;

which makes pos.position become a null pointer.

This resetting of the pointer will lead both to a memory leak as you lose the pointer returned by malloc, and it will also lead to undefined behavior when you dereference the pointer with e.g. pos.position[j].

Furthermore, when you after the inner while loop do

printf("%c", pos.position[j]);

you print an element of the memory that have not been initialized.

To make the current code work, don't reset the pointer in the loop. Add a terminator after the inner while loop. And print the array as a string. Oh and add a condition so you don't write out of bounds of the allocated memory.

Actually, since you always allocate a fixed amount of memory I rather recommend you don't allocate memory dynamically at all. Instead make pos.position an array.

How to use malloc and strtok properly?

1 Answers1