2

I'm trying to solve "recover" but, I'm unable to compare my if statements. This is my error message:

error: comparison of constant 224 with expression of type 'char' is always false [-Werror,-Wtautological-constant-out-of-range-compare] if(store[3] == 0xe0 || store[3] == 0xe1 || store[3] == 0xe2 || store[3] == 0xe3 || store[3] == 0xe4 || store[3] == 0xe5 || store[3...

Should I be changing my store[i] to char.. or the hex to int.. How do I compare them..?

In Pset4/resize, they used structs and was able to access each byte, how can i do the same without using structs..?

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

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

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

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

    //could the 4th byte belong to a jpg?
    bool fourthByteJpg = false;

    //filename counter
    int fileNameCounter = 0;

    //found jpg?
    bool foundJpg = false;

    //firstPass = true
    bool firstPass = true;

    bool foundFirstThreeBytes = false;
    bool storeIsJpg = false;
    //MY PSEUDOCODE






//solution 3 (I think this is 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(&(memory[i]), 1, 1, inptr);


                //read & store first four bytes into store[i]
                if(i < 4)
                {
                    fread(&(store[i]), 1, 1, inptr);
                }
            }
            //define if store is jpg
            if(store[3] == 0xe0 || store[3] == 0xe1 || store[3] == 0xe2 || store[3] == 0xe3 || store[3] == 0xe4 || store[3] == 0xe5 || store[3] == 0xe6 ||
            store[3] == 0xe7 || store[3] == 0xe8 || store[3] == 0xe8 || store[3] == 0xe9 || store[3] == 0xea || store[3] == 0xeb || store[3] == 0xec || store[3] == 0xed
             || store[3] == 0xee || store[3] == 0xef)
             {
                 fourthByteJpg = true;
             }
             if(store[0] == 0xff && store[1] == 0xd8 && store[2] == 0xff)
             {
                 foundFirstThreeBytes = true;
             }

            if((foundFirstThreeBytes == true) && (fourthByteJpg == 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(&(memory[i]), 1, 1, outptr);
                }
            }   
        }


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

Version 2:

I added the functions as Matt Obert has suggested and still, it isn't getting the jpgs out..

/**
 * 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?
    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
  • To streamline the code, don't try to check whether the start of a buffer definitely is a JPEG. Just test to see if it definitely is not a JPEG. If it is not a JPG, 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 JPG, and keep going until you get to another buffer that doesn't fail any of your tests and might be another JPG. – Matt Obert Dec 6 '16 at 20:00
3

@Blauelf is correct: you can't count on char to be an unsigned 8-bit int.

However, you can use a typedef to make certain that a BYTE is really a byte.

Look at these type definitions, borrowed from bmp.h:

#include <stdint.h>

/**
 * Common Data Types 
 *
 * The data types in this section are essentially aliases for C/C++ 
 * primitive data types.
 *
 * Adapted from http://msdn.microsoft.com/en-us/library/cc230309.aspx.
 * See http://en.wikipedia.org/wiki/Stdint.h for more on stdint.h.
 */
typedef uint8_t  BYTE;   // You can use this one in recover.c
typedef uint32_t DWORD;
typedef int32_t  LONG;
typedef uint16_t WORD;

Note: I just looked back at your other question and saw that you are already using this in your jpg.h file. It would be sufficient to include the typedef directly in recover.c and avoid defining another struct here.

In other news, you're still over-complicating your approach. For example, if you are reading your data stream into a 512-byte buffer, you do not need to copy the first four bytes of your buffer into another array before comparing them to your JPEG signature. You can just compare the first four bytes of your buffer to the JPEG signature directly.

There's more than one way to do this. Here's one way to approach it using an array:

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

Then, you could write a function to check the first three bytes like so:

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

This is an incomplete solution (it still does not check for all the possible values of the fourth byte) but it should be enough to get you started.

Don't forget to accept the most helpful answer to each of your questions by clicking the green check mark; otherwise they will always be marked as unanswered questions, and they will clutter the forum. Fight Zombie Questions!

  • Hey Matt thanks so much for that tip. I've incorporated it. My code looks a bit cleaner, but it seem to do the same thing. Is that the only thing i over complicated?.. I've went through my code line by line again, but can't seem to spot the problem.. Would you help me out? – olafironfoot Dec 6 '16 at 18:29
  • It's not the only thing you've over-complicated. One example is that you don't need a bool for foundJpg. Instead, just call the jpgCheck() function! If it returns false, move on to the next 512-byte buffer; if it returns true, process the file. You should throw away extra steps like this to streamline your code, or start from scratch with just the jpgCheck() function and rewrite main() to do only what is necessary. – Matt Obert Dec 6 '16 at 18:50
  • 1
    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. But since you asked, your implementation of the fourth byte check will always return false. (That's why the printf() statements after that don't run.) Just comment it out and see how far you can get by checking only the first three bytes. The goal of jpgCheck() is mainly to identify buffers that are definitely not the start of a new JPG, so they can be skipped. Perhaps testing the first three bytes will get you most of the way there! – Matt Obert Dec 6 '16 at 19:10
  • 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. – Matt Obert Dec 6 '16 at 20:59
  • 1
    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. – Matt Obert Dec 6 '16 at 21:03
2

char can be signed or unsigned by default (and is 8-bit on every system I know, but that's not guaranteed by the specs).

On this system, char datatype is signed 8-bit int, ranging from -128 to 127, and 224 is outside that range. You can use unsigned char or BYTE instead, which ranges from 0 to 255.

  • Thanks blaulf. It compiles! But it doesn't work still for some reason.. – olafironfoot Dec 6 '16 at 17:29

You must log in to answer this question.

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