C program not terminating

Question

I use c programming to get jpeg data that was deleted on a memory card (represented by file card.raw). I am now trying to restore these jpeg files. The problem: my code compiles but it wont terminate. I thought about another condition for the while loop, but unfortunately i don't know how to do correctly. (tried EOF and feof)

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

int main(int argc, char *argv[])
{
// ensure proper usage
if (argc != 2)
{
    fprintf(stderr, "Usage: ./recover filename\n");
    return 1;
}

// open input file
FILE *inptr = fopen(argv[1], "r");
//error if unable to open
if (inptr == NULL)
{
    fprintf(stderr, "Could not open %s.\n", argv[1]);
    return 2;
}

//variables
char buffer[512] = {0};
char jpeg1[4] = {0xff, 0xd8, 0xff, 0xe0};
char jpeg2[4] = {0xff, 0xd8, 0xff, 0xe1};
int count = 0;
char name[8] = {0};
FILE *outfile;
int isopen = 0;

//do while end of file is not reached
while (fread(buffer,1, 512, inptr) > 0)
{
    //compare buffer to bytes
    if (memcmp(buffer, jpeg1,4) == 0    || memcmp(buffer, jpeg2,4) == 0)

    {
        //close old outfile if open
        if(isopen ==1)
        {
            fclose(outfile);
        }

        //name for next outfile
        count ++;
        sprintf(name, "%03d.jpg", count);

        //open outfile and catch errors
        outfile = fopen(name, "w");
        if (outfile == NULL)
        {
            printf("Error opening outfile.\n");
            return 3;
        }
        isopen = 1;

        // write the first 512 bytes
        fwrite(buffer, 1, 512, outfile);
    }
    //no new jpeg
    // if outfile is open
    if (isopen == 1)
    {
        fwrite(buffer, 1, 512, outfile);
    }

    //move reader of fread()
    fseek(inptr, 1, SEEK_SET);
}

//close files
fclose(inptr);
fclose(outfile);

// success
return 0;
}

Instead of `fread(...) > 0` you should use `fread(...) == 512` — Treyten Carey, Sep 22 '17 at 13:36
Try and avoid the use of *magic numbers* in your code (e.g. `512`). It is must easier (and easier to maintain) if you `#define NMEMB 512` at the top of your code. If you ever need to change the value, you have a single place to make the change and don't have to go picking through each `fread/fwrite` call in your code to change it `:)` It's also no surprise when someone looks at `man fread`, e.g. `size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);` — David C. Rankin, Sep 22 '17 at 13:58
That's one of the CS50 course questions. Read [ask], expecially the part about homework questions. — too honest for this site, Sep 22 '17 at 14:04
Note: With `fread()`, best to open the file in _binary_ mode `fopen(argv[1], "rb");` — chux - Reinstate Monica, Sep 22 '17 at 15:21
Tip: Avoid stinginess with buffers. `char name[8] = {0}; ... sprintf(name, "%03d.jpg", count);` This is a buffer overflow with `count` outside the range [-99...999]. Be prepared for much wider counts. Maybe `char name[21 /*64-bit int */ + 4 /* fmt */ + 1] = {0};`. By design you "know" the code will never need more than size 8 buffer, yet code is broke now, so maybe a large `count` is compounding the problem. — chux - Reinstate Monica, Sep 22 '17 at 15:31

gsamaras · Answer 1 · 2017-09-22T16:51:53.460

2

You loop indefinitely.

Remove

fseek(inptr, 1, SEEK_SET);

which resets the file pointer to the start of the file every time your loop gets executed.

PS: Try to avoid magic numbers, like 512. I would use a define like this #define NMEMB 512, instead.

edited Sep 22 '17 at 16:51

answered Sep 22 '17 at 13:37

gsamaras

71,951
46
188
305

Surely the problem is just the `fseek` which is resetting the read pointer to the start of the file. `fread` can return less than 512 bytes if it doesn't read in a full chunk, but that'd still need processing. – Chris Turner Sep 22 '17 at 15:20
The bit about `fread` is still off - if it reads 511 bytes, you'd exit the loop and miss out on the last chunk of data read in. – Chris Turner Sep 22 '17 at 16:51
1

Yup, looks much better now :) – Chris Turner Sep 22 '17 at 16:57

C program not terminating

1 Answers1