1

Hello i have been struggling with my code for a while and finally found that the free() funtion is the cause. I think i am missing something about the details of how free() works.

My output is :

test test test test test 
ID: 200
RELEASE YEAR: 2006


ID: 201
RELEASE YEAR: 2006


ID: 202
RELEASE YEAR: 2006


ID: 203
RELEASE YEAR: 2006


ID: 204
RELEASE YEAR: 2006


AB

Edit : Added the full code

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

#define MAX 1000                                        //Do not edit this macro.

typedef struct{
    int film_id;
    char title[255];
    char description[1023];
    unsigned int release_year;
    char rental_duration;
    float rental_rate;
    unsigned char length;
    float replacement_cost;
    char rating[10];
    char last_update[30];
} RECORD_t, *RECORD;                                    //Do not edit this struct.


RECORD *find_by_year(int release_year, RECORD film_array, int start, int end,int *found);


int main(){
    RECORD rec = (RECORD)malloc(sizeof(RECORD_t)*MAX);  //Do not edit this line.
    FILE *file = fopen("data.txt", "rb");               //Do not edit this line.
    if (file == NULL) {                                 //Do not edit this line.
        printf("Cannot open the file.\n");              //Do not edit this line.
        exit(0);                                        //Do not edit this line.
    }                                                   //Do not edit this line.
    fread(rec, sizeof(RECORD_t)*MAX, 1, file);          //Do not edit this line.
    fclose(file);                                       //Do not edit this line.


    int i,test;
    RECORD *rec_arr;

    rec_arr=find_by_year(2006,rec,200,203,&test);
    for(i=0;i<test;i++){
            printf("ID: %d\n", rec_arr[i]->film_id);
            printf("RELEASE YEAR: %d\n", rec_arr[i]->release_year);
            printf("\n\n");
            fflush(stdout);
    }
    printf("A");
                fflush(stdout);

//  file = fopen("data.txt", "wb");                     //Do not edit this line.
//  fwrite(rec, sizeof(RECORD_t)*MAX, 1, file);         //Do not edit this line.
//  fclose(file);                                       //Do not edit this line.
    free(rec);                                          //Do not edit this line.


    printf("B");
    fflush(stdout);

    free(rec_arr);

printf("C");
    fflush(stdout);

    return 1;                                           //Do not edit this line.
}

RECORD *find_by_year(int release_year, RECORD film_array, int start, int end,int *found) {

    RECORD *rec_arr=malloc(sizeof(RECORD)*1);
    RECORD *narray;//for realloc check
    int size=1,i,j;
    start--;
    if(rec_arr==NULL){//if malloc fails
        printf("MALLOC FAILED find_by_year returning NULL\n");
        fflush(stdout);
        return NULL;
    }

    for(i=start;i<=end;i++){
        if(film_array[i].release_year==release_year){
            rec_arr[size-1]=&film_array[i];
            size++;
            narray=realloc(rec_arr,size);//increment the size by 1


            //ERROR HANDLING
            if(narray==NULL){//if realloc fails
                printf("INNER REALLOC FAILED find_by_year");
                fflush(stdout);
                narray =malloc(sizeof(RECORD) * size);

                if(narray==NULL){ //if malloc fails
                    printf("INNER MALLOC ALSO FAILED find_by_year returning NULL\n");
                    fflush(stdout);
                    return NULL;
                }

                for(j=1;j<size;j++){//copy
                    narray[size-1]=rec_arr[size-1];
                    free(rec_arr);
                }
            }
            printf("test ");
            fflush(stdout);

            rec_arr=narray;
        }
    }
    printf("\n");
    fflush(stdout);

    *found=size-1;


    if(size==1)//if not found anything
        return NULL;

    return rec_arr;
}



From the results of debugging free(rec_arr) fails everytime what could be the problem here. I cropped the code and i am almost sure that the cropped parts are working from the debugging.

