0

There is only one use of malloc in the program and it is freed before the program exits:

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

int main(int argc, char *argv[])
{
    if (argc != 2)
    {
        printf("Only one argument is required!");
        return 1;
    }

    FILE *img;
    FILE *f = fopen(argv[1], "r");
    uint8_t buffer[512];
    char *filename = malloc(7);
    int counter = 0;
    int open = 0;
    int start = 1;
    int read;

    while (start)
    {
        while ((read = fread(&buffer, 1, 512, f)) == 512)
        {
            if (((buffer[0] == 0xff) && (buffer[1] == 0xd8) && (buffer[2] == 0xff) &&
                 (buffer[3] >= 0xe0) && (buffer[3] <= 0xef)))
            {
                sprintf(filename, "%03i.jpg", counter);
                img = fopen(filename, "w");
                fwrite(&buffer, 1, 512, img);
                open = 1;
                counter++;
            }
            else if (open)
            {
                fwrite(&buffer, 1, 512, img);
            }
        }
        if (read < 512)
        {
            fclose(img);
            open = 0;
        }
        if (read == 0)
        {
            start = 0;
            free(filename);
        }
    }
    return 0;
}

This is the output of Valgrind:

==87240== HEAP SUMMARY:
==87240==     in use at exit: 0 bytes in 0 blocks
==87240==   total heap usage: 0 allocs, 0 frees, 0 bytes allocated
==87240== 
==87240== All heap blocks were freed -- no leaks are possible
==87240== 
==87240== For lists of detected and suppressed errors, rerun with: -s
==87240== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

I don't know if I'm reading anything wrong because Check50 is still saying that the program is not free of memory errors.

1 Answer 1

1

I get a different result when I run the program with valgrind. It's showing both an invalid write and memory leaks.

$ valgrind ./recover card.raw
==21722== Memcheck, a memory error detector
==21722== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==21722== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==21722== Command: ./recover card.raw
==21722== 
==21722== Invalid write of size 1
==21722==    at 0x4A0998E: __vsprintf_internal (iovsprintf.c:97)
==21722==    by 0x49E8927: sprintf (sprintf.c:30)
==21722==    by 0x1092E5: main (recover.c:29)
==21722==  Address 0x4bb4267 is 0 bytes after a block of size 7 alloc'd
==21722==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==21722==    by 0x10920E: main (recover.c:16)
==21722== 
==21722== Syscall param openat(filename) points to unaddressable byte(s)
==21722==    at 0x4A9C72B: open (open64.c:41)
==21722==    by 0x4A140B5: _IO_file_open (fileops.c:188)
==21722==    by 0x4A14411: _IO_file_fopen@@GLIBC_2.2.5 (fileops.c:280)
==21722==    by 0x4A076AD: __fopen_internal (iofopen.c:75)
==21722==    by 0x4A076AD: fopen@@GLIBC_2.2.5 (iofopen.c:86)
==21722==    by 0x1092F8: main (recover.c:30)
==21722==  Address 0x4bb4267 is 0 bytes after a block of size 7 alloc'd
==21722==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==21722==    by 0x10920E: main (recover.c:16)
==21722== 
==21722== 
==21722== HEAP SUMMARY:
==21722==     in use at exit: 23,600 bytes in 50 blocks
==21722==   total heap usage: 103 allocs, 53 frees, 232,975 bytes allocated
==21722== 
==21722== 472 bytes in 1 blocks are still reachable in loss record 1 of 2
==21722==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==21722==    by 0x4A0764D: __fopen_internal (iofopen.c:65)
==21722==    by 0x4A0764D: fopen@@GLIBC_2.2.5 (iofopen.c:86)
==21722==    by 0x109200: main (recover.c:14)
==21722== 
==21722== 23,128 bytes in 49 blocks are still reachable in loss record 2 of 2
==21722==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==21722==    by 0x4A0764D: __fopen_internal (iofopen.c:65)
==21722==    by 0x4A0764D: fopen@@GLIBC_2.2.5 (iofopen.c:86)
==21722==    by 0x1092F8: main (recover.c:30)
==21722== 
==21722== LEAK SUMMARY:
==21722==    definitely lost: 0 bytes in 0 blocks
==21722==    indirectly lost: 0 bytes in 0 blocks
==21722==      possibly lost: 0 bytes in 0 blocks
==21722==    still reachable: 23,600 bytes in 50 blocks
==21722==         suppressed: 0 bytes in 0 blocks
==21722== 
==21722== For lists of detected and suppressed errors, rerun with: -s
==21722== ERROR SUMMARY: 100 errors from 2 contexts (suppressed: 0 from 0)

For starters, there's this:

char *filename = malloc(7);

It's not allocating enough space, so during runtime, it's overwriting memory following this allocation. The filename is 7 chars, 3 for the number, 1 for the dot and 3 for the "jpg" suffix. There ALWAYS needs to be one more byte allocated for the EOS marker, 0x00, or it will cause all kinds of problems.

The next thing I have a problem with is the outer while loop. It's not necessary and adds unneeded complexity. The current output file can be closed just before a new one is open, so it can be done inside the inner while loop.

When the inner loop terminates because the last read fails (if everything is done right), there's nothing left to do except to close files, free memory and close the program. The added complexity may well be causing some issues.

Of course, when you remove the outer loop, the code still needs to make sure all files are closed.

Simply put, the program can be simplified considerably.

3
  • Valgrind is still giving an error on line img = fopen(filename, "w"); I can't understand it, it's very cryptic. I even tried to use fclose after the loop ended (there is one loop now...) but the problem still persists. What am I supposed to check? ==33536== 23,128 bytes in 49 blocks are still reachable in loss record 1 of 1 ==33536== at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==33536== by 0x4A0664D: __fopen_internal (iofopen.c:65) ==33536== by 0x4A0664D: fopen@@GLIBC_2.2.5 (iofopen.c:86) ==33536== by 0x1092D1: main (recover.c:27) Commented May 13 at 16:01
  • 1
    thought 1 : you need a fclose before every fopen except the first one.. thought 2: post an update section with your updated code.
    – UpAndAdam
    Commented May 13 at 17:50
  • If you haven't resolved this, please start a new question with the current code. There are likely too many changes. Having said that, look at the valgrind result. There are 49 blocks of code still reachable. That's one less than the number of output files. Sounds like a problem with either the code handling the first 49 or the last 49. Did you close each output file before opening the next?
    – Cliff B
    Commented May 14 at 0:47

You must log in to answer this question.

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