While doing the CS50-task recover i finished everything and ran valgrind over it. Valgrind returns a possible segmentation fault, even though I made sure to never access memory, I don't have access to.
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
int main(int argc, char *argv[])
{
if (argc != 2) {
printf("Usage: ./recover [argument]\n");
return 1;
}
FILE* sdCard = fopen(argv[1], "r");
if (sdCard == NULL) {
printf("Couldn't open %s in read-mode", argv[1]);
return 1;
}
uint8_t buffer[512];
// ###.png\0 => 8 characters
char file_name[8];
int fileNumber = 0;
FILE* recoveredJPG;
char* zeroes = NULL;
int foundFirstJPEG = 0;
// Create files
while(fread(buffer, sizeof(uint8_t), 512, sdCard)) {
if (buffer[0] == 0xFF && buffer[1] == 0xD8 && buffer[2] == 0xFF && (buffer[3] & 0xFF) == 0xE0) {
foundFirstJPEG = 1;
if (fileNumber < 10) {
zeroes = malloc(3*sizeof(char));
if (zeroes == NULL) {
printf("Memory allocation failed!");
fclose(sdCard);
sdCard = NULL;
if (recoveredJPG != NULL) {
fclose(recoveredJPG);
recoveredJPG = NULL;
}
return 1;
}
strcpy(zeroes, "00");
} else if (fileNumber >= 10) {
zeroes = malloc(2*sizeof(char));
if (zeroes == NULL) {
printf("Memory allocation failed!");
fclose(sdCard);
sdCard = NULL;
if (recoveredJPG != NULL) {
fclose(recoveredJPG);
recoveredJPG = NULL;
}
return 1;
}
strcpy(zeroes, "0");
}
if (zeroes == NULL) {
printf("Memory allocation failed!");
fclose(sdCard);
sdCard = NULL;
if (recoveredJPG != NULL) {
fclose(recoveredJPG);
recoveredJPG = NULL;
}
return 1;
}
sprintf(file_name, "%s%i.jpg", zeroes, fileNumber);
if (recoveredJPG != NULL){
fclose(recoveredJPG);
recoveredJPG = NULL;
}
recoveredJPG = fopen(file_name, "w");
if (recoveredJPG == NULL) {
printf("Error while creating %s.", file_name);
free(zeroes);
zeroes = NULL;
fclose(sdCard);
sdCard = NULL;
return 1;
}
free(zeroes);
zeroes = NULL;
fwrite(buffer, sizeof(uint8_t), 512, recoveredJPG);
fileNumber++;
} else if (foundFirstJPEG) {
fwrite(buffer, sizeof(uint8_t), 512, recoveredJPG);
}
}
if (recoveredJPG != NULL) {
fclose(recoveredJPG);
recoveredJPG = NULL;
}
fclose(sdCard);
sdCard = NULL;
if (zeroes != NULL) {
free(zeroes);
zeroes = NULL;
}
return 0;
}
I tried everything, NULLing every file after deleting, freeing every pointer before every return, making sure i check that every pointer isn't NULL before accessing. I still get a possible segmentation fault.
This is valgind's output:
==3125== Memcheck, a memory error detector
==3125== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==3125== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
==3125== Command: ./recover card.raw
==3125==
==3125== Conditional jump or move depends on uninitialised value(s)
==3125== at 0x10948C: main (recover.c:81)
==3125== Uninitialised value was created by a stack allocation
==3125== at 0x1091C4: main (recover.c:7)
==3125==
==3125== Use of uninitialised value of size 8
==3125== at 0x4A0E294: fclose@@GLIBC_2.2.5 (iofclose.c:48)
==3125== by 0x10949D: main (recover.c:82)
==3125== Uninitialised value was created by a stack allocation
==3125== at 0x1091C4: main (recover.c:7)
==3125==
==3125== Use of uninitialised value of size 8
==3125== at 0x4A0E33C: fclose@@GLIBC_2.2.5 (iofclose.c:51)
==3125== by 0x10949D: main (recover.c:82)
==3125== Uninitialised value was created by a stack allocation
==3125== at 0x1091C4: main (recover.c:7)
==3125==
==3125== Invalid read of size 8
==3125== at 0x4A0E353: fclose@@GLIBC_2.2.5 (iofclose.c:51)
==3125== by 0x10949D: main (recover.c:82)
==3125== Address 0x8 is not stack'd, malloc'd or (recently) free'd
==3125==
==3125==
==3125== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==3125== Access not within mapped region at address 0x8
==3125== at 0x4A0E353: fclose@@GLIBC_2.2.5 (iofclose.c:51)
==3125== by 0x10949D: main (recover.c:82)
==3125== If you believe this happened as a result of a stack
==3125== overflow in your program's main thread (unlikely but
==3125== possible), you can try to increase the size of the
==3125== main thread stack using the --main-stacksize= flag.
==3125== The main thread stack size used in this run was 8388608.
==3125==
==3125== HEAP SUMMARY:
==3125== in use at exit: 4,571 bytes in 3 blocks
==3125== total heap usage: 3 allocs, 0 frees, 4,571 bytes allocated
==3125==
==3125== 3 bytes in 1 blocks are still reachable in loss record 1 of 3
==3125== at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==3125== by 0x1092F2: main (recover.c:38)
==3125==
==3125== 472 bytes in 1 blocks are still reachable in loss record 2 of 3
==3125== at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==3125== by 0x4A0EE6E: __fopen_internal (iofopen.c:65)
==3125== by 0x4A0EE6E: fopen@@GLIBC_2.2.5 (iofopen.c:86)
==3125== by 0x109210: main (recover.c:13)
==3125==
==3125== 4,096 bytes in 1 blocks are still reachable in loss record 3 of 3
==3125== at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==3125== by 0x4A0E1A4: _IO_file_doallocate (filedoalloc.c:101)
==3125== by 0x4A1E513: _IO_doallocbuf (genops.c:347)
==3125== by 0x4A1CF3B: _IO_file_xsgetn (fileops.c:1288)
==3125== by 0x4A0F48A: fread (iofread.c:38)
==3125== by 0x10927A: main (recover.c:33)
==3125==
==3125== LEAK SUMMARY:
==3125== definitely lost: 0 bytes in 0 blocks
==3125== indirectly lost: 0 bytes in 0 blocks
==3125== possibly lost: 0 bytes in 0 blocks
==3125== still reachable: 4,571 bytes in 3 blocks
==3125== suppressed: 0 bytes in 0 blocks
==3125==
==3125== For lists of detected and suppressed errors, rerun with: -s
==3125== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 0 from 0)
/opt/cs50/bin/valgrind: line 13: 3125 Segmentation fault (core dumped) /usr/bin/valgrind "$@"
PS: Please don't give any tips on how to solve the task recover.c from CS50 more efficiently, I will try doing that after i find out what causes this segmentation fault.
PPS: When running the file myself, i get no error and everything runs as normal.
Thanks for your help.
Well you've gotten as far as letting Valgrind chew through it, we just have to interpret the messages:
Conditional jump or move depends on uninitialised value(s) ==3125== at 0x10948C: main (recover.c:81)
Yep, you are using recoveredJPG all over the place and not necessarily after calling fopen first. Since you check for NULL everywhere, this should be easy to fix by initializing the variable: FILE* recoveredJPG = NULL;.
Also, for readability reasons the program could do with some splitting up in smaller functions, instead of one big loop with multiple nested if statements.