17
  • Your example cannot be compiled! Try to edit it. Commented Mar 7, 2020 at 8:24
  • 1
    Please post a stackoverflow.com/help/minimal-reproducible-example . There's a bug in a part of the code you didn't show . The design of find_by_year is suspicious as it provides no way for the caller to know how many items are returned Commented Mar 7, 2020 at 8:30
  • @M.M okay here is the full code if you want to take a look Commented Mar 7, 2020 at 8:47
  • @MustafaÇığGökpınar can you also provide a sample of data.txt ? Commented Mar 7, 2020 at 8:54
  • Does your file contains exactly 1000 records? if not you are overflowing the data or accessing out of bounds in the function. I suggest to use ftell divided by sizeof(RECORD_t) to get the number of records instead of using a magic number like 1000 Commented Mar 7, 2020 at 9:00

2 Answers 2

2

The main thing wrong with your code is the approach. Repeatedly incrementing the size of an array by one using realloc() is horrifically inefficient. Instead, just do one pass over the data to check how many records match, then allocate the entire storage at once and do a second pass to populate it. Or, allocate an array as large as all the records, populate just the part you need, and don't worry about the "wasted" memory because it probably doesn't matter.

Either of the above approaches will result in much simpler code with fewer bugs.

Sign up to request clarification or add additional context in comments.

6 Comments

Inefficient use of realloc has nothing to do with a runtime error due to free() -- this should be acomment at best
@M.M we can not see why it fails because 1) the code doesn't compile and 2) the OP is ommiting the critical part: //reading data from a txt file, furthermore John is giving a good advice maybe too long to be a comment, it looks like a good answer to me
@DavidRanieri The question is why free() crashes , and this "answer" does not address that question at all. The fact that the question text is rubbish doesn't change that . Answers are for an answer to the question, not general advice.
@M.M: Unlike you, I am trying to help OP by addressing what may be the root cause of their problems.
@JohnZwinck thanks for the advice thats way more efficient .However as we are learning realloc and malloc our lecturers requested us to approch the problem like this.
|
1

This line is incorrect:

narray=realloc(rec_arr,size);//increment the size by 1

The second argument to realloc should be the number of bytes, not the number of records. You should multiply size by sizeof(RECORD) .


Here's some general recommendations about the rest of the code:

  • I suggest just aborting in the if(narray=NULL) case. That code will almost never happen anyway , and probably will fail anyway even if it does enter. Recovering from out-of-memory is an advanced topic (especially taking into account that modern operating systems overcommit).
  • You mentioned using Eclipse+CDT -- you should be able to step through the code in the debugger instead of having to use printf debugging statements.
  • It would be good to check for rec being NULL on the first line of main, and also check the return value of find_by_year -- if it returns NULL then you don't want to go on to do the i loop. Your function does not set *found in the case of returning NULL, sometimes, so the caller has to do the null check before using the record count.
  • I don't really agree with the other suggestion to change the realloc strategy. Keeping the code simple to read is not a bad plan , either for beginners or experts. And I doubt it is really inefficient as modern operating systems have a minimum allocation size so the realloc calls will basically be doing nothing unless your search returns more than say 1000 records.

8 Comments

@MustafaÇığGökpınar OK, I added some more suggestions
@M.M good catch and good advices! and I agree, using realloc even with the cost of restoring the previous data is most of the times preferable than memory fragmentation, but in this case (the origin was there and it was computable beforehand) I will prefer a single allocation
@DavidRanieri it probably is a single allocation -- say the OS provides a min size 4K block to the program, then the realloc call will just be adjusting the size used from 4, then 8, then 12 etc. , not actually requesting any more memory from the OS. I suspect it might be slower to pass over the data twice than to stick with the current code, if we're not returning thousands of records (which are pointers so you get lots of them in that 4K block) . If we look towards scaling up then I'd still tend to favour a single read pass but with a larger block strategy as the first attempt
@DavidRanieri C++ allocators don't support realloc, they always obtain a new block and free the old . There has been much debate over whether this was a good idea or not... the issue is that C++ objects might not be trivially relocatable (e.g. they may contain a pointer to themself) so having an unpredictable movement would spoil things. I think the library implementor would be allowed to use a realloc for a vector of trivially relocatable objects with default allocator under the as-if rule -- but that discussion would exceed the space in comments!
@DavidRanieri Yes; I guess you could dig into the libc implementation of realloc to see how any particular one works
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.