Pset4 recover.c compiles, but can't seem to print Jpgs at all

Question

Am starting a new thread as I'm having trouble understanding what is wrong with the code.. I've added printf statements at the jpgCheck function and did not get a print. I suspect it's the function?

Is there any problems with my logic? (included pseudocode if it help/is faster)

/**
 * recover.c
 *
 * Computer Science 50
 * Problem Set 4
 *
 * Recovers JPEGs from a forensic image.
 */

#include <stdio.h>
#define MEMORYBLOCK 512
#include <cs50.h>
#include "jpg.h"

bool jpgCheck(BYTE data[], BYTE jpgSig[]);

int main(void)
{
    //declare variables
    BYTE store[MEMORYBLOCK];       //int == 4 bytes, we want, 1 byte 

    //chars of arrays for sprintf
    char title[sizeof "###.jpg"];


    //filename counter
    int fileNameCounter = 0;

    //found jpg yet?
    bool foundJpg = false;

    //firstPass = true
    bool firstPass = true;

    bool storeIsJpg = false;


    // JPEG signature
    BYTE jpgSig[3] = { 0xff, 0xd8, 0xff };


    //MY PSEUDOCODE


//solution 3 (I think this is it..!!.. [update: 7-Dec-2016] just kidding.. not it.)
//once open
   FILE* inptr = fopen("card.raw", "r");
    if(inptr == NULL)
    {
        printf("could not open file\n");
        fclose(inptr);
        return 1;
    }

    //size of card
    fseek(inptr, 0, SEEK_END);
    int cardSize = ftell(inptr);

        //for each block in the card
        for(int h = 0, noOfBlocks = cardSize/512; h < noOfBlocks; h++)
        {
            //for within each 512 memory block
            for(int i = 0; i < MEMORYBLOCK; i++)
            {
                //read & store 512bytes in memory[i]
                fread(&(store[i]), 1, 1, inptr);

            }

            if(jpgCheck(store, jpgSig) == true)
            {
                storeIsJpg = true;
            }


            //if store[1],[2],[3],[4] is jpg && already found jpg
            if((storeIsJpg == true) && (foundJpg == true))
            {
                //fileNameCounter ++
                fileNameCounter++;
                //foundJpg = false
                foundJpg = false;
                //create a new file, name it accordingly
                sprintf(title, "%03d.txt", fileNameCounter);
                //open the newly created file
                FILE* outptr = fopen(title, "w");
                if(outptr == NULL)
                {
                    printf("could not create file\n");
                    fclose(outptr);
                    return 2;
                }

            }
            //if store[1],[2],[3],[4] is jpg || already found jpg
            if((storeIsJpg == true) || (foundJpg == true))
            {    //foundJpg = true
                foundJpg = true;
                //if(firstPass = true)
                    //open the newly created file
                    FILE* outptr = fopen(title, "w");
                    if(outptr == NULL)
                    {
                        printf("could not create file\n");
                        fclose(outptr);
                        return 2;
                    }

                if(firstPass == true);
                {
                    //create a new file, name it accordingly                     //fopen("fileNameCounter.jpg", "w")
                    //create a new file, name it accordingly
                    sprintf(title, "%03d.txt", fileNameCounter);

                    //firsPass = false
                    firstPass = false;
                }   
                //write current memory[i] into output                   ////fwrite(&(store[i]), 1, 1, outptr);
                 for(int i = 0; i < MEMORYBLOCK; i++)
                {
                    fwrite(&(store[i]), 1, 1, outptr);
                }
            }   
        }


    //remember to close opened files and free memory(if any)
    fclose(inptr);
}

//function to JpgCheck
bool jpgCheck(BYTE data[], BYTE jpgSig[])
{
    for (int i = 0; i < 3; i++)
    {
        if ((data[i]) != (jpgSig[i]))
        {
            return false;
        }
    }

    if (data[3] != 0xe0 || data[3] != 0xe0 || data[3] != 0xe1 || data[3] != 0xe2 || data[3] != 0xe3 || data[3] != 0xe4 || data[3] != 0xe5
     || data[3] != 0xe6 || data[3] != 0xe7 || data[3] != 0xe8 || data[3] != 0xe8 || data[3] != 0xe9 || data[3] != 0xea || data[3] != 0xeb
      || data[3] != 0xec || data[3] != 0xed || data[3] != 0xee || data[3] != 0xef)
    {
        return false;
    }
    return true;
}

