Malloc error when reading from file

Question

I have a file with each line of strings is in this format

key1 = value1
key2 = value2
....

I need to extract the strings in third column. The code I've written so far is

    fp = fopen(file, "r");
    assert(fp && "checkpoint file not found  \n");

    char **data = (char **) malloc(sizeof (char*) * lines ); // lines=100
    size_t i = 0;

    while ((read = getline(&line, &len, fp)) != -1){
        size_t l = strlen(line);

        char value[256];
        sscanf( line, "%s %s %s", field, tmp, value); // field stores 'key1', tmp stores '+', value stores 'value1'

        data[i] = value;
        i++;
        printf("%s \n", value);
        // printf("%s %s\n", value, data[i]); -- when this line is uncommented, it leads to a seg-fault.
    }
    fclose(fp);


      for (int i=0; i < lines; ++i)
        free(data[i]);
      free(data);

I get an error "malloc error for object .., pointer being freed is not allocated".

[Please see this discussion on why not to cast the return value of `malloc()` and family in `C`.](http://stackoverflow.com/q/605845/2173917). — Sourav Ghosh, Apr 05 '17 at 21:06
You didn't show any code calling `free`, which the error message seems to have come from. We also don't know what `tmp`, `field` and some other things are. Please post an [MCVE](https://stackoverflow.com/help/mcve) — interjay, Apr 05 '17 at 21:14
`data[i] = value;` is copying a pointer to the same input string whose content gets overwritten in every loop. Try the non-standard `data[i] = strdup(value);` which might fix it. The function `strdup` calls `malloc` so you'll have to `free` each element of the array of pointers later. — Weather Vane, Apr 05 '17 at 21:15
is it segfault, "malloc error for object .., pointer being freed is not allocated", or both alternatively with a 43% probability for the segfault ? :) anyway your code is very very buggy. — Jean-François Fabre, Apr 05 '17 at 21:16
@WeatherVane +1 if OP tries to free the pointers of the array, that could explain a lot because freeing auto (invalidated) memory is rarely appreciated by the system. — Jean-François Fabre, Apr 05 '17 at 21:19
In C, it is not appropriate to use assert to check the return value of a function. `assert` is for things which are logically necessary. `fopen` can return NULL, and it is not correct to `assert` otherwise. — William Pursell, Apr 05 '17 at 21:26

score 1 · Answer 1 · edited May 23 '17 at 11:46

1

The problem as I see it is, before

  printf("%s %s\n", value, data[i]);

this line, you're doing i++ which changes the value of i. Then data[i] points to an uninitialized memory. Passing that as argument to %s causes undefined behavior.

[Edit:]

Then again, you're doing

 free(data[i]);

whereas, data[i] is not a pointer returned by a memory allocator function. That's another cause for UB.

That said,

You never bound i with lines, which is the one-over the maximum allowed index. You need to keep a check such that i < lines satisfies at all times.
You did not check for the success of sscanf() call, failure to this call may lead to reading of invalid memory too.
See this discussion on why not to cast the return value of malloc() and family in C.. You should also check for the success of malloc() before dereferencing the returned pointer.

edited May 23 '17 at 11:46

Community

1
1

answered Apr 05 '17 at 21:08

Sourav Ghosh

133,132
16
183
261

1

Very true, I was that too. But isn't there other errors? like number of lines too low (ok: telepathy :))? because the "pointer being freed" is probably not because of that error. Well, you never know with UB :) – Jean-François Fabre Apr 05 '17 at 21:10
@Jean-FrançoisFabre Does the expansion cover this, or you meant anything else? :) – Sourav Ghosh Apr 05 '17 at 21:12
1

your answer is well documented (and you got my vote). I was just wondering if OP gets segfault or "malloc error for object .., pointer being freed is not allocated", which could only occur when freeing memory, and probably not because of a printf present or not. Memory corruption + free can lead to that all right. – Jean-François Fabre Apr 05 '17 at 21:15
@Jean-FrançoisFabre Agreed, but with this snippet, above is what we got. :) – Sourav Ghosh Apr 05 '17 at 21:16
Weather Vane has a point when he says that OP puts a local array in the pointer array (and the same array!!) so probably crashes when attempting to free the pointers in the array when they haven't been allocated in the firtst place. – Jean-François Fabre Apr 05 '17 at 21:17
@Jean-FrançoisFabre Very probable, my answer just follows the comment present in OP's snippet, whatever is shown here. – Sourav Ghosh Apr 05 '17 at 21:20
@Jean-FrançoisFabre and there you go...check OP's edit. :) – Sourav Ghosh Apr 05 '17 at 21:21
crystal ball strikes again :) also `data` isn't even valid memory since it copied an auto variable which went out of scope. You can add that (but I cannot upvote once more :)) – Jean-François Fabre Apr 05 '17 at 21:24

score 0 · Accepted Answer · answered Apr 05 '17 at 21:34

0

The direct cause of the observed error is:

char value[256];
//....
data[i] = value;

and later (in a subsequent loop):

free(data[i]);

You are freeing the statically allocated array.

You want:

char *value=malloc(sizeof(char)*256);

I know sizeof(char)==1 by definition. It's just good practice.

answered Apr 05 '17 at 21:34

Persixty

8,165
2
13
35

1

Thanks, accepting this answer as it has example to understand better. – marc Apr 05 '17 at 23:33

score 0 · Answer 3 · answered Apr 05 '17 at 21:34

Declaring char value[256] reserves some room on the function stack. Your data[i] keeps pointing to that same place on the stack, for different i. Note that you have a possible overflow problem on the stack when value is too small, unless there is a fixed limit to its size. This stack region is not alloced so cannot be freed, hence the error.

A consequence is that data[i] gets overwitten by all further lines after its first asignment. You get a (non-freeable) pointer to the same region every time. You need to copy that value to tyour own pointer:. What you also need to do is free the variable line in the end (as it gets a malloc-ed pointer value and gets realloc-ed to make it fit the line.), and for every line do a malloc to make a pointer to copy value into and set data[i] to that new pointer. So there are all sorts of problems in your code.

Excellent ! Thanks for a detailed explanation. – marc Apr 05 '17 at 23:33 — marc, Apr 05 '17 at 23:33

Malloc error when reading from file

3 Answers3