per valgrind: multiple invalid read and invalid write; invalid free(); "still reachable: 568 bytes in 1 blocks"

Question

My recover.c seems to recover all images correctly (per check50) but still seems to have issues with a couple of things per valgrind (output copied at the bottom). As background, I am using a recursive function to match the first bytes of the buffer to magic numbers and read to an old or new file accordingly.

On the one hand, although valgrind finds a matching 53 alloc's and 53 frees, it also returns 19 invalid reads, 3 invalid writes, and one invalid free() / delete / delete[] / realloc(), all of which seem to be related to my fclosing the output target file when cleaning up (line 57 of the culprit code). If I remove this line, valgrind returns no read/write/free errors (output here), but does find only 52 frees for 53 alloc's (makes sense, I think, when target is left dangling open). If anyone has any idea how to deal with the invalid reads/writes, I would like to know. Perhaps (or maybe not?) it has to do with the second issue:

On the other hand, there is the issue of the still reachable bytes, and I am clueless as I am pretty sure I only malloc and free() once. Valgrind returns the same error regardless of whether the buffer is malloc'd and freed either within the function or in main.

The culprit code is here.

----UPDATE: An earlier version of my code (pre-7/22/2016) erroneously re-defined a variable within the function. (Thanks to Alejandro's answer to his post here. Although I have now corrected that in the updated culprit code, this unfortunately does not eliminate valgrind errors.----

Apologies for the verbosity. Any input is greatly appreciated!

--Begins valgrind output--

==8990== Memcheck, a memory error detector
==8990== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==8990== Using Valgrind-3.10.0.SVN and LibVEX; rerun with -h for copyright info
==8990== Command: ./recover
==8990== 
==8990== Invalid read of size 4
==8990==    at 0x51AA994: fclose@@GLIBC_2.2.5 (iofclose.c:54)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55022c0 is 0 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 8
==8990==    at 0x51AA9AD: fclose@@GLIBC_2.2.5 (iofclose.c:57)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x5502348 is 136 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 8
==8990==    at 0x51AA9BD: fclose@@GLIBC_2.2.5 (iofclose.c:57)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55023a8 is 232 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 4
==8990==    at 0x51AA9DE: fclose@@GLIBC_2.2.5 (iofclose.c:57)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55023a0 is 224 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 8
==8990==    at 0x51AA9E8: fclose@@GLIBC_2.2.5 (iofclose.c:57)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x5502348 is 136 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 8
==8990==    at 0x51AA9EF: fclose@@GLIBC_2.2.5 (iofclose.c:57)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x5502348 is 136 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 4
==8990==    at 0x51AA9F6: fclose@@GLIBC_2.2.5 (iofclose.c:57)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55022c0 is 0 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid write of size 8
==8990==    at 0x51AA9F8: fclose@@GLIBC_2.2.5 (iofclose.c:57)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55023a8 is 232 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 4
==8990==    at 0x51AA9FC: fclose@@GLIBC_2.2.5 (iofclose.c:57)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55023a4 is 228 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 8
==8990==    at 0x51AAA19: fclose@@GLIBC_2.2.5 (libioP.h:905)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x5502348 is 136 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 4
==8990==    at 0x51AAA20: fclose@@GLIBC_2.2.5 (libioP.h:905)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55023a4 is 228 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid write of size 8
==8990==    at 0x51AAB30: fclose@@GLIBC_2.2.5 (libioP.h:905)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55023a8 is 232 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 4
==8990==    at 0x51AAB4C: fclose@@GLIBC_2.2.5 (libioP.h:905)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55023a0 is 224 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 8
==8990==    at 0x51AAA30: fclose@@GLIBC_2.2.5 (iofclose.c:63)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x5502398 is 216 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 4
==8990==    at 0x51B6D71: _IO_file_finish@@GLIBC_2.2.5 (fileops.c:203)
==8990==    by 0x51AAA3E: fclose@@GLIBC_2.2.5 (iofclose.c:63)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x5502330 is 112 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 8
==8990==    at 0x51B8DC4: _IO_default_finish (genops.c:688)
==8990==    by 0x51AAA3E: fclose@@GLIBC_2.2.5 (iofclose.c:63)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55022f8 is 56 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 8
==8990==    at 0x51B8DD2: _IO_default_finish (genops.c:694)
==8990==    by 0x51AAA3E: fclose@@GLIBC_2.2.5 (iofclose.c:63)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x5502320 is 96 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 8
==8990==    at 0x51B8DF0: _IO_default_finish (genops.c:697)
==8990==    by 0x51AAA3E: fclose@@GLIBC_2.2.5 (iofclose.c:63)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x5502308 is 72 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 1
==8990==    at 0x51B7A99: _IO_un_link (genops.c:61)
==8990==    by 0x51AAA3E: fclose@@GLIBC_2.2.5 (iofclose.c:63)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55022c0 is 0 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 4
==8990==    at 0x51AAA3F: fclose@@GLIBC_2.2.5 (iofclose.c:64)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x5502380 is 192 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid read of size 8
==8990==    at 0x51AAAF0: fclose@@GLIBC_2.2.5 (iofclose.c:79)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x5502308 is 72 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid write of size 4
==8990==    at 0x51AAAD7: fclose@@GLIBC_2.2.5 (iofclose.c:84)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55022c0 is 0 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== Invalid free() / delete / delete[] / realloc()
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400A44: main (recover.c:57)
==8990==  Address 0x55022c0 is 0 bytes inside a block of size 568 free'd
==8990==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8990==    by 0x51AAAE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==8990==    by 0x400B2D: showmethepix (recover.c:73)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400BA1: showmethepix (recover.c:82)
==8990==    by 0x400A26: main (recover.c:52)
==8990== 
==8990== 
==8990== HEAP SUMMARY:
==8990==     in use at exit: 568 bytes in 1 blocks
==8990==   total heap usage: 53 allocs, 53 frees, 30,048 bytes allocated
==8990== 
==8990== LEAK SUMMARY:
==8990==    definitely lost: 0 bytes in 0 blocks
==8990==    indirectly lost: 0 bytes in 0 blocks
==8990==      possibly lost: 0 bytes in 0 blocks
==8990==    still reachable: 568 bytes in 1 blocks
==8990==         suppressed: 0 bytes in 0 blocks
==8990== Rerun with --leak-check=full to see details of leaked memory
==8990== 
==8990== For counts of detected and suppressed errors, rerun with: -v
==8990== ERROR SUMMARY: 27 errors from 23 contexts (suppressed: 0 from 0)

Answer 1

You can use your recursive function, but to do so, you need to pass the FILE* target variable by reference, rather than by value as you have done.

Recall Prof Malan's noswap.c example in Week 3. Or perhaps as an even simpler example, this:

#include <stdio.h>

void addOne(int num);
int main(void)
{
    int num = 99;
    printf("num before: %d\n", num);
    addOne(num);
    printf("num after: %d\n", num);
}

void addOne(int num)
{
    num = num + 1;
    printf("num + 1 = %d\n", num);
}

What you'll see if you run this:

num before: 99
num + 1 = 100
num after: 99

Notice how num in main is unchanged, because function arguments are local to the function. num inside addOne is not the same int as num in main. By that same logic, in your recursive function, target in main is not the same variable as target in showmethepix.

Let's say that target has the value of 0x100. You call showmethepix with that value which gets put into the local variable target. When you hit your first jpg signature, you call fclose(target) which will free the memory at location 0x100. Your function then calls fopen and saves the new address to target. Let's say it's 0x200. Then it will get closed and a new value will be assigned, etc. etc. Let's say you get to your last signature, and your last call to open sets target equal to 0x5000. And now you reach the end of the card, so your recursive function ends and you return to main.

What will be the value in the main variable target? It will, of course, still be 0x100, as it wasn't changed by your recursive function, just like num wasn't changed in my example. So, when you then call fclose(target), fclose will fail because the memory at 0x100 was freed already back in your recursive function. You'll see all those read/write fails (which are internal to fclose) and finally a failed free, due to the memory already having been freed. As well, you'll leave one block of memory unfreed (the one at 0x5000).

This is what you see with your program.

If you want to do your read/writes recursively (although, I don't see any real advantage to doing it this way), you should pass your target variable by reference.

Recall again the swap.c program, or this modification to my example:

#include <stdio.h>

void addOne(int* num);
int main(void)
{
    int num = 99;
    printf("num before: %d\n", num);
    addOne(&num);
    printf("num after: %d\n", num);
}

void addOne(int* num)
{
    *num = *num + 1;
    printf("num + 1 = %d\n", *num);
}

Here, I am passing num by reference, which means that addOne will actually update the value stored in main's num variable.

With your recursive function, you can do the same by using FILE** target as your argument, and passing &target to it. You'll have to dereference target inside the function as well, as I've done with *num in mine. In that way, once your recursive function finishes, the value in target for main to fclose will be the same value that you got in your last fopen.