pset4 Recover Cant figure out why jpeg file pointer gets reset to NULL.

Question

Using Error messages and gdb I figured out why I wasnt getting segfaults- img pointer - to the current jpeg file gets set to NULL. Are my loops all wrong? Im getting myself pretty confused. Code produces 2 corrupt jpgs with correct titles. Help!!

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


 int main(int argc, char* argv[])
 {
// check for correct amount of command line arguments
if (argc != 1)
{
    printf("Incorrect amount of command line arguments\n");
    return 1;
}

//open memory card file
FILE* memorycard = fopen("card.raw", "r");
if (memorycard == NULL)
{
    printf("Could not open memory card.\n");
    return 2;
}

//declaring and initializing variables and arrays
uint8_t * buffer = malloc(512);   
int jpegcount = 0;
char* name = malloc(8);
FILE* img = NULL; 
int open = 0;


//read 512 bytes from memory card into buffer until end of memory card    
while(fread(buffer, 512, 1, memorycard) > 0)
{   

    //if start of jpeg 
    if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] == 0xe0 || buffer[3] == 0xe1))
    {  

        if (open == 0)
        {
            if (jpegcount < 10)
            {
                sprintf(name, "00%d.jpg", jpegcount);
            }
            else 
            {
                sprintf(name, "0%d.jpg", jpegcount);
            }

            // open new jpeg file
            img = fopen(name, "a");

            if (img == NULL)
            {
                //fprintf(stderr, "Could not create %s.\n", name);
                return 3;
            }

            // set file condition to "open"            
            fwrite(buffer, 512, 1, img);
            open = 1;
        }

        //if this is the (not the first) iteration of finding a new jpeg         
        else if (open == 1)
        {            
            fclose(img);

            jpegcount++;

            //NAMING the jpeg      
            if (jpegcount < 10)
            {
                sprintf(name, "00%d.jpg", jpegcount);
            }
            else 
            {
                sprintf(name, "0%d.jpg", jpegcount);
            }

            // open new jpeg file
            img = fopen(name, "a");

            if (img == NULL)
            {
                fprintf(stderr, "Could not create %s.\n", name);
                return 3;
            }

            fwrite(buffer, 512, 1, img);

        }
    }

    // if jpeg already open
    else if(open == 1)
    {

            img = fopen(name, "a");

            if (img == NULL)
            {
                fprintf(stderr, "Could not append %s.\n", name);
                return 4;
            }                

            fwrite(buffer, 512, 1, img);                
    }        
}

fclose(memorycard);
fclose(img);
free(buffer);
free(name);

//the end
return 0;
 }

Answer 1

You have the following code block at the end of your while loop:

// if jpeg already open
else if(open == 1)
{
        img = fopen(name, "a");
        if (img == NULL)
        {
            fprintf(stderr, "Could not append %s.\n", name);
            return 4;
        }                

        fwrite(buffer, 512, 1, img);                
}  

It checks to see if you have a file open for output. Then the very next thing you do is reopen the already opened file. This is resetting the pointer and messing with what is output to the file. If you were to look at the first few bytes of the 000.jpg file, you'd see zeros where the opening signature should be. The only line necessary in this IF() block is the fwrite() call.

Next, a minor issue is that you open the output files in "a" or append mode. If the output file exists from a prior run of the program, any data will only be appended to it instead of starting a new file.

Finally, as a matter of efficient coding, you have two blocks of essentially identical code - one for opening the first output file and one for opening later files. This should be a clue that there's a more efficient way to do this. Since the only difference is a check whether an output file is already opened, you could start by checking for the open file, and if there is one, close it. Then you can open a new output file without being concerned whether it is the first one or not - meaning one block of code to open output files.

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)