-1

EDIT 2: Per my initial problem, it was solved via the help of

Weather Vane

Where we found a stark misunderstanding of pointer fundamentals. Once I reviewed pointer information, I solved my Segmentation Error. Thanks everyone

OP:

** The goal is, given a card.raw file, to recover all jpegs using the jpeg signature and assuming that once a jpeg is found, all other jpegs follow consecutively until the end of the file. The file is formatted in FAT 512 byte blocks. **

As the title says, I'm not sure what I'm doing to get the segmentation error. I've tried debugging using valgrind and gcd, but being as new to coding as I am, I couldn't find the source of the segmentation error. I will admit that attempting to decipher the output of these debugging functions proves to be more cryptic than I was expecting.

Additionally, I've searched for others with a similar segmentation error, but the others that I found, especially on this site, source the error from another error in code that I don't (I think) have.

It's driving me crazy as it appears I've created a buffer correctly, ensured that files are always opened and closed in the context of not trapping them in conditional statements, and have utilized fwrite correctly to create a jpeg of correct block size.

I appreciate all and any help given.

EDIT:

As per a commenter requested, here is my valgrind output: 

~/workspace/pset4/recover/ $ valgrind --leak-check=full ./recover card.raw
==26277== Memcheck, a memory error detector
==26277== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==26277== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==26277== Command: ./recover card.raw
==26277== 
==26277== Invalid read of size 1
==26277==    at 0x42DC1F: write_jpeg (recover.c:79)
==26277==    by 0x9A904008211C1099: ???
==26277==    by 0x82104489A502E0F: ???
==26277==    by 0x2104082104082103: ???
==26277==    by 0x408210408210407: ???
==26277==    by 0x821040821040820: ???
==26277==    by 0x9304082104082103: ???
==26277==    by 0x21840021840048: ???
==26277==    by 0x4208104248208481: ???
==26277==    by 0x13F0E30705210447: ???
==26277==    by 0x13E8E0CA918D0902: ???
==26277==    by 0xD384118482262983: ???
==26277==  Address 0x821040821040821 is not stack'd, malloc'd or (recently) free'd
==26277== 
==26277== 
==26277== Process terminating with default action of signal 11 (SIGSEGV)
==26277==  General Protection Fault
==26277==    at 0x42DC1F: write_jpeg (recover.c:79)
==26277==    by 0x9A904008211C1099: ???
==26277==    by 0x82104489A502E0F: ???
==26277==    by 0x2104082104082103: ???
==26277==    by 0x408210408210407: ???
==26277==    by 0x821040821040820: ???
==26277==    by 0x9304082104082103: ???
==26277==    by 0x21840021840048: ???
==26277==    by 0x4208104248208481: ???
==26277==    by 0x13F0E30705210447: ???
==26277==    by 0x13E8E0CA918D0902: ???
==26277==    by 0xD384118482262983: ???
==26277== 
==26277== HEAP SUMMARY:
==26277==     in use at exit: 1,136 bytes in 2 blocks
==26277==   total heap usage: 2 allocs, 0 frees, 1,136 bytes allocated
==26277== 
==26277== LEAK SUMMARY:
==26277==    definitely lost: 0 bytes in 0 blocks
==26277==    indirectly lost: 0 bytes in 0 blocks
==26277==      possibly lost: 0 bytes in 0 blocks
==26277==    still reachable: 1,136 bytes in 2 blocks
==26277==         suppressed: 0 bytes in 0 blocks
==26277== Reachable blocks (those to which a pointer was found) are not shown.
==26277== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==26277== 
==26277== For counts of detected and suppressed errors, rerun with: -v
==26277== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Segmentation fault

Here is my code:

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



// prototypes
void write_jpeg(int count, FILE *file, uint8_t buffer[]);

int main(int argc, char *argv[]) {
    // ensure correct argument usage
    if (argc != 2) {
        fprintf(stderr, "Error. Correct usage: ./recover [infile]\n");
        return 1;
    }

    // open infile
    FILE *file = fopen(argv[1], "r");

    // ensure can open file
    if (file == NULL) {
        fprintf(stderr, "Error, could not open file\n");
        return 2;
    }

    // initializes bool variable that signals start of jpegs
    bool start = false;

    // buffer for file block
    uint8_t buffer[512];

    // loop to find the start of the series of jpegs
    while (!start) {
        // read FAT blocks into buffer
        fread(&buffer, 1, 512, file);

        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff
        && (buffer[3] & 0xf0) == 0xe0) {
            start = true;
            fseek(file, -4, SEEK_CUR);
            memset(buffer, 0x00, 512);
        }
    }

    // count for number of jpegs
    int count = 0;

    // loop to create jpegs
    while ((fread(&buffer, 1, 512, file)) == sizeof(buffer)) {
        write_jpeg(count, file, buffer);
    }

    fclose(file);

    return 0;

}