The jpg.h file

#include <stdint.h>
typedef uint8_t BYTE;

Below is my pseudo code to ease the reading (if required):

//solution 3 
//once open
    //firstPass = true
        //for each block in the card                        //for(int h = 0, noOfBlocks = cardSize/512; h < noOfBlocks; h++)
            //for within each 512 memory block                        //for(int i = 0; i < MEMORYBLOCK; i++)
                //read & store 512bytes in store[i]         //fread(&(store[i]), 1, 1, inptr);

            //if this 512 block is jpg && already found jpg
                //fileNameCounter ++
                //foundJpg = false
                //create a new file, name it accordingly                     //fopen("fileNameCounter.jpg", "w")

            //if this 512 block is jpg  || already found jpg
                //foundJpg = true

                //if(firstPass = true)
                    //create a new file, name it accordingly                     //fopen("fileNameCounter.jpg", "w")
                    //firsPass = false

                    //write current memory[i] into output                   ////fwrite(&(store[i]), 1, 1, outptr);

//close files

Answer 1

Just to be crystal clear:

if (data[3] != 0xe0 || data[3] != 0xe0 || data[3] != 0xe1 || data[3] != 0xe2 || data[3] != 0xe3 || data[3] != 0xe4 || data[3] != 0xe5 || data[3] != 0xe6 || data[3] != 0xe7 || data[3] != 0xe8 || data[3] != 0xe8 || data[3] != 0xe9 || data[3] != 0xea || data[3] != 0xeb || data[3] != 0xec || data[3] != 0xed || data[3] != 0xee || data[3] != 0xef) { return false; }

will return false as soon as it matches the first data[3] != 0xe0.

And if the byte is 0xe0, it will return false as soon as it matches data[3] != 0xe1.

The flow of execution will never reach data[3] != 0xe2, because your function will always return false after one of the first two conditional expressions.

So, no matter what the fourth byte is, the code you have written will return false -- even if the buffer really contains the beginning of a new JPEG with a valid signature in the first four bytes.

If you really want to write a conditional expression to evaluate whether data[3] is part of a valid JPEG signature, it can be much simpler than this. Since the acceptable values are a contiguous range, you can check whether data[3] < 0xe0 || data[3] > 0xef.

You don't need to declare a bool for values like storeIsJpg and foundJpg because you don't need to store those values, you can just act on them. If I were you, I would throw away all of the code that uses these boolean variables, and just focus on modifying the flow of execution based on whether isJpg() returns true or false.

Storing this value in a bool and then checking the bool later, instead of checking the function's return value directly, is error-prone. Can you see why?

More generally, to streamline your code, don't try to check whether the start of a buffer is definitely a JPEG. Just test to see whether it is definitely not a JPEG.

If it fails any of your tests, then it is definitely not a JPEG, so just skip it and move on to the next buffer.

If it doesn't fail any of your tests, then it might be a JPEG, so start processing that buffer as if it is a JPEG, and keep going until you get to another buffer that doesn't fail any of your tests and might be the start of another JPEG.

This advice is (mostly) copied from my comments on your earlier question.

Answer 2

I tried to isolate some of the codes in modular form and created a file just to check this code. It seem to be the problem.

Did I declare the function within the if statement wrongly?

#include <stdio.h>
#include "jpg.h"
#include <cs50.h>

bool jpgCheck(BYTE data[], BYTE jpgSig[]);

int main(void)
{
    BYTE jpgSig[1] = { 0xff };
    BYTE data[1] = { 0xff };


    if(jpgCheck(data, jpgSig) == true)          //this line seem to be the problem.. It doesn't return my printf even thought it's true..
    {
        printf("matched cause true!\n");
    }
}




bool jpgCheck(BYTE data[], BYTE jpgSig[])
{
    for (int i = 0; i < 1; i++)
    {
        if ((data[i]) != (jpgSig[i]))
        {
            return false;
            printf("returned false\n");
        }

        if (data[i] < 0xe0 || data[i] > 0xef)
        {
            return false;
            printf("returned false 2\n");
        }
    }
    return true;
}

*Edit: I basically only did one array to test it out. But it didn't work.