Why is my recover.c solution writing invalid .jpg files?

Question

First, just to be clear: I know my solution is an abomination. I know of several problems with it, and I will be doing it the right way, with a buffer and a while loop and a check for EOF and all of that. But I'm stubborn, and wanted to prove to myself that I could in fact make the solution I came up with work.

The solution below succeeds at parsing card.raw, it finds 50 files, it creates 50 files named appropriately (I think, I'll have to review the specs), and it writes data to the files. It ends up creating files of sizes that seem appropriate (this version gives me an idea of how much data should go into each file by writing a "." to the terminal every time another 512mb is read that doesn't start a new .jpg file.)

But the files are all "invalid or unsupported file format" when I try to view them.

I went ahead and downloaded them and converted them to .txt, to try and compare outputs. Of course I realize that the possibility of a frame shift could cause reading the binary into symbols to make the .txts entirely different, which made this unlikely to be a valuable debugging method, but it did provide me with one finding: while card.raw is a mostly random series of nonrepeating symbols, each of my files is a mostly repeating set of symbols, with one line repeated many times with only slight variations.

I was hoping this meant I had some kind of error in resetting my pointer-- eg, perhaps I was writing the same 512mb over and over again to each file-- but I can't figure out why that would be happening from my code.

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <cs50.h>
// #define CARD_SIZE 17095168;

 typedef uint8_t  BYTE;

 typedef struct
{
    BYTE  first;
    BYTE  second;
    BYTE  third;
    BYTE  fourth;
} __attribute__((__packed__))
STARTBYTES;

int main(int argc, char* argv[])
{
    FILE* inptr = fopen("card.raw", "r");
    //should check for error opening file

    STARTBYTES sb;

    int match = 0;
    int pic_count = 0;
    int last_pic_count = 0;

    char title[16];
    FILE* outptr = fopen(title, "a");


    for(int i =0; i < 17095168/512 /*CARD_SIZE*/; i++){
        fseek(inptr, i*512, SEEK_SET);
        fread(&sb, sizeof(STARTBYTES), 1, inptr);
        fseek(inptr, - 4, SEEK_CUR);

        if (sb.first == 0xff && sb.second == 0xd8 && sb.third == 0xff && sb.fourth >= 0xe0 && sb.fourth <= 0xef){
            printf("\nFound an image at %i.\n", i * 512);
            match = 1;
            pic_count++;
        }
        else match = 0;

        //to save time, only run this part if pic_count has changed since it was last run
        if(pic_count > last_pic_count){

            sprintf(&title[0], "./images/%3d.jpg", pic_count);

        outptr = fopen(title, "w");
        last_pic_count++;
        }

        if (match == 1 && pic_count == 1){
            //on the first match, open a new file
            //write 512 bytes

            fwrite(inptr, 512, 1, outptr);


            printf("First match found. Writing data to %s\n", title);
        }

        if (match == 1 && pic_count > 1){
            //on subsequent matches, close the previous file
            //write 512 bytes
            fclose(outptr);
            outptr = fopen(title, "w");
            fwrite(inptr, 512, 1, outptr);
            printf("\nStarting a new file called %s!\n",title);

        }

        if (match == 0 && pic_count > 0){
            //after finding the first file, if the current 512mb 
            //doesn't match a new file start,
            //write to current open file
            printf(".");
            fwrite(inptr, 512, 1, outptr);
        }
    }
    fclose(inptr);

}

//skip 512 bytes. Read 4. reset read head. 
//if 4 bytes match, close the previous open file (if applicable) and read 512 bytes. Save to new file 
//if no match, read 512 bytes and add to previous image.


//for every 512byte chunk of the file:
//check for a match
//if no matches have ever been found, and the current chunk doesn't match, then increment i without writing
//on the first match found, open a new file and start writing to it. increment i.
//if previous matches have been found, and the current chunk doesn't match, then write to file and increment i
//if previous matches have been found, and current chunk does match, then close the old file, open a new one, and write to it. increment i

Answer 1

You can use the xxd tool to look at the first 4 bytes of your output files. I'll bet that they do not contain a proper signature.

I'm surprised your code works at all. When I ran it, it generated seg faults. The problem lies in the fwrite() calls. The fwrite() function takes data from a variable in memory and writes to a file. You cannot read directly from one file and write to another using fwrite() (or any other command that I know of). The data has to be read from the input file into memory, i.e., a buffer, and can then be written out to the output file.

Having said that, you're reading 4 bytes from the input file, checking the signature, repositioning the file pointer back 4 bytes, then reading 512 bytes. It's far more efficient to just read 512 bytes and check the first 4 bytes in memory. A file read (or almost any disk operation) can take as much as 1000 times longer, or more, than a memory read, depending on hardware configurations and system load. Minimizing file operations should always be a priority. Also, it takes about the same amount of time and resources to read 512 bytes as it does 4 bytes from disk because of the disk operation overhead. And if you simply read in one 512 byte block at a time, you don't need to worry about fseeks or need to know the size of the file. (Don't know how much of that you already knew, but I thought I'd throw it out there. ;-) )

If, however, you are actually running different code, then perhaps you can edit the question and change the code?

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance.