0

I have been on this problem all day but I cannot seem to discover what is wrong with my code and why it won't pass on check 50. If I try to make changes I would get a segmentation fault and I cannot seem to figure out what I am missing. I have checked filename at initialization and it has sufficient memory for 8 char characters. I am lost on this one.

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>

int main(int argc, char *argv[])
{
    if (argc != 2)
    {
        printf("Usage: ./recover filename.exe\n");
        return 1;
    }
    FILE *file = fopen(argv[1], "r");
    if (file == NULL)
    {
        printf("Error: cannot open file\n");
        return 2;
    }
    typedef uint8_t BYTE;
    int counter = 0;
    char Filename[8];
    FILE *output = NULL;

    BYTE buffer[512] = {0};

    while(fread(buffer, 512, 1, file) == 1)
    {
        if(buffer[0] == 0xff || buffer[1] == 0xd8 || buffer[2] == 0xff || (buffer[3] & 0xf0) == 0xe0 || feof(file) == 0)
        {
            if(output != NULL)
            {
                fclose(output);
                sprintf(Filename,"%03i.jpg", counter);
                output = fopen(Filename, "w");
                fwrite(buffer, 512, 1, output);
                counter++;
            }
            else
            {
                sprintf(Filename,"%03i.jpg", counter);
                output = fopen(Filename, "w");
                fwrite(buffer, 512, 1, output);
                counter++;
            }
        }
        if(output != NULL)
        {
            fwrite(buffer, 512, 1, output);
        }
    }
    fclose(output);
    fclose(file);
    return 0;
}
Andreas Wenzel
  • 22,760
  • 4
  • 24
  • 39
Leonard Offor
  • 1
  • 1
  • Have you tried running your code line-by-line in a debugger while monitoring the values of all variables, in order to determine in which line your program stops behaving as intended? If you did not try this, then you may want to read this: [What is a debugger and how can it help me diagnose problems?](https://stackoverflow.com/q/25385173/12149471) Note that CS50 has its own debugger, which is called [debug50](https://video.cs50.io/v_luodP_mfE?screen=J0ND72qsI9U&start=1688&end=2012). – Andreas Wenzel Sep 26 '22 at 03:19
  • Two things: 1) Why not add a few simple print statements that report on the progress of the execution. ("print debugging"). 2) Your machine probably has 'n' Gb of memory. Why is `filename[8]` dimensioned for the minimum possible size required? It's likely the data file being read is more than 1000x512 bytes (0.5Mb)... "%03i" won't prevent a name like "1000.jpg\0" being squished into that tiny buffer (and not fitting there.) – Fe2O3 Sep 26 '22 at 03:50
  • The line `if(buffer[0] == 0xff || buffer[1] == 0xd8 || buffer[2] == 0xff || (buffer[3] & 0xf0) == 0xe0 || feof(file) == 0)` appears wrong. It should probably be `&&` (and) instead of `||` (or), except for the `feof`, which should be `||`. The line should probably look like this: `if ( ( buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0 ) ) || feof(file) )`. – Andreas Wenzel Sep 26 '22 at 04:11
  • Is [this](https://cs50.harvard.edu/x/2022/psets/4/recover/) the assignment that your question is about? If so, you may want to add a link to it in your question. – Andreas Wenzel Sep 26 '22 at 04:37
  • @AndreasWenzel Thank you for your suggestion. I think next time I would try checking first with debug50. I ended up having to replace the entire if statement condition I was using to check if each read file had the Jpeg headers with a bool function and it worked. My first time on stack overflow and I am so grateful for the insight. – Leonard Offor Sep 26 '22 at 05:17
  • @Fe2O3 Thanks for your response. I thought the sprintf function would only allow 3 digits plus the .jpeg based on what the manual said? – Leonard Offor Sep 26 '22 at 05:20
  • From the online manual: "an optional **minimum** field width," (emphasis mine)... Nothing about 'maximum' field width... (Yes, you can specify a maximum width to printf, but you're not doing that here.) And, if you are going > 999, you'll start to clobber "000.jpg" that had recently been written... Not what you want to do...) – Fe2O3 Sep 26 '22 at 05:23
  • Side note: You have 4 lines of unnecessary code duplication in your program. If you write `if(output != NULL) fclose( output );`, then you can remove the `else` and remove one set of the 4 duplicate lines, so that you have no code duplication. – Andreas Wenzel Sep 26 '22 at 05:51

0 Answers0