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);
        }
    }
}

`malloc(sizeof(BitIO));` ... That's a creative way to leak memory! — autistic, Jun 22 '15 at 09:07
Every time check a memory allocation or opening a file is successful ! — alifirat, Jun 22 '15 at 09:08

score 2 · Accepted Answer · answered Jun 22 '15 at 09:26

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);

score 0 · Answer 2 · edited May 23 '17 at 12:14

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.

I honestly don't know why I thought malloc would automagically put the returned memory where it should be, that's a really good point. — MarcusJ, Jun 22 '15 at 09:27

Infinite loop with fread

2 Answers2