Having some leaking memory in C

Question

I have some problems with my C code.

The code is intended to read a specific type of txt file (it's used in a C project of a basic search engine by similarity, which can search image, audio or text files).

Here's the code :

typedef struct HISTOGRAMME_E{
    int ** valeurs;
    int nbcolonne;
    int nbligne;
    char type;
}HISTOGRAMME;

HISTOGRAMME * lireDescripteur(FILE * read){

    HISTOGRAMME * retour = malloc(sizeof(HISTOGRAMME));


    int etat = 0;
    int id, type, nbligne, nbcolonne;
    unsigned int max;
    unsigned int cpt = 0;
    int i;
    char canswitch = 1;
    char* val = malloc(sizeof(char));

    int ** histoTempo;


    while (fscanf(read,"%s",val) == 1) {
        // Fonctionnement en MAE

        // Actions
        if(etat == 1){
            id = atoi(val);
            etat = 0;
        }

        if(etat == 2){
            if(strcmp(val, "RGB"))
                type = 3;
            else
                type = 1;
            etat = 0;
        }

        if(etat == 3){
            nbcolonne = atoi(val);
            etat = 0;
        }

        if(etat == 4){
            nbligne = atoi(val);
            etat = 0;
        }



        // Valeurs

        if(etat == 5){
            max = nbligne * nbcolonne;
            histoTempo = malloc(sizeof(int*)*2);
            histoTempo[0] = malloc(sizeof(int)*max);
            histoTempo[1] = malloc(sizeof(int)*max);
            cpt = 0;
            canswitch = 0;
            histoTempo[0][0] = (int)strtol(val, NULL, 0);
            etat = 52;
        }

        if(etat == 51 && canswitch){
            histoTempo[0][cpt] = (int)strtol(val, NULL, 0);
            etat = 52;
            canswitch = 0;
        }


        if(etat == 52  && canswitch){

            histoTempo[1][cpt] = atoi(val);
            etat = 51;
            canswitch = 0;
            cpt += 1;
        }





        // Changement d'états
        if(strcmp(val, "<id>") == 0 && (etat == 0))
            etat = 1;

        if(strcmp(val, "<type>") == 0 && (etat == 0))
            etat = 2;

        if(strcmp(val, "<nbcolonne>") == 0 && (etat == 0))
            etat = 3;

        if(strcmp(val, "<nbligne>") == 0 && (etat == 0))
            etat = 4;

        if(strcmp(val, "<valeurs>") == 0 && (etat == 0))
            etat = 5;


        //if(strcmp(val, "</valeurs>") == 0 && ((etat==51) || (etat == 52))) 
        if(strcmp(val, "</valeurs>") == 0)
            {
                //affichage debug
                printf("id:%u, type:%u, nbcolonne:%u, nbligne:%u\n", id, type, nbcolonne,nbligne);
                /*for(i=0;i<cpt;i++){
                    printf("%x : %u \n", histoTempo[0][i], histoTempo[1][i]);
                }*/

                int ** histogramme = malloc(sizeof(int*)*2);
                histogramme[0] = malloc(sizeof(int)*cpt);
                histogramme[1] = malloc(sizeof(int)*cpt);

                for(i=0;i<cpt;i++){
                    histogramme[0][cpt] = histoTempo[0][cpt];
                    histogramme[1][cpt] = histoTempo[1][cpt];
                }
                cpt = 0;
                etat = 0;
                retour->valeurs = histogramme;
                retour->nbcolonne = nbcolonne;
                retour->nbligne = nbligne;
                retour->type = type;


                nbligne = 0;
                nbcolonne = 0;
                type = 0;
                free(histoTempo[0]);
                free(histoTempo[1]);
                free(histoTempo);
                free(val);
                return retour;
            }
        canswitch =1;
    }
    return retour;
}

void test()
    {
    FILE * read =  fopen(FICHIER_DESCRIPTEUR,"r");


    HISTOGRAMME * test;
    int i = 0;

    test = lireDescripteur(read);

    //printf("%i\n\n\n", test->valeurs[1][1]);
    fclose(read);
    free(test);
}

Here's Valgrind log:

> 220 bytes in 1 blocks are indirectly lost in loss record 1 of 3
>> ==21968==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==21968==    by 0x402AF2: lireDescripteur (index_img.c:905)             
>> ==21968==    by 0x402C62: test (index_img.c:942)                             
>> ==21968==    by 0x402C8F: main (index_img.c:958)               


> ==21968== 220 bytes in 1 blocks are indirectly lost in loss record 2 of 3
>> ==21968==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==21968==    by 0x402B13: lireDescripteur (index_img.c:906)            
>> ==21968==    by 0x402C62: test (index_img.c:942)                  
>> ==21968==    by 0x402C8F: main (index_img.c:958)



> ==21968== 456 (16 direct, 440 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 3
>> ==21968==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==21968==    by 0x402ADF: lireDescripteur (index_img.c:904)         
>> ==21968==    by 0x402C62: test (index_img.c:942)                 
>> ==21968==    by 0x402C8F: main (index_img.c:958)

It seems it's comming from the malloc of "histogramme" in "lireDescripteur" but I can't grasp why. (The three line of code pointed by valgrind :)

int ** histogramme = malloc(sizeof(int*)*2);
histogramme[0] = malloc(sizeof(int)*cpt);
histogramme[1] = malloc(sizeof(int)*cpt);

The function "lireDescripteur" should return a pointer on a structure of type "HISTOGRAMME", with its "valeurs" pointing on "histogramme".

Complete code

Complete valgrind log

Answer 1

The issue valgrind points out seems to be outside the code you're showing, but in the code you're showing, you also have a memory leak because you're not freeing val in all return paths: here:

    canswitch =1;
}
return retour;  // you're not freeing `val` here

but more importantly you have undefined behaviour because val is an array of 1 byte, and you're putting way more data in it (see: How dangerous is it to access an array out of bounds?).

I'd suggest a local array, with a correct size (increase if you have bigger words):

char val[100];

int ** histoTempo;


while (fscanf(read,"%99s",val) == 1) {  // safe fscanf: cannot read more than 99 chars

and don't free(val) in the end. That way you're solving the memory leak and the memory out-of-bounds that can crash your program.

(your program has this issue several times, from the link you provided. As a general rule, use properly sized local arrays when you don't need to return the buffer to the caller)