0

i have a few issues with that i was wondering what am i doing wrong and if can someone explain me futher details about the realloc and malloc functions. the problem i have is from my main:

typedef struct Books {
   char *id;
   char *title;
   char *author;
   char *pages;
   char *year;
   char *subject;
} book;
char* filename;
int bookcount=1;
int libsize=4;
int main(int argc, char* argv[]){
 if (argc < 2)
    return -1;
 filename=argv[1];
 FILE* fptr;
 book* books;
 char tempstring[maxsize],* token;
 int i=0,ch;
 fptr=fopen(filename,"r");
 if(fptr==NULL)
    return-1;
//this count how many books are in the file
  while(ch!= EOF){
    ch=fgetc(fptr);
    if(ch == '\n')
    ++bookcount;
  }
 fclose(fptr);
 while(libsize<bookcount){
    libsize *= 1.5;
 }
 books=(book*) malloc(libsize*sizeof(book));
 if(books==NULL)
    exit(-1);

 fptr=fopen(filename,"r");
 if(fptr==NULL)
    return-1;
//this gets all the books into the book array
  for(i=0;i<bookcount;i++){
    fgets(tempstring,maxsize,fptr);
    token=strtok(tempstring,",");
    ch=strlen(token);
    books[i].id=(char*)malloc(ch+1);
    strcpy(books[i].id,token);
    token=strtok(NULL,",");
    ch=strlen(token);
    books[i].title=(char*)malloc(ch+1);
    strcpy(books[i].title,token);
    token=strtok(NULL,",");
    ch=strlen(token);
    books[i].author=(char*)malloc(ch+1);
    strcpy(books[i].author,token);
    token=strtok(NULL,",");
    ch=strlen(token);
    books[i].pages=(char*)malloc(ch+1);
    strcpy(books[i].pages,token);
    token=strtok(NULL,",");
    ch=strlen(token);
    books[i].year=(char*)malloc(ch+1);
    strcpy(books[i].year,token);
    token=strtok(NULL,",");
    ch=strlen(token);
    books[i].subject=(char*)malloc(ch+1);
    strcpy(books[i].subject,token);
   }
  books=(book*) realloc(books,libsize*sizeof(book));
  fclose(fptr);
    printf("to add a book press 1\n");
    printf("to delete a book press 2\n");
    printf("to find a book press 3\n");
    printf("to print all books press 4\n");
    printf("to save library in a file press 5\n");
    printf("to add books from a file press 6\n");
    printf("to exit press 0\n");
    pick(books);
    free(books);
    return 1;
}

i send an array of dynamicly allocated structs into a funtction that let me pick now when i call to print the books

void printbooks(book* books){
    int i;
    for(i=0;i<bookcount;++i){
        printf("%s\n",books[i].title);
    }
    printf("Fin\n");
    pick(books);
}

i get an expected output how ever when i use the function addbook

void addbook(book* books){
    char tempstring[maxsize];
    gets(tempstring);
    book* temp;
    int i=bookcount-1,ch;
    ++bookcount;
    if(libsize < bookcount){
    while(libsize < bookcount){
    libsize*=1.5;}
    temp=(book*)realloc(books,libsize);
    }
    if(temp==NULL){
        printf("no more space\n");
        exit(-1);}

    if(temp!=NULL){
        books=temp;}

    printf("add the id\n");
    gets(tempstring);
    ch=strlen(tempstring);
    books[i].id=(char*)malloc(ch+2);
    strcpy(books[i].id,tempstring);
    printf("add the title\n");
    gets(tempstring);
    ch=strlen(tempstring);
    books[i].title=(char*)malloc(ch+2);
    strcpy(books[i].title,tempstring);
    printf("add the author\n");
    ch=strlen(tempstring);
    books[i].author=(char*)malloc(ch+2);
    gets(tempstring);
    strcpy(books[i].author,tempstring);
    printf("add the pages\n");
    gets(tempstring);
    ch=strlen(tempstring);
    books[i].pages=(char*)malloc(ch+2);
    strcpy(books[i].pages,tempstring);
    printf("add the year\n");
    gets(tempstring);
    ch=strlen(tempstring);
    books[i].year=(char*)malloc(ch+2);
    strcpy(books[i].year,tempstring);
    printf("add the subject\n");
    gets(tempstring);
    ch=strlen(tempstring);
    books[i].subject=(char*)malloc(ch+2);
    strcpy(books[i].subject,tempstring);

    printf("book number %d added",bookcount);
    printf("\n");
    pick(books);
}

some of the struct members gets corrupted and the program crash, also when i tried to change the library size to 20 and then run it once i tried to enter the first member of the new book the program crashed.

