Segmentation fault and only one unreadable image - RECOVER

Question

I'm getting a segmentation fault and only one, unreadable image (and some other temp nonsense files) when running the following code:

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

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

if (argc != 2)
{
    fprintf(stderr, "Usage: ./copy infile\n");
    return 1;
}

FILE *infile = fopen(argv[1], "r");
typedef uint8_t BYTE;
BYTE buffer [512];
char filename [3];
int filesfound = 0;
int startfound = 0;

// NEED TO FIND

while (infile!=NULL){
    if (fread(buffer, 512, 1, infile)){
        // check if start of that block is the header!
        if ((buffer[0] == 0xff) && (buffer[1] == 0xd8) && (buffer[2] == 0xff) && ((buffer[3] & 0xf0) == 0xe0)){
            // mark the start of the first JPEG
            startfound = 1;
            // create new file
            sprintf(filename, "%03i.jpg", filesfound);
            // increment name for next file
            filesfound++;
            // open file
            FILE *img = fopen(filename, "w");
            // write buffer to file
            fwrite(buffer, 512, 1, img);
            }

        else if (startfound == 1){
            // if we already found file previously, open file
            FILE *img = fopen(filename, "w");
            // write to file
            fwrite(buffer, 512, 1, img);
        }
    }
}
}

Valgrind is showing the following but I'm not adept enough at using it to properly interpret.

==6258== Memcheck, a memory error detector
==6258== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==6258== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==6258== Command: ./recover card.raw
==6258== 
==6258== Invalid read of size 4
==6258==    at 0x5E28CD0: fwrite (iofwrite.c:41)
==6258==    by 0x42DA3C: main (recover.c:43)
==6258==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==6258== 
==6258== 
==6258== Process terminating with default action of signal 11 (SIGSEGV)
==6258==  Access not within mapped region at address 0x0
==6258==    at 0x5E28CD0: fwrite (iofwrite.c:41)
==6258==    by 0x42DA3C: main (recover.c:43)
==6258==  If you believe this happened as a result of a stack
==6258==  overflow in your program's main thread (unlikely but
==6258==  possible), you can try to increase the size of the
==6258==  main thread stack using the --main-stacksize= flag.
==6258==  The main thread stack size used in this run was 8388608.
==6258== 
==6258== HEAP SUMMARY:
==6258==     in use at exit: 1,136 bytes in 2 blocks
==6258==   total heap usage: 3 allocs, 1 frees, 1,704 bytes allocated
==6258== 
==6258== LEAK SUMMARY:
==6258==    definitely lost: 0 bytes in 0 blocks
==6258==    indirectly lost: 0 bytes in 0 blocks
==6258==      possibly lost: 0 bytes in 0 blocks
==6258==    still reachable: 1,136 bytes in 2 blocks
==6258==         suppressed: 0 bytes in 0 blocks
==6258== Reachable blocks (those to which a pointer was found) are not 
shown.
==6258== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==6258== 
==6258== For counts of detected and suppressed errors, rerun with: -v
==6258== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Segmentation fault

Answer 1

For "%03i.jpg", you need at least 8 char (7 for the file name like 000.jpg, and one for the null terminator).

DO NOT open a file in your else path, it just makes no sense, especially with a filename variable which initially can contain arbitrary data.

Also, I don't see any way that infile could become NULL. Either it already is (then you should complain about missing input file and end the programme immediately), or it isn't and won't become.

You can use fread(buffer, 512, 1, infile) as the while condition instead of an extra if. And maybe close any open files as soon as you no longer need them (close output files before opening next output file, close input file and any open output file after loop).