0

I am getting a segmentation fault when running my program that only allows me to print out 1 broken image. However, when I run the program with valgrind it prints out 49 of the 50 photos, and the only photo not working is the last one.

After running help50 valgrind with the program, I realized there is a segmentation fault with sprintf, as well as something possibly wrong with the fwrite() towards the end of the program. Any clarification would be appreciated.

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

int main(int argc, char *argv[])
{
    // Check you're actually recovering a file.
    if (argc != 2)
    {
        printf("Invalid entry.\n");
        return 0;
    }

    // Open file for reading.
    FILE *file = fopen(argv[1], "r");
    if (!file)
    {
        return 1;
    }

    // Set 1st photo as jpg.001.
    int counter = 1;

    // Check if bytes are jpg. signatures.
    for (int n = 0; counter < 51; n = n + 512)
    {
        // Sets an array to contain 4 values and directs pointer to start from nth value each time.
        unsigned char array[512];
        fseek(file, n, SEEK_SET);
        fread(array, 1, 512, file); // if EOF, won't have 512 to write into!!!

        // Declare character array jpg.
        char* jpg_name;

        // While 1st 4 bytes are jpg signature.
        if (array[0] == 0xff && array[1] == 0xd8 && array[2] == 0xff && (array[3] & 0xf0) == 0xe0)
        {
            // Convert integer to string and store into jpg character array. Increment image number.
            jpg_name = malloc(4*sizeof(char));
            sprintf(jpg_name, "%03i.jpg", counter);
            counter++;

            // Open images file to write into,allocate memory to jpg file to write into, write 512 bytes from array into image file.
            FILE *images = fopen(jpg_name, "a");
            fwrite(array, 1, 512, images);

            // Free file memory?
            free(jpg_name);
            fclose(images);
        }
        else // If 1st 4 bytes aren't jpg signature.
        {
            // Add bytes to existing image (if present), otherwise repeat loop.
            if (counter > 1)
            {
                FILE *images = fopen(jpg_name, "a");
                fwrite(array, 1, 512, images); // SEGMENTATION FAULT; need to detect end of file.
                fclose(images);
            }
            else
            {
                continue;
            }
        }
    }
}
1

Look at the following code:

    char* jpg_name;
       ...
        jpg_name = malloc(4*sizeof(char));
        sprintf(jpg_name, "%03i.jpg", counter);

jpg_name only has space for 4 chars allocated to it, but the sprintf call is writing 8 chars. C allows the write to go past the allocated space (there's a historical reason for it), so it will attempt to overwrite the physical memory that follows the memory allocated to jpg_name. Keep in mind that you're not allocating space for the number of the file. There needs to be room for the complete filename plus the end of string number. The filenames are 7 chars long and EOS marker is one additional char, for 8 total.

Next, while it's technically correct, why use a pointer and a malloc for this (other than for the practice)? This is a temporary holder for the file names, but they're all going to be the same size and don't need to be retained after the file is opened. In this case, it's more practical and efficient to just allocate a string or char array and reuse it over and over. It's also less overhead at run time.

char jpg_name[8];

Now, let's talk about the images pointer. First, why is it being opened for append? This will work if the file doesn't exist, but it will corrupt an existing file. It should be opened for "w" or write. This will force the deletion of any existing file of the same name and eliminate corrupting an existing file.

Second, why is it being closed after writing the signature block? It needs to be left open so subsequent data blocks can be written to it! It should only be closed when the next signature block is found and a new file is about to be opened.

As a general statement, if you're working with a file, you should open it and leave it open while you continue processing until you're done with it. The process of opening a file has some of the biggest runtime overhead of any activity, especially when stored on a spinning drive of any type. (SSD storage isn't so bad, but there's still a lot of overhead.) To put it in real world terms, say that you open a file, read or write to it, and then close it. The opening and closing operations will be about 95% of the time and processing power involved. That's why it's best to open it ONCE and leave it open until done.

There's another issue in this code. images is repeatedly declared, but more importantly, is declared inside of if code blocks. This represents a scope issue. When processing leaves the nearest enclosing curly braces that surround the images pointer, it ceases to exist. That means that if you try to use the pointer that has been opened, once it goes out of scope like this, the pointer doesn't exist.

It would be much better to declare the pointer early on and outside of all the curly braces, and to reuse the pointer throughout the rest of the code. NOTE: creating the pointer and initializing or reusing it are two different things.

Finally, look at this:

        else
        {
            continue;
        }

This is dead code. It does nothing and can be deleted without any negative impact. Deleting it will actually improve things. There is no requirement to have an else clause when it doesn't do anything.

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

2
  • Thanks Cliff; I appreciate you taking the time to write all this out for me. I've made the suggested improvements to my code, just have a quick question: just to try it out, I've tried changing malloc to allocate 8 characters worth of memory, but I'm still getting a segmentation fault; why is that? – ckwan Jul 13 '20 at 23:32
  • I don't know. I'd have to see the revised code. At this point, with all those changes, a new question would be in order. – Cliff B Jul 13 '20 at 23:48

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .