CHECK50 Passes, but I Get 6/7 When Submitting

Question

I've just completed pset4 - recover.

I did quite a bit of debugging and was finally able to pass all checkmarks with check50, style50 and Valgrind. After submitting I checked my grade and I received 6/7 (.97) with the error ":( program is free of memory errors. Cause: timed out while waiting for program to exit".

I'm not sure why I'm getting this error as all my testing passes in the terminal. I'm not as concerned with the grade itself as I am about not understanding if I've done something wrong. Can anyone help?

Thanks!

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

typedef uint8_t BYTE;

int main(int argc, char *argv[])
{
    // Ensure only 1 command line argument.
    if (argc != 2)
    {
        printf("Error: Use only 1 command line argument.\n");
        return 1;
    }

    // Open memory card.
    FILE *card = fopen(argv[1], "r");
    if (card == NULL)
    {
        printf("Could not open file.\n");
        return 1;
    }

    // Declare FILE variable.
    FILE *img;


    // Allocate memory for buffer.
    unsigned char *buffer = malloc(512);
    if (buffer == NULL)
    {
        printf("Not enough memory available.\n");
        return 1;
    }

    // Declarations for use in while loop
    bool jpg = false;
    int count = 0;
    char filename[8];
    bool isOpen = false;

    // Repeat until end of card:
    while (fread(buffer, 1, sizeof(buffer), card) != 0)
    {
        // Check first 4 bytes of buffer - is it the start of a JPG?
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && ((buffer[3] & 0xf0) == 0xe0))
        {
            // Closes previous img file.
            if (isOpen == true)
            {
                fclose(img);
            }

            // Change jpg variable to true.
            jpg = true;

            // Print the name of the JPEG file into the variable "filename" and add to JPEG counter
            sprintf(filename, "%03i.jpg", count);
            count++;

            // Open previously created "filename" file in order to write to.
            img = fopen(filename, "w");
            if (img == NULL)
            {
                printf("Could not open file.\n");
                return 1;
            }
            fwrite(buffer, 1, sizeof(buffer), img);

            // Change isOpen to true.
            isOpen = true;
        }

        // If buffer does not equal a jpg header, but jpg is true then do this.
        else if (jpg == true)
        {
            fwrite(buffer, 1, sizeof(buffer), img);
        }
    }

    // Free malloc memory.
    free(buffer);

    // Close img
    fclose(img);

    // Close memory card.
    fclose(card);

    return 0;
}

Answer 1

check50 is timing out because your program takes longer to run than is allowed.

It comes down to this

Your buffer is unsigned char *buffer = malloc(512); Your fread is fread(buffer, 1, sizeof(buffer), card)

so it’s only reading in 8 bytes at a time, since buffer is a pointer. sizeof(buffer) is 8.

Because of that, valgrind on check50 times out since it takes 64 times longer than it would if you read in 512 bytes at a time. :)

You might consider not using malloc and simply declaring an array of 512 bytes. In that way, sizeof(buffer) would then be 512.

Answer 2

As my comment mentioned the main issue I see is that you have some logical errors in your code that should be fixed, and your read is also not of the proper size because you used sizeof(buffer) which is going to be 8 (the size of a unsigned char* and not 512 the size of the block you have reserved. This is because sizeof is a compile time operator. As a seperate aside there is no real need for dynamic allocation here and you could have just used an unsigned char buffer[512] as it will fit on the stack. sizeof does then work as pointed out by curiouskiwi, but it would be preferable IMHO to use a constant instead of 512 and then provide sizeof(unsigned char) * <ThatConstant> as the argument or as another stored constant to be provided as the argument to the fread and fwrite calls.

I also found a pathway in which you could technically leak memory if you encountered an error creating a file.

See notes in comments:

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

typedef uint8_t BYTE; // where do you use this?

int main(int argc, char *argv[])
{
    // Ensure only 1 command line argument.
    if (argc != 2)
    {
        printf("Error: Use only 1 command line argument.\n");
        return 1;
    }

    // Open memory card.
    FILE *card = fopen(argv[1], "r");
    if (card == NULL)
    {
        printf("Could not open file.\n");
        return 1;
    }

    // Declare FILE variable.
    FILE *img;


    // Allocate memory for buffer.
    //CHANGE HERE TO REMOVE MAGIC NUMBER
    const int len = 512 * sizeof(unsigned char);
    unsigned char *buffer = (unsigned char*)malloc(len); // adjusted
    if (buffer == NULL)
    {
        printf("Not enough memory available.\n");
        return 1;
    }

    // Declarations for use in while loop
    // bool jpg = false; DONT NEED THIS
    int count = 0;
    char filename[8];
    bool isOpen = false;

    // Repeat until end of card:
    // changed to use the constant instead of sizeof
    while (fread(buffer, 1, len, card) != 0)
    {
        // Check first 4 bytes of buffer - is it the start of a JPG?
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && ((buffer[3] & 0xf0) == 0xe0))
        {
            // Closes previous img file.
            if (isOpen == true)
            {
                fclose(img);
                isOpen = false; // THIS WAS MISSING
                //just to be safe here
                img = NULL;
            }

            // Print the name of the JPEG file into the variable "filename" and add to JPEG counter
            sprintf(filename, "%03i.jpg", count);
            count++;

            // Open previously created "filename" file in order to write to.
            img = fopen(filename, "w");
            if (img == NULL)
            {
                printf("Could not open file.\n");
                // ADDED TO AVOID LEAK IN EARLY EXIT
                free(buffer); //without this you were leaking
                return 1;
            }
            // MOVED
            // Change isOpen to true.
            isOpen = true;
            // revised and removed duplicated fwrite code
        }
        // with just an if on isOpen we can handle both cases
        if (isOpen == true)
        {
            // we should check that fwrite wrote the length we asked it to
            fwrite(buffer, 1, len, img); //changed to len
        }
    }

    // Free malloc memory.
    free(buffer);

    // Close img
    // add check to avoid double close
    if (isOpen == true)
    {
        fclose(img);
    }

    // Close memory card.
    fclose(card);

    return 0;
}