c - Which is the correct way to dynamically allocate multidimensional float arrays? Valgrind error

Question

I'm implementing a K-means algorithm in C. It works well most of the time, but debugging it with Valgrind tell me that I'm doing an "Invalid read of size 8 - Invalid write of size 8 - Invalid read of size 8" using '''memcpy''' at the beginning. I think the problem isn't there, but where I assign a value to the multidimensional float array element, which memory is dynamically allocated with '''malloc''' with a for loop at some point. 'Cause Valgrind also tell "Address 0x572c380 is 0 bytes after a block of size 80 alloc'd".

I've tried to add 1 to the number of bytes that I allocate, cause I thought that maybe '''malloc''' "needed" more memory to do its job, but nothing changed. I know maybe it's a basic error, but I'm quite new to the language and at my course it wasn't explain anything so "technical". I've tried to search the answer and explanation of the error but I have only found problems with '''char''' arrays, and with those I'd understood the function '''strcpy''' can resolve the issue. What about float arrays? It's the first time a use '''memcpy'''.

Here are pieces of code that raise those Valgrind messages.

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

void main(){
    FILE* fp; //used to open a .txt file to read
    char buf[100];
    float ** a;
    char * s;
    int i;
    int j;
    int rows = 10;
    fp = fopen("data.txt", "r");
    if(fp==NULL){
        perror("Error at open file.");
        exit(1);
    }

    a = (float**) malloc(rows*sizeof(float*));
    for(i=0; i<rows; i++){
        s = fgets(buf, 100, fp); //reading .txt file
        if (s==NULL){
            break;
        }
        a[i] = malloc(dim*sizeof(float));
        a[i][0] = atof(strtok(s, ","));
        for(j=1; j<dim; j++){
            a[i][j] = atof(strtok(NULL,","));  //save as float value the                token read from a line in file, for example, from line "1.0,2.0,3.0" as first line -> get a[0][1] = 2.0
        }
    }
    fclose(fp);

    m = (float**) malloc(rows*sizeof(float*));
    for (i=0; i<rows; i++){
        m[i]=malloc(dim*sizeof(float)); //not initialized
    }

    memcpy(m, a, rows*dim*sizeof(float));
}

Can someone also help me understand why it works but Valgrind raises these error messages?

That memcpy call does not do what you apparently think it does. It does a byte-wise copy from the source, which is a shallow copy. — Some programmer dude
Sorry, I've edited: there is no righe variable, just rows — erika
@Someprogrammerdude can you please explain me the correct use of memcpy, in this case? Should I memcpy every float in a[i][j] in m[i][j] with a for loop or it's the same issue? — erika

Matthieu Matthieu · Accepted Answer · 2019-01-14T19:18:20

You're first allocating an array of float*, then allocating several arrays of float so your last memcpy(m, a, rows*dim*sizeof(float)) copies an array of float* (pointers to float) to another one, but using rows * dim floats, which @SomeProgrammerDude rightfully noted. That would copy pointers, and not values.

~~Also, as pointed by @xing, you're allocating rows but using righe (which you didn't show). It might be a cause of problems.~~

I would suggest allocating the whole array at once on the first row, then having all other rows pointing to adequate rows:

a = malloc(rows * sizeof(float*));
a[0] = malloc(dim * rows * sizeof(float)); // Allocate the whole matrix on row #0
for (i = 1; i < rows; i++) {
    a[i] = a[0] + i * dim; // sizeof(float) automatically taken into account as float* pointer arithmetics
}
...
m = malloc(rows * sizeof(float*));
m[0] = malloc(dim * rows * sizeof(float));
memcpy(m[0], a[0], dim * rows * sizeof(float));

(add NULL checks of course)

c - Which is the correct way to dynamically allocate multidimensional float arrays? Valgrind error

1 Answers