CS50 week 4, "Recover problem", issue with memory leak

your textI actually, after a lot of sweating, finished the "recover" problem of the cs50 course. The last error I was getting was a memory leak and although I fixed it I still do not understand why I was getting it. So I would be glad if somebody could explain it to me.

The line that give me the error is the one below( BYTE is a typedef they suggested to create in the problem's specification, I created it like this: typedef uint8_t BYTE; and I used it somewhere else in the code without problems)

//Allocating space for images names
char *filename = malloc(1 *(sizeof(BYTE)));

And this is what valgrind says:

  ==2963== Memcheck, a memory error detector
  ==2963== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
  ==2963== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
  ==2963== Command: ./recover card.raw
   ==2963==
  ==2963== Invalid write of size 1
  ==2963==    at 0x4A17034: \_IO_default_xsputn (genops.c:394)
  ==2963==    by 0x4A17034: \_IO_default_xsputn (genops.c:370)
  ==2963==    by 0x4A09822: \_IO_padn (iopadn.c:64)
  ==2963==    by 0x49FF817: pad_func (vfprintf-internal.c:196)
  ==2963==    by 0x49FF817: \__vfprintf_internal (vfprintf-internal.c:1516)
  ==2963==    by 0x4A0AA08: \__vsprintf_internal (iovsprintf.c:95)
  ==2963==    by 0x49E99A7: sprintf (sprintf.c:30)
  ==2963==    by 0x1092FD: main (recover.c:52)
   ==2963==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==2963==    by 0x109241: main (recover.c:29)
  ==2963==
   ==2963== Invalid write of size 1
  ==2963==    at 0x4A17034: \_IO_default_xsputn (genops.c:394)
  ==2963==    by 0x4A17034: \_IO_default_xsputn (genops.c:370)
  ==2963==    by 0x49FED28: outstring_func (vfprintf-internal.c:239)
  ==2963==    by 0x49FED28: \__vfprintf_internal (vfprintf-internal.c:1516)
   ==2963==    by 0x4A0AA08: \__vsprintf_internal (iovsprintf.c:95)
  ==2963==    by 0x49E99A7: sprintf (sprintf.c:30)
  ==2963==    by 0x1092FD: main (recover.c:52)
  ==2963==  Address 0x4bb5262 is 1 bytes after a block of size 1 alloc'd
  ==2963==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==2963==    by 0x109241: main (recover.c:29)
  ==2963==
  ==2963== Invalid write of size 1
  ==2963==    at 0x4A17034: \_IO_default_xsputn (genops.c:394)
  ==2963==    by 0x4A17034: \_IO_default_xsputn (genops.c:370)
  ==2963==    by 0x49FF049: outstring_func (vfprintf-internal.c:239)
  ==2963==    by 0x49FF049: \__vfprintf_internal (vfprintf-internal.c:1593)
  ==2963==    by 0x4A0AA08: \__vsprintf_internal (iovsprintf.c:95)
  ==2963==    by 0x49E99A7: sprintf (sprintf.c:30)
  ==2963==    by 0x1092FD: main (recover.c:52)
   =2963==  Address 0x4bb5263 is 2 bytes after a block of size 1 alloc'd
  ==2963==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==2963==    by 0x109241: main (recover.c:29)
  ==2963==
  ==2963== Invalid write of size 1
  ==2963==    at 0x4A0AA0E: \__vsprintf_internal (iovsprintf.c:97)
  ==2963==    by 0x49E99A7: sprintf (sprintf.c:30)
  ==2963==    by 0x1092FD: main (recover.c:52)
  ==2963==  Address 0x4bb5267 is 6 bytes after a block of size 1 alloc'd
  ==2963==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==2963==    by 0x109241: main (recover.c:29)
  ==2963==
  ==2963== Syscall param openat(filename) points to unaddressable byte(s)
  ==2963==    at 0x4A9D6EB: open (open64.c:41)
  ==2963==    by 0x4A15135: \_IO_file_open (fileops.c:188)
  ==2963==    by 0x4A15491: \_IO_file_fopen@@GLIBC_2.2.5 (fileops.c:280)
  ==2963==    by 0x4A0872D: \__fopen_internal (iofopen.c:75)
  ==2963==    by 0x4A0872D: fopen@@GLIBC_2.2.5 (iofopen.c:86)
  ==2963==    by 0x109316: main (recover.c:56)
  ==2963==  Address 0x4bb5261 is 0 bytes after a block of size 1 alloc'd
  ==2963==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==2963==    by 0x109241: main (recover.c:29)
  ==2963==
  ==2963==
  ==2963== HEAP SUMMARY:
  ==2963==     in use at exit: 0 bytes in 0 blocks
  ==2963==   total heap usage: 104 allocs, 104 frees, 233,481 bytes allocated
  ==2963==
  ==2963== All heap blocks were freed -- no leaks are possible
  ==2963==
  ==2963== For lists of detected and suppressed errors, rerun with: -s
  ==2963== ERROR SUMMARY: 400 errors from 5 contexts (suppressed: 0 from 0)

And these are the check50 results:

:) recover.c exists.
:) recover.c compiles.
:) handles lack of forensic image
:) recovers 000.jpg correctly
:) recovers middle images correctly
:) recovers 049.jpg correctly
:( program is free of memory errors
valgrind tests failed; see log for more information.

But if a change the error line in:

 //Allocating space for images names
 char *filename = malloc(8 *(sizeof(char)));

everything works fine. The space allocated is 1 byte in both case and it is freed in both cases, why do I get an error if I use the typedef BYTE?

//Allocating space for images names
char *filename = malloc(1 *(sizeof(BYTE)));

BYTE is aliasing uint8_t, which is an (at least) 8-bit unsigned type, which itself is aliasing an unsigned char. This allocates memory for just 1 char/ byte. Assuming it's a string, it can only ever hold the null-byte.

 //Allocating space for images names
 char *filename = malloc(8 *(sizeof(char)));

> The space allocated is 1 byte in both case.

No, the space allocated is 8 bytes here (sizeof (char) is by definition 1), which is large enough to hold 000.jpg + \0. But you do not require malloc() here. The size of the allocation is known at compile time and remains constant, so it'd better to just use a plain array[8] of char.

> I used it somewhere else in the code without problems.

You got lucky. It's undefined behaviour to reference out of bounds memory (and that includes reading and writing).

> Permissible undefined behavior ranges from ignoring the situation
> completely with unpredictable results, to behaving during translation
> or program execution in a documented manner characteristic of the
> environment (with or without the issuance of a diagnostic message), to
> terminating a translation or execution (with the issuance of a
> diagnostic message).

The OS gives out memory in pages (usually 4k or so). If you ask malloc() for one byte, it might allocate a pagesize worth of memory instead (if the call to malloc() doesn't get optimized or it's out of memory). malloc() then organizes the page into chunks, do some book keeping, ensures the memory is suitably-aligned for any built-in type, and more. Writing past the end of the chunk may stay on the same page, and as far as the OS is concerned, you didn't access any illegal memory address. So your program might appear to work just fine until you go too far, and it segfaults, or erase your harddrive, or launch a missile (assuming you have the correct hardware installed), or worse, make demons fly out of your nose..

Remember the adage:

"Anything that can go wrong will go wrong, and at the worst possible time."