Search code examples
cjpegcs50

Pset4 (cs50) recover does not work properly. It compiles, but does not recover more than 2 jpegs. Is something wrong with checking for JPEG signature?


I am learning how to code and I have no experience with that at all. I've successful got to PSET4 and stuck on recover. I've read everything online about this problem and i found out that many people have similar code as I do and it works. Does not work for me whatsoever. Please have a look and give me a hint what did I do wrong and how to correct it. Here is everything about the pset4 recover i downloaded their card.raw from here card.raw

/** recovering JPEG files from a memory card
* 
*/
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>

typedef uint8_t BYTE;

int main(int argc, char* argv[])
{
    // ensure proper usage
    if (argc != 2)
    {
        fprintf(stderr,
            "Usage: ./recover infile (the name of a forensic image from which to recover JPEGs)\n");
        return 1;
    }

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

    FILE* outptr = NULL;

    // create a pointer array of 512 elements to store 512 bytes from the memory card
    BYTE* buffer = malloc(sizeof(BYTE) * 512);
    if (buffer == NULL)
    {
        return 3;
    }

    // count amount of jpeg files found
    int jpeg = 0;

    // string for a file name using sprintf
    char filename[8] = { 0 };

    // read memory card untill the end of file
    while (fread(buffer, sizeof(BYTE) * 512, 1, inptr) != 0)
    {
        // check if jpeg is found
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff
            && (buffer[3] >= 0xe0 || buffer[3] <= 0xef))
        {
            if (jpeg > 0)
            {
                fclose(outptr);
            }
            sprintf(filename, "%03d.JPEG", jpeg);
            outptr = fopen(filename, "w");
            jpeg++;
        }

        if (jpeg > 0)
        {
            fwrite(buffer, sizeof(BYTE) * 512, 1, outptr);
        }
    }

    // free memory
    free(buffer);

    // close filename
    fclose(outptr);

    // close input file (forensic image)
    fclose(inptr);

    return 0;
}

Solution

  • The main problem is that you invoke undefined behavior because filename is not enough big. sprintf() need be 9 and 17 bytes with your code but you only has 8. So you have a buffer overflow.

    Just change:

    char filename[8] = { 0 };
    

    to

    char filename[17] = { 0 };
    

    Because, you use an int, this value is implemented defined but in many system has an int with 32 bits. So the value possible are between -2^31 and 2^31 - 1 that make a maximum of 11 chars (-2147483648). We add the number of chars in ".JPEG", 5. We have 16 but you forget the null terminate byte of a c-string. So we are 17 maximum.

    Modern compiler warning you: gcc version 7.1.1 20170516 (GCC):

    In function ‘main’:
      warning: ‘sprintf’ writing a terminating nul past the end of the destination [-Wformat-overflow ]
      sprintf(filename, "%03d.JPEG", jpeg++);
                                  ^
    note: ‘sprintf’ output between 9 and 17 bytes into a destination of size 8
      sprintf(filename, "%03d.JPEG", jpeg++);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    

    Plus, your typedef is useless because a char world be always a byte in C. More than that you don't need a byte but an octet so like char, uint8_t would be always an octet in C. So you don't need typedef.

    Again one thing, you allocate your buffer but it's useless because your buffer has a constant size. So just create an array is more simple.

    #include <stdint.h>
    #include <stdio.h>
    
    int main(int argc, char *argv[]) {
      if (argc != 2) {
        fprintf(stderr, "Usage: ./recover infile (the name of a forensic image "
                        "from which to recover JPEGs)\n");
        return 1;
      }
    
      FILE *inptr = fopen(argv[1], "r");
      if (inptr == NULL) {
        fprintf(stderr, "Could not open %s.\n", argv[1]);
        return 2;
      }
    
      FILE *outptr = NULL;
      uint8_t buffer[512];
      size_t const buffer_size = sizeof buffer / sizeof *buffer;
      size_t jpeg = 0;
      while (fread(buffer, sizeof *buffer, buffer_size, inptr) == buffer_size) {
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff &&
            buffer[3] == 0xe0) {
          if (outptr != NULL) {
            fclose(outptr);
          }
    
          char filename[26];
          sprintf(filename, "%03zu.JPEG", jpeg++);
          outptr = fopen(filename, "w");
        }
        if (outptr != NULL) {
          fwrite(buffer, sizeof *buffer, buffer_size, outptr);
        }
      }
    
      if (outptr != NULL) {
        fwrite(buffer, sizeof *buffer, buffer_size, outptr);
      }
    
      if (outptr != NULL) {
        fclose(outptr);
      }
    
      fclose(inptr);
    }
    

    Note: This example is clearly not perfect, this will be better to make a true parser for jpeg file to have a better control flow. Here we suppose that all gonna be right.