Omer Guttman
  • 141
  • 8
  • 1
    Read this https://ericlippert.com/2014/03/05/how-to-debug-small-programs/ – StoryTeller - Unslander Monica Apr 06 '17 at 11:53
  • 3
    This wall of code is quite unreadble. Use empty rows as a way to format code, keeping non-related parts separated. Also, there is no function called `gets` in the C language. Whoever taught you to use it is bad. – Lundin Apr 06 '17 at 11:56
  • Standard C (as of C11) no longer has a function `gets()` because [`gets()` is too dangerous to be used — ever!](http://stackoverflow.com/questions/1694036/why-is-the-gets-function-dangerous-why-should-it-not-be-used) However, it is widely available in C libraries. Even though it is widely available, you should assume that it is implemented as `char *gets(char *str) { abort(); }`. That's safer than the normal implementation that attempts to read stuff from standard input. – Jonathan Leffler Apr 06 '17 at 22:04
  • when calling any of the heap allocation functions (malloc, calloc, realloc) 1) In C, do not cast the returned value. The returned value has type `void*` so can be assigned to any other pointer. Casting just clutters the code, making it more difficult to understand, debug, etc 2) always check (!=NULL) the returned value to assure the operation was successful. Special case: check the returned value BEFORE assigning to the target pointer, Otherwise, if `realloc()` fails, the original pointer will be lost, resulting in a memory leak. Note: the posted code failed to check `realloc()` – user3629249 Apr 07 '17 at 19:35
  • this kind of code block: `if(fptr==NULL) return-1;` would be much better written as: `if( !fptr ) { perror( "fopen to read file failed"); exit( EXIT_FAILURE );` as that would result in an appropriate exit AND would output to `stderr` both the enclosed message string AND the reason the OS thinks the operation failed. – user3629249 Apr 07 '17 at 19:40
  • for ease of readability and understanding: 1) consistently indent the code: indent after every opening brace '{'. unindent before every closing brace '}'. Suggest each indent level be 4 spaces as that is visible even with variable width fonts and allows for many indent levels across the page. 2) separate code blocks (for, if, else, while, do...while, switch, case, default) via a single blank line. 3) separate functions by 2 or 3 blank lines (be consistent) – user3629249 Apr 07 '17 at 19:43
  • the code block to count the number of entries in the book file can be off by one if the file does not end with a '\n', Suggest using `fgets() instead, where each successful call is one line in the input file (of course, the input buffer must be longer than the longest line in the file – user3629249 Apr 07 '17 at 19:46
  • why open the same file more than once, just do something like: `rewind( fptr );` – user3629249 Apr 07 '17 at 19:48
  • the value `maxsize` is not defined within the scope of the posted code. – user3629249 Apr 07 '17 at 19:50
  • the posted code is missing the `#include` statements for the needed header files. Do you expect us to guess as to which header files you code uses? – user3629249 Apr 07 '17 at 19:51
  • the variable `ch` is being used for two different purposes 1) a char read from the book file 2) the length of a string. This multiple usage of a variable is a poor programming practice Suggest using a unique variable. Also, the function: `strlen()` returns a `size_t` value, NOT a `int` value. – user3629249 Apr 07 '17 at 19:56
  • when obtaining a pointer via `strtok()`, always check (!=NULL) that the operation was successful – user3629249 Apr 07 '17 at 19:58
  • appropriate horizontal spacing, like inside parens and after commas, makes the code much more readable. – user3629249 Apr 07 '17 at 19:59
  • a menu is displayed toward the bottom of the posted code, but no response ever requested from the user, nor any code to process such a response. Note: the 'selection choice' of each menu item is best displayed in a vertical column, not at some random location off to the right of each menu item – user3629249 Apr 07 '17 at 20:04
  • the posted code contains one call to to `free()` for 'books', but is missing all the needed calls to `free()` for the many calls to `malloc/realloc` – user3629249 Apr 07 '17 at 20:05
  • the posted code calls a function: pick() but is missing the needed prototype statement . Similar concerns apply to the functions: `addbook()` and `printbooks()` – user3629249 Apr 07 '17 at 20:13
  • for readability, never write lines like this: `libsize*=1.5;}` instead, place the closing brace on the next line. – user3629249 Apr 07 '17 at 20:16
  • in function: `addbook()`, this code block: ` if(temp!=NULL) { books=temp; }` can be reduced to the statement: `books = temp;` because the prior code block has already eliminated the possibility of `temp` being NULL. – user3629249 Apr 07 '17 at 20:23

1 Answers1

0

In addbook

void addbook(book* books){
    ..
    libsize*=1.5;}
    temp=(book*)realloc(books,libsize);
    }

the realloc call does not resize your array properly (you do it properly in another function). It should include type size:

temp = realloc(books,libsize*sizeof(book));

otherwise it shrinks the size and you get corruption/undefined behaviour because memory is reused somewhere else for instance.

General note: those allocation functions are tricky and error prone: better put them in utility functions and only use the functions, don't copy/paste such code a million times (ex: malloc(ch+2) allocates 1 byte too much, a lot of times). In the case of your code, it could use a review.

Jean-François Fabre
  • 137,073
  • 23
  • 153
  • 219