Infinite loop with fread

Question

I'm trying to allocate an array 64 bytes in size and then loop over the array indexes to read a byte each from the inputfile. but when I don't malloc() the array indexes, the loop stays in index0 (so each time it loops, it replaces the content in index0 with the next byte, instead of putting each byte in the next array index and keeping them all chronological).

When I use malloc() it uses the array indexes properly, but it's an infinite loop and uses gigs of ram.

Here's my code:

struct BitIO {
      FILE        *FilePointer;
      uint8_t      *Buffer[64];
      uint64_t   BitsAvailable;
      uint64_t BitsUnavailable;
} BitIO;


void Init_BitIO(const char *FileName, const char *Mode) {
     BitIO.FilePointer = fopen(FileName, Mode);
     malloc(sizeof(BitIO));
     while (!feof(BitIO.FilePointer)) {
         size_t BytesRead = 0;
         for (int i = 0; i < 64; i++) {
             BitIO.Buffer[i] = (uint8_t*)malloc(1);
             BytesRead = fread(BitIO.Buffer[i], 1, 1, BitIO.FilePointer);
        }
    }
}

Answer 1

If you're "trying to allocate an array 64 bytes in size", you may consider

uint8_t      Buffer[64];

instead of

uint8_t      *Buffer[64];

(the latter is an array of 64 pointers to byte)

After doing this, you will have no need in malloc as your structure with a 64 bytes array inside is allocated statically. The 'main' loop will look like

for (int i = 0; i < 64; i++) {
     BytesRead += fread(&BitIO.Buffer[i], 1, 1,BitIO.FilePointer);
}

But, of course, I would advise a more efficient form:

BytesRead = fread(BitIO.Buffer, 1, 64, BitIO.FilePointer);

Answer 2

• Point 1

  You need to collect the return value of malloc() into some variable (and check for malloc() success before you use the returned pointer) to make use of the allocated memory. Then, seeing your usage, I believe you're confused with the struct member variable types. You don't need a uint8_t *Buffer[64]; as the struct member based on your usage.

  1.1. If you want to use dynamic memory, then, change the structure member as

  uint8_t *Buffer;
  

  and inside for loop you do

  BitIO.Buffer[i] = malloc(sizeof(uint8_t));   //allocate memory
  BytesRead = fread(BitIO.Buffer[i], 1, 1,BitIO.FilePointer);
  

  or, better, as you're looping for fixed number of time, you can get the memory allocated at one time outside the for loop

  BitIO.Buffer = malloc( 64 * sizeof(uint8_t));
  

  and then loop to read one element at a time.

  1.2. Otherwise, change the structure member as

  uint8_t Buffer[64];
  

  and get rid of the malloc() completely.

• Point 2:

  Read Why is “while ( !feof (file) )” always wrong?

• Point 3:

  Please see why not to cast the return value of malloc() and family in C.