CS50 PSET3 Recover作业：JPG生成异常求代码排查

Let's break down the problems in your code and fix them step by step — I've been through this PSET too, so I know how tricky those JPG signatures can be!

Key Problems in Your Current Code #

1. Incorrect Non-JPG Block Check #

Your condition for writing non-JPG blocks is backwards:

if (buffer[0] != 0xff && buffer[1] != 0xd8 && buffer[2] != 0xff && (buffer[3] & 0xf0) != 0xe0)

This only writes a block if all four bytes don't match the JPG signature — but a block is part of the current JPG as long as it doesn't match the full signature. You should check if the block is NOT a JPG start, which means using a negation of the full signature check, not && between all the != conditions.

2. Improper File Closing for the Last Image #

Your code only closes the last file if counter > 1, which means the first image (counter=0) or second (counter=1) won't get closed properly. Unclosed files can lose data because the OS might not flush the write buffer to disk.

3. Overcomplicated Flow with goto #

While goto isn't inherently evil, it makes your code harder to follow and increases the chance of missing edge cases (like forgetting to handle the end of the input file cleanly). A simpler linear flow will avoid these issues.

Fixed Code #

Here's a revised version of your code that addresses all these issues:

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

typedef uint8_t BYTE;

int main(int argc, char *argv[]) {
    // Check argument count
    if (argc != 2) {
        fprintf(stderr, "Usage: ./recover image\n");
        return 1;
    }

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

    BYTE buffer[512];
    int counter = 0;
    FILE *outptr = NULL; // Initialize to NULL to track if we're writing a file

    while (fread(buffer, sizeof(BYTE), 512, inptr) == 512) {
        // Check if this is the start of a JPG
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0) {
            // If we were already writing a file, close it first
            if (outptr != NULL) {
                fclose(outptr);
            }

            // Create new output file
            char filename[8];
            sprintf(filename, "%03i.jpg", counter);
            outptr = fopen(filename, "w");
            if (outptr == NULL) {
                fprintf(stderr, "Could not create %s.\n", filename);
                fclose(inptr);
                return 3;
            }

            // Write the current block to the new file
            fwrite(buffer, sizeof(BYTE), 512, outptr);
            counter++;
        }
        // If we're already writing a file, write the current block
        else if (outptr != NULL) {
            fwrite(buffer, sizeof(BYTE), 512, outptr);
        }
    }

    // Close the last output file if it was open
    if (outptr != NULL) {
        fclose(outptr);
    }

    // Close input file
    fclose(inptr);
    return 0;
}

What Changed? #

• Fixed the block write condition: Instead of checking if all bytes don't match the signature, we check if we're already writing a file (meaning this block is part of the current JPG) and write it directly.
• Proper file closing: We close the previous file before opening a new one, and explicitly close the last file after the loop ends — no more lost data from unclosed files.
• Simplified flow: Removed the goto and nested loops, using a single main loop that handles both detecting new JPGs and writing blocks to the current file. This makes the code easier to read and debug.
• Added error checking for output files: Now we handle cases where we can't create an output file, which is good practice for robust code.

Give this a try — your recovered JPGs should now display correctly!

内容的提问来源于stack exchange，提问作者Rohit_