pset 4 - recover: undefined behavior?

Question

I got a question regarding check50 on pset4 - recover. If i run check50 a couple times in a row, without even compiling my code in between I get different results (might be all green smileys or showing errors). Is this due to undefined behavior in my code and/or is check50 using random card images?

I can recover all 50 pictures from the given card image, but still, something seems wrong with my code and I am having a hard time fixing this bug as check50 shows no errors three times in a row but shows an error on the fourth check.

I was struggeling to implement the mask to check the first four bits of the fourth byte, so it might have to do with that, but I can't see something wrong there.

This is the relevant part of my code:

//check number or commandline arguments
...
//open and check file
...
//stop and name variables
int stop = 0;
    int readreturn = 0;
    int picturenumber = 0;
    unsigned char buffer[blocksize];
//read until first .jpg header
    while (buffer[0] != 0xff && buffer[1] != 0xd8 && buffer[2] != 0xff && (buffer[3] & 0xf0) != 0xe0 && stop == 0 )
    {
        readreturn = fread(&buffer[0], blocksize, 1, inptr);
    //if end of file is reached stop and return error
        if(readreturn != 1)
        {
            stop =1;
            fprintf(stderr,"no pictures found");
            return 3;
        }
    }
//open and name first file
    FILE* outptr;
    char picturename[8] ={'\0'};
    int d2 = picturenumber/100;
    int d1 = picturenumber%100/10;
    int d0 = picturenumber%10;
    sprintf(picturename, "%i%i%i.jpg", d2,d1,d0);
    outptr = fopen(picturename, "w");
//write files until next .jpg header and iterate until end of file
while(stop==0 )
{
    fwrite(&buffer[0], blocksize, 1, outptr);
//read and check for end of file
    readreturn = fread(&buffer[0], blocksize, 1, inptr);
    if(readreturn != 1)
        stop =1;
//close if next .jpeg header or end of file is reacher
    if((buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0) || stop == 1 )
    {
        fclose(outptr);
        picturenumber++;
        d2 = picturenumber/100;
        d1 = picturenumber%100/10;
        d0 = picturenumber%10;
        sprintf(picturename, "%i%i%i.jpg", d2,d1,d0);
    //only open new file if the end of file is not reached
        if (stop !=1)
            outptr = fopen(picturename, "w");
    }
}

Any help highly appreciated. Cheers

edit: updated version

//check number or commandline arguments
...
//open and check file
...
//initialize variables and pointers
FILE *outptr = NULL;
unsigned char buffer[blocksize] = {'0'};
char picturename[8] = {'\0'};
int picturenumber = 0;
int write = 0;
//read through whole data
while (fread(&buffer, blocksize, 1, inptr) == 1)
{
    //find the first .jpg header and open first file
    if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
    {
        //open output file on first .jpg header
        if (write == 0)
        {
            sprintf(picturename, "%03i.jpg", picturenumber);
            outptr = fopen(picturename, "w");
        }
        //from second header on close output file and open a new file
        if (write == 1)
        {
            fclose(outptr);
            picturenumber++;
            sprintf(picturename, "%03i.jpg", picturenumber);
            outptr = fopen(picturename, "w");
        }
    }
        //don't write before first header
        if (outptr != NULL)
        {
        write = fwrite(&buffer, blocksize, 1, outptr);
        }
}
fclose(outptr);
fclose(inptr);

Answer 1

If you haven't omitted code between declaration and first use of buffer, you enter the first while loop and test the content of buffer before you ever load any content into the array, so it contains arbitrary values at that moment. Stack variables and those malloced on heap are not initialised by default (global variables are, by default to zero).

And the negation of

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

is

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

or applying De Morgan's Law

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

I'd not do two separate loops, but one. You could even do the fread in the condition, like while (fread(buffer, blocksize, 1, inptr) == 1). Just don't write anything if you so far had no reason to open an output file.

Maybe valgrind could help you, invoked like valgrind ./recover card.raw. Its memory checker will test for various things, including access to memory without defined value. While it produces false positives and false negatives, most output is pointing to actual problems (or their consequences, sometimes those aren't as easy to trace back).

Answer 2

Good question, had a similar issue regarding the negation. In the end I was able to remove the negation all together. Thanks @Blauelf for your help.

    //init. pointer, variables and arrays
FILE *outptr = NULL;
int counter = 0;
char name[8] = {0};
unsigned char buffer[512] = {'0'};

//main Loop
while (fread(&buffer[0], 1, 512, inptr) == 512)
{
    //.jpg header ID found
    if (buffer[0] == 0xff &&
    buffer[1] == 0xd8 &&
    buffer[2] == 0xff &&
    (buffer[3] & 0xf0) == 0xe0)
    {
        //first .jpg header is found -> only true for 000.jpg
        if (counter == 0)
        { ... }

        //(i-th) .jpg header found
        if (counter > 0)
        { ... }

        fwrite(...);
        counter++;
    }

    //blocks not starting with header bytes
    else
    {
        //continuation of an already opened .jpg
        if (counter != 0)
        {
            fwrite( ... );
        }
    }

//job done! :)
fclose(outptr);
fclose(inptr);
return 0;
}