So anyways, ive been stuck on recover for like a month because of some issues, im completely lost and id appreciate some feedback on my program. here are the issues: 1. the program only finds 4 jpegs 2. when a jpeg is is found i think the entire card is copied to it 3. the program only opens 1 jpeg (jpg1)

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

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

    // Remember filenames (card)
    char *incard = argv[1];

    // open the card and make a pointer called "pCard" that points to its address.
    FILE *pCard = fopen(incard, "r");

    // TEST - Ensure i dont have NULL value:
    if (pCard == NULL)
        fprintf(stderr, "Could not open %s.\n", incard);
        return 2;
    // making array that store the signature headers, start and end.
    unsigned char startBytes[] = {0xff, 0xd8, 0xff};
    unsigned char forthByte[] = {0xe0, 0xe1, 0xe2, 0xe3, 0xe4, 0xe5, 0xe6, 0xe7,
    0xe8, 0xe9, 0xea, 0xeb, 0xec, 0xed, 0xee, 0xef};
    int k = 0;
    // loop to find the headers
    for(int i = 0; i < 512; i++)
        // make a loop that compares currentByte to the first 3 bytes!
        for (int j = 0; j < 3; j++)

            // getting curser location
            int curloc = ftell(pCard);
            //printf("(cur loc: %i) ", curloc);
            // getting current byte and storing it in "currentByte "
            unsigned char currentByte = fgetc(pCard);
            // var describing the number of header bytes found
            int headerBytesFound = j + 1;

            // finding begining of jpeg
            if(currentByte == startBytes[j])
                // Printing info
                printf(" \n### FOUND A HEADER BYTE in: ### ");
                printf("\n | currentByte = %u cur Loc = %i | ", currentByte , curloc);

                //checking if found the 3 first bytes
                if(headerBytesFound == 3)
                    // alert for found a new header
                    printf("\n### FOUND FIRST 3 HEADER BYTES ###\n");

                    // moving the curser back for some reason..
                    fseek(pCard, -1, SEEK_CUR);

                    // open a jpeg file
                    //      and make a for loop that copies 512 bytes at a time to a file until the end header is found.
                    //int count = 0;

                    // making an array that stores the bytes to copy into the new file
                    unsigned char bytes[512];

                    // open a .jpg file
                    FILE * pJpg1 = fopen("jpg1", "w");

                    bool foundHeader = false;
                    while(foundHeader == false)
                        // checking to see if found end bytes
                        currentByte = fgetc(pCard);
                        for(int m = 0; m < 16; m++)
                            if(currentByte == forthByte[m])
                                printf("\nFOUND END BYTES\n");
                                foundHeader = true;
                        fseek(pCard, -1, SEEK_CUR);
                        // read and write into new .jpg
                        fread(bytes, 1, 512, pCard);
                        fwrite(bytes, 1, 512, pJpg1);
                    printf("\n ### DIDNT FOUND FIRST 3 BYTES ### ");

                fseek(pCard, 511, SEEK_CUR);
                // make the curser move enough steps in order to skip
                // to next jpeg, ie if it moved 2 bytes, move the curser
                // 510 bytes.

    // close card


This code has a lot of issues.

First, when a header is found, the code should close the current output file, if any. Then it should open a new, correctly named output file. Reopening jpeg1 just overwrites any existing file, and it doesn't conform to the file naming convention described in the spec. Files should be named sequentially, i.e., 000.jpg, 001.jpg, etc. This is why you're only getting one output file.

Next, don't overthink it. Your code does a lot of things that just make the process too complicated. Complicated usually means lots of opportunity for problems. Keep it simple! Just read in 512 bytes to a buffer, check that the first 4 bytes meet the spec for a signature (this can be done with a single IF statement), and decide whether an old file needs to be closed and a new file opened. Then, decide what to do with the 512 bytes! They either get written out to a file or discarded. It's just that simple!

Also, what about 512 byte blocks that don't have a signature? As stated in the program spec, all the blocks before the first signature are garbage to be discarded. Once the first signature is found, all the remaining data is image file data. BUT, not every block will have a signature. They are continuing data of the same file!

Hopefully this will help you understand the problem and give you insight on how to rewrite your program. ;-)

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

  • omg thank you so much, finally, i started from scratch and this time it only took me 2 hours. – Adrian Jerusalmi Aug 24 '19 at 20:15

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .