5

I have a big problem that need's to be solved before I can continue with my program.

I have to open a binary file, read it's content, save the content into a buffer, allocate space on the heap with malloc, close the file and finally printf( the content of the .bin file). I came this far (closing file is not implemented yet):

void executeFile(char *path){
    FILE *fp; /*filepointer*/
    size_t size; /*filesize*/
    unsigned int buffer []; /*buffer*/

    fp = fopen(path,"rb"); /*open file*/
    fseek(fp, 0, SEEK_END); 
    size = ftell(fp);         /*calc the size needed*/
    fseek(fp, 0, SEEK_SET); 
    buffer = malloc(size);  /*allocalte space on heap*/

    if (fp == NULL){ /*ERROR detection if file == empty*/
        printf("Error: There was an Error reading the file %s \n", path);           
        exit(1);
    }
    else if (fread(&buffer, sizeof(unsigned int), size, fp) != size){ /* if count of read bytes != calculated size of .bin file -> ERROR*/
        printf("Error: There was an Error reading the file %s - %d\n", path, r);
        exit(1);
    }else{int i;
        for(i=0; i<size;i++){       
            printf("%x", buffer[i]);
        }
    }
}

I think I messed up the buffer and I am not really sure if I read the .bin file correctly because I can't print it with printf("%x", buffer[i])

Hope you guys can help

Greetings from germany :)

user2418126
  • 51
  • 1
  • 1
  • 3
  • 2
    Where is `size` being set? You have it commented out. – lurker May 24 '13 at 15:54
  • 1
    Just for that case when things don't work out *perfectly* (like, right **now** for example), you may wish to consider *checking the return value of the functions you call*. `fopen()`, `malloc()`, `fseek()`.. None are being checked, though you do eventually check for `fp == NULL`, but only after you blindly `fseek()` on it, and the NULL check appears to be a file-empty check (which isn't correct either) rather than failure-to-open. *Read the library documentation*. – WhozCraig May 24 '13 at 16:24
  • Commented `size` out per accident->fixed – user2418126 May 24 '13 at 16:33
  • 1
    I started writing up a detailed answer, but just do this: (1) Check return values, (2) Change buffer base type to `unsigned char *buffer;`. (3) Fix `fread()` call to pass `buffer` as the first parameter (not `&buffer`), and `sizeof(buffer[0])` as the second. and finally (4) close your file with `fclose()` and free your buffer with `free()`. Optional checking for `size == 0` before entering into the read-and-print I leave to you. – WhozCraig May 24 '13 at 16:39
  • 1. checked return values as far as i could ->everything is fine ?! 2. calculation of the filesize was given by my prof so this workes 3. fopen() returns FILE if correct and NULL if not, so checking if fp == NULL is a way of error-checking while opening the file 4. I did not inserted size == 0 check before priting out because it is just to test and see what's in the buffer. will be erased if everything is ok 5. i will use free() later on. I need to work with this buffer. – user2418126 May 24 '13 at 17:06
  • 1
    Also note that `fstat()` might be a better way to determine file size than multiple `fseek()` calls... – twalberg May 24 '13 at 17:09
  • I did everything that WhozCraig said I should do. I have a problem with my buffer. It need's a size when it's beeing initialized but I can't give it a size because idk how big the file is yet. it is calculated 4 lines under the declaration of my buffer. so what should I do ? – user2418126 May 24 '13 at 17:17
  • @twalberg I concur fstat() is a better than the pair of fseek() and ftell(). Either gets the job done, so my below answer focused on needed changes. – chux - Reinstate Monica May 24 '13 at 19:28
  • If you are using C99 (or C11), you could use a VLA (variable length array) by determining the size and then defining `unsigned int buffer[size / sizeof(int)];` in the middle of the function. That brings up another point; you are reading into an array of `int` from a file sized in terms of bytes (`char`, roughly); hence the compensating `/ sizeof(int)` in my suggestion. – Jonathan Leffler May 24 '13 at 19:29
  • unsigned int buffer[size / sizeof(int)]Using a buffer of type int, where sizeof(int) could be greater than 1 imposes an additional concerns. Since the file may not have a .> 1 – chux - Reinstate Monica May 24 '13 at 21:08
  • Please ignore previous comment. Using C99 or C11 and buffer of type int, where sizeof(int) could be greater than 1 imposes an additional concern. Since the file byte size may not be a multiple of sizeof(int), one would need to define a slightly larger "unsigned int buffer[(size + sizeof(int) -1)/ sizeof(int)];" to cope with pesky odd additional bytes. But a buffer of type int could work - just some issues. – chux - Reinstate Monica May 24 '13 at 21:15
  • should read files in chunk? what if the file is too large? – Shawn Shaw Feb 20 '22 at 22:39

1 Answers1

8

Recommended changes:

  1. Change your buffer to a char (byte), as the ftell() will report the size in bytes (char) and malloc() use the byte size also.

    unsigned int buffer []; /buffer/

to

unsigned char *buffer; /*buffer*/

  1. [Edit] 2021: Omit cast
    2) This is OK, size is bytes and buffer points to bytes, but could be explicitly cast

    buffer = malloc(size); /allocate space on heap/

to

buffer = (unsigned char *) malloc(size);  /*allocate space on heap*/
/* or for those who recommend no casting on malloc() */
buffer = malloc(size);  /*allocate space on heap*/

  1. change 2nd parameter from sizeof(unsigned int) to sizeof *buffer, which is 1.

    else if (fread(buffer, sizeof(unsigned int), size, fp) != size){

to

else if (fread(buffer, sizeof *buffer, size, fp) != size){

  1. Change "%x" to "%02x" else single digit hexadecimal numbers will confuse the output. E. g. is "1234" four bytes or two?

    printf("%x", buffer[i]);

to

printf("%02x", buffer[i]);

  1. Your clean-up at the end of the function may include

    fclose(fp); free(buffer);

chux - Reinstate Monica
  • 143,097
  • 13
  • 135
  • 256