Why does this buffer point to unaddressable bytes?

Question

EDIT: Thanks to repliers I have changed fread to (...sizeof buffer, 1,...), but now I get this error in valgrind:

==2409== Invalid read of size 4
==2409==    at 0x51AB8D0: fread (iofread.c:41)
==2409==    by 0x4007B6: main (recover2.c:31)
==2409==  Address 0x5502000 is not stack'd, malloc'd or (recently) free'd
==2409== 
==2409== Use of uninitialised value of size 8
==2409==    at 0x51B8787: _IO_sgetn (genops.c:495)
==2409==    by 0x51AB93E: fread (iofread.c:42)
==2409==    by 0x4007B6: main (recover2.c:31)
==2409== 
==2409== Invalid read of size 8
==2409==    at 0x51B8787: _IO_sgetn (genops.c:495)
==2409==    by 0x51AB93E: fread (iofread.c:42)
==2409==    by 0x4007B6: main (recover2.c:31)
==2409==  Address 0x40 is not stack'd, malloc'd or (recently) free'd
==2409== 
==2409== 
==2409== Process terminating with default action of signal 11 (SIGSEGV)
==2409==  Access not within mapped region at address 0x40
==2409==    at 0x51B8787: _IO_sgetn (genops.c:495)
==2409==    by 0x51AB93E: fread (iofread.c:42)
==2409==    by 0x4007B6: main (recover2.c:31)
==2409==  If you believe this happened as a result of a stack
==2409==  overflow in your program's main thread (unlikely but
==2409==  possible), you can try to increase the size of the
==2409==  main thread stack using the --main-stacksize= flag.
==2409==  The main thread stack size used in this run was 8388608.

---

I'm new here so I hope this makes sense. I'm writing this code to retrieve data from a file and copy it out to jpeg files. The code is meant to find a jpg file by its header then write it to files. The code is:

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

int main(int argc, char* argv[])
{

FILE* file = fopen("card.raw", "r");
if (file == NULL)
{ 
    printf("Could not open file!\n");
    return 1;
}

char title[7];
int currentImage = 0;
uint8_t buffer[512];
FILE* img;
while (fread(buffer, sizeof(buffer), 512, file) == 1)
{
printf("found data!\n");
if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff)
{
    if (buffer[3] == 0xe0 || buffer[3] == 0xe1 || buffer[3] == 0xe2 || buffer[3] == 0xe3 || buffer[3] == 0xe4 || buffer[3] == 0xe5 || buffer[3] == 0xe6 || buffer[3] == 0xe7 || buffer[3] == 0xe8 || buffer[3] == 0xe9 || buffer[3] == 0xea || buffer[3] == 0xeb || buffer[3] == 0xec || buffer[3] == 0xed || buffer[3] == 0xee || buffer[3] == 0xef)
    {
        printf("Found new jpg!\n");
        sprintf(title, "%03d.jpg", currentImage);
        img = fopen(title, "a");
        currentImage++;
        printf("size of buffer to print is %lu\n", sizeof(buffer));
        fwrite(buffer, sizeof(buffer), 1, img);
        }
}
else if (currentImage > 0)
{ 
        fwrite(buffer, sizeof(buffer), 1, img);


}

}
}

I am getting a segmentation fault once it finds a jpeg, and does the fwrite, and then returns to the while loop.

The valgrind error is:

==1866== Syscall param read(buf) points to unaddressable byte(s)
==1866==    at 0x5228810: __read_nocancel (syscall-template.S:81)
==1866==    by 0x51B63B8: _IO_file_xsgetn (fileops.c:1438)
==1866==    by 0x51AB93E: fread (iofread.c:42)
==1866==    by 0x4007C3: main (recover2.c:31)
==1866==  Address 0xfff001000 is not stack'd, malloc'd or (recently) free'd
==1866== 
==1866== Jump to the invalid address stated on the next line
==1866==    at 0x0: ???
==1866==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==1866== 
==1866== 
==1866== Process terminating with default action of signal 11 (SIGSEGV)
==1866==  Bad permissions for mapped region at address 0x0
==1866==    at 0x0: ???

I'm new to this so still learning about memory and etc, so any help in understanding why it's gone wrong would be appreciated.

Answer 1

By doing this

fread(buffer, sizeof(buffer), 512, file)

you are asking fread to read 512 blocks, each block being sizeof(buffer) bytes long. I.e. you are trying to read 512*512 = 262144 bytes into an array declared as uint8_t buffer[512]. That certainly won't fit.

If you simply want to read data into the buffer array, that would be either

fread(buffer, sizeof buffer, 1, file)

or

fread(buffer, 1, sizeof buffer, file)

or, if you prefer,

fread(buffer, sizeof *buffer, sizeof buffer / sizeof *buffer, file)

depending on what you consider an "atomic" data block in your read operation.

---

Also

sprintf(title, "%03d.jpg", currentImage);

will generate a string at least 7 characters long (e.g. 001.jpg), which means that title has to be at least 8 characters long to accommodate zero terminator. However, your title is declared as

char title[7];

This is too small.

Answer 2

As per the man page of fread(), the signature is

size_t fread(void *ptr, size_t size, size_t nmemb, FILE * stream );

where the description is

The function fread() reads nmemb elements of data, each size bytes long, from the stream pointed to by stream, storing them at the location given by ptr.

So, your code should be

 while (fread(buffer, sizeof(buffer[0]), 512, file) == 1)

Otherwise, you're ending up asking to read and store 512 number of blocks of size 512 bytes each, which is wrong and will cause buffer overflow, as reported by valgrind.