I've written a short program that reads JPEGs from a memory card and writes the JPEGs as individual files as the output. The program works as I intended, but I understand my code is poorly written as a don't free dynamic memory from the heap.
I've attempted to use fclose() and free() in my code, but I get the following error when I do:
free(): double free detected in tcache 2
Aborted
I've commented out these functions, and my code works as intended. I want to know why it didn't work when I tried to use these two functions. What did I do wrong?
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
// define/declare variables
typedef uint8_t BYTE;
FILE *outptr;
int main(int argc, char *argv[])
{
// declare variables
int counter = 0;
// ensure correct command-line useage
if (argc != 2)
{
printf("Useage: ./recover memorycard\n");
return 1;
}
// open memory card to read
FILE *inptr = fopen(argv[1], "r");
if (inptr == NULL) // ensure enough memory
{
fprintf(stderr, "Could not open file");
return 2;
}
// read into infile, 512 bytes at a time until end of file
BYTE buffer[512];
while (fread(buffer, sizeof(BYTE), 512, inptr) == 512)
{
// if JPEG headerfile is found
if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
{
if (counter > 0) // if not first JPEG
{
// close previous file
// fclose(outptr); // here is the issue
// free(outptr); // and here
}
// allocate memory for new file
char *outfile = malloc(sizeof(BYTE) * 512);
if (outfile == NULL)
{
printf("Not enough memory for output file\n");
fclose(inptr);
return 3;
}
// write new file name
sprintf(outfile, "%03i.jpg", counter);
// open new file
outptr = fopen(outfile, "w");
if (outptr == NULL)
{
fprintf(stderr, "Could not open file.\n");
fclose(inptr);
return 4;
}
// write 512 bytes from infile into new file
fwrite (buffer, sizeof(BYTE), 512, outptr);
counter++;
}
// if not start of new JPEG
else if (buffer[0] != 0xff || buffer[1] != 0xd8 || buffer[2] != 0xff || (buffer[3] & 0xf0) != 0xe0)
{
// if already found JPEG
if (counter > 0)
{
// keep writing into JPEG
fwrite (buffer, sizeof(BYTE), 512, outptr);
}
continue;
}
}
// Close remaining files once gone through memory card
// fclose(inptr); // here
// free(inptr); // and here
}
In the first iteration of a loop the outptr variable is uninitialized, so any action taken with it brings undefined behavior.
Specifically, you haven't assigned it any value obtained from fopen() so you should not fclose() it.
Also, you haven't assigned it any value obtained from malloc() or calloc() so you should not free() it.
You need to fopen() a file when you need to write to it, keep a pointer to it as long as it's needed (you write to it) and fclose() when done. No malloc() and no free() is necessary for file handling.
Here is the code simplified a bit:
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
// define/declare variables
typedef uint8_t BYTE;
FILE *outptr = NULL;
int main(int argc, char *argv[])
{
// declare variables
int counter = 0;
// ensure correct command-line useage
if (argc != 2)
{
printf("Useage: ./recover memorycard\n");
return 1;
}
// open memory card to read
FILE *inptr = fopen(argv[1], "r");
if (inptr == NULL) // ensure enough memory
{
fprintf(stderr, "Could not open file");
return 2;
}
// read into infile, 512 bytes at a time until end of file
BYTE buffer[512];
while (fread(buffer, sizeof(BYTE), 512, inptr) == 512)
{
// if JPEG headerfile is found
if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
{
if (outptr != NULL) // previous file is opened
{
fclose(outptr); // close it
}
char outfilename[ 512]; // output file name
sprintf(outfilename, "%03i.jpg", counter ++);
// open new file
outptr = fopen(outfilename, "w");
if (outptr == NULL)
{
fprintf(stderr, "Could not open file.\n");
fclose(inptr);
return 4;
}
}
// whether it's a new or still the same file...
if(outptr != NULL)
{
// keep writing into JPEG
fwrite (buffer, sizeof(BYTE), 512, outptr);
}
}
fclose(outptr); // close the last file created
fclose(inptr); // as well as the input
}
I have removed a call to malloc for a file name - it's short enough to be allocated automatically on the stack, and you do not need to free it after use (which you forgot to do, BTW, hence creating a memory leak...)
I noticed that you do exactly the same write in if and in else branch, so I moved it outside the conditional.
I also removed the continue; instruction as the loop continues itself once control reaches the closing brace.
And the most important: I added initialization FILE* outptr = NULL; so it can be check on the file pointer variable whether the file has been successfuly opened instead of checking an apparently unconnected counter variable.