mithilesh mithilesh - 10 days ago 9
C Question

My code for recover.c recovers jpegs correctly but does not pass cs50 check

Here is the code. I am a beginner at C language so any suggestions to shorten my code will be greatly appreciated. I checked all the 50 images and they look perfect but the code doesn't pass the cs50 check.

int main(void)
{
FILE* source = fopen("card.raw", "r");



uint8_t jpg[512];
int direct = 0;
int jpgcounter = 0;
uint8_t checkjpg[4];
FILE* outputfile ;
char filename[7] ;

while(feof(source) == 0)
{
fread(jpg,512,1,source);

for (int i = 0 ; i < 4 ; i++)
{
checkjpg[i] = jpg[i];
}

if( checkjpg[0] == 0xff && checkjpg[1] == 0xd8 && checkjpg[2] == 0xff && checkjpg[3] >= 0xe0 && checkjpg[3] <= 0xef )
{

if ( direct == 0 )
{
sprintf( filename, "%03d.jpg" , jpgcounter);
outputfile = fopen(filename, "w");
fwrite(jpg,512,1,outputfile);
direct = 1;
}
else
{
fclose(outputfile);
jpgcounter++;
sprintf( filename, "%03d.jpg" , jpgcounter);
outputfile = fopen(filename, "w");
fwrite(jpg,512,1,outputfile);
}

}
else
{
fwrite(jpg,512,1,outputfile) ;
}
}
fclose(outputfile);
fclose(source);
return 0;
}


Now the major problem is that it does not pass cs50 check so there must be some data that i missed on card.raw or something else and the human eye cannot detect these mistakes in the images but the computer can.

Answer

I think I know what the problem is. You are never initializing outputfile. As a beginner, you should always initialize variables when you declare them. Never write

int i;

write

int i = ...;

and give it an initial value (0, -1, INT_MAX, whatever you like). This is very important for pointers. If you write

FILE * outputfile;

where is that pointer pointing to? Well, it's random. It may point anywhere or nowhere. This is an "invalid" pointer, under no circumstances you must use outputfile now as the result of any usage is undefined. But how would you know it's initialized? Well, you can't! You can't as you never assigned it any value you could check for.

Better code would be

FILE * outputfile = NULL;

as now outputfile has a defined value, it is NULL and that means you can test if it was initialed

if (outputfile == NULL) {
    // Not initialized
} else {
    // Initialized
}

Look at your code and think about the following:
What happens if your loop runs for the very first time and the first 512 byte chunk does not match the if-test for 0xffd8ff..? Then you end in the else-case and this case does the following

fwrite(jpg,512,1,outputfile) ;

but what value has outputfile here? No value, it's totally undefined. You accessing memory at any address and this is very likely to crash your application which is exactly what happens for you. If you had initialized outputfile to NULL, the correct code would have been:

} else if (outputfile != NULL) {
    fwrite(jpg,512,1,outputfile);
}