// writes into a new file until the start of another jpeg
void write_jpeg(int count, FILE *file, uint8_t buffer[]) {
    // creates filenames for jpegs
    char file_name[8];
    sprintf(file_name, "%03i.jpg", count);

    // opens file to write to
    FILE *img = fopen(file_name, "w");

    if (img == NULL) {
        fprintf(stderr, "Error: couldnt create jpg file\n");
        return;
    }

    // bool for end condition
    bool end = false;

    while (!end) {
        // ends if reaches new jpeg signature
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff
        && (buffer[3] & 0xf0) == 0xe0) {
            end = true;
            fseek(file, -4, SEEK_CUR);
            memset(buffer, 0x00, 512);
            count++;
        } else {

        // read the next block into buffer
        fread(&buffer, 1, 512, file);

        // write the buffer into the new file
        fwrite(&buffer, 1, 512, img);

        }
    }

    fclose(img);
}
cartwmic
  • 1
  • 1
  • Did you get a crash when you used Valgrind? What did Valgrind report? – Jonathan Leffler Jul 27 '17 at 05:35
  • I don't understand the purpose of `fseek(file, -4, SEEK_CUR);`. You checked 4 signature bytes but you actually read 512 bytes - at least we can *guess* that you did, since you don't check the return value from `fread` in the function that is doing most of the work. This means you do not detect the end of file in the loop that is reading one jpeg's data. – Weather Vane Jul 27 '17 at 05:57
  • I reviewed my use of fseek, and I now believe that it holds no purpose. I originally used it because I believed that I had moved the infile's cursor 4 bytes and needed to return, but I had a misunderstanding of the actual mechanism that fread and the buffer was serving. If I am understanding correctly I don't need to use fseek. – cartwmic Jul 27 '17 at 06:40
  • @Weather Vane, I am not sure I understand the second part concerning checking the return value of fread and its impact on detecting the end of file in the loop that is rading one jpeg's data. I do recall that it is important to ensure that fread does not return null, which I will implement, but I am not sure how that resolves any of my current issues. Thanks by the way for comments. – cartwmic Jul 27 '17 at 06:42
  • `fread` never returns a pointer value but the number of bytes read and should *always* be checked. How else will you detect the end of file? Looking for another jpeg signature won't work because there won't be one, and you have no other way of breaking the loop in `write_jpeg`. – Weather Vane Jul 27 '17 at 06:44
  • Error: `fread(&buffer, 1, 512, file);` should be `fread(buffer, 1, 512, file);` and similar for `fwrite`. – Weather Vane Jul 27 '17 at 06:47
  • @Weather Vane There won't be another jpeg signature in a series of jpeg fat blocks? I do understand that fread does not return a pointer value, and now am envisioning the correct way to implement this check to stop the loop. – cartwmic Jul 27 '17 at 06:50
  • How can there be another jpeg signature during the copy of the last jpeg in the file? You don't need to call a function. A simple loop `while(fread(buffer, 1, 512, file) == 512) { /* check for jpeg codes, close and open file */ fwrite(buffer, 1, 512, img); }` with some startup and closing code will do. Deal with the first occurrence, and close the last file afterwards. – Weather Vane Jul 27 '17 at 06:51
  • Ok i'll implement it in that way and see if that resolves my issues. Another question. From my understanding, I thought you needed &buffer as it indicates that the 512 elements of size 1 would be read from file and stored into the dereferenced pointer &buffer. I thought I had a firm grasp on pointer fundamentals, but obviously I'm a bit confused. – cartwmic Jul 27 '17 at 06:56
  • What do you suppose the address of a function argument will give you?? An array is never passed to a function, but it decays to a pointer. So the function parameter `uint8_t buffer[]` is really `uint8_t *buffer` - meaning it is *already* a pointer. This may be what caused your segfault. – Weather Vane Jul 27 '17 at 06:56
  • I see, I had a fundamental misunderstand of the correct usage of & in regards to pointers. I understand now that it is used to refer to the address of the pointer, not the value. My apologies. Thank you for the help, I will update once I've reimplimented. – cartwmic Jul 27 '17 at 06:59
  • As a parallel, look at the different ways the arguments for `scanf` are passed here for an integer and a string: `scanf("%d %s", &i, s);` – Weather Vane Jul 27 '17 at 07:02
  • I see. Because a string is really a pointer to a character array, you can simply put the variable name as its pointer because it IS a pointer, correct? In this same vein of understanding, initialized ints in the format "int i = 3" is obviously not a pointer, so in the implementation of scanf, you must use &i in order to point to the pointer. – cartwmic Jul 27 '17 at 07:12
  • An array is not a pointer, but decays to a pointer in this usage. – Weather Vane Jul 27 '17 at 07:13

1 Answers1

0

In the write_jpeg function this is incorrect:

 fread(&buffer,

The fread function expects a pointer to the space in which to write. However you provided a pointer to space in which there is stored a pointer to said space.

It should be fread(buffer,. You make the same mistake with fwrite, and you also make the same mistake in main (however in that case you get away with it due to implementation details).

Also, there will be a buffer overflow if count reaches 1000. The %03i printf specifier means a minimum of 3 digits, not a maximum.

M.M
  • 138,810
  • 21
  • 208
  • 365