Memory Leak using C++ Exceptions

Question

I'm struggling to write a little file handle class in C++11. I know there already is a lot in the STL to actually handle files, but for learning purposes I wanted to do this by myself.

Unfortunately I don't seem understand, what effect exceptions have on the memory leakage behavior of a C++ program because Valgrind tells me, there are 2 memory leaks in following code:

file.h

#ifndef FILE_H
#define FILE_H

#include <iostream>
#include <memory>
#include <string>

#include <stdio.h>

class FileDeleter {
public:
    void operator()(FILE *p);
};

class File {
public:
    File(const std::string path);

private:
    const std::string _path;
    std::unique_ptr<FILE, FileDeleter> _fp;

};

#endif // FILE_H

file.cpp

#include <cstring>
#include <iostream>
#include <stdexcept>
#include <utility>

#include <stdio.h>

#include "file.h"

void FileDeleter::operator()(FILE *p)
{
if (!p)
    return;

    if (fclose(p) == EOF)
        std::cerr << "FileDeleter: Couldn't close file" << std::endl;
}

File::File(const std::string path)
    : _path(std::move(path)),
      _fp(fopen(_path.c_str(), "a+"))
{
    if (!_fp)
        throw std::runtime_error("Couldn't open file");
}

main.cpp

#include <cstdlib>
#include "file.h"

int main()
{
    File my_file("/root/.bashrc");

    return EXIT_SUCCESS;
}

I did choose to open /root/.bashrc on purpose to actually make the File ctor throw the exception. If I don't throw there, Valgrind is perfectly happy. What am I missing here in using exceptions? How do you implement a simple file handle "correctly" (exception safe)?

Thanks in advance!

Note: the read/write operations are still missing, since I'm already struggling with the basics.

Edit #1: This is the actual Valgrind output, using --leak-check=full:

==7998== HEAP SUMMARY:
==7998==     in use at exit: 233 bytes in 3 blocks
==7998==   total heap usage: 5 allocs, 2 frees, 817 bytes allocated
==7998== 
==7998== 38 bytes in 1 blocks are possibly lost in loss record 1 of 3
==7998==    at 0x4C27CC2: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7998==    by 0x4EEC4F8: std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (in /usr/lib/libstdc++.so.6.0.18)
==7998==    by 0x4EEDC30: char* std::string::_S_construct<char const*>(char const*, char const*, std::allocator<char> const&, std::forward_iterator_tag) (in /usr/lib/libstdc++.so.6.0.18)
==7998==    by 0x4EEE047: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) (in /usr/lib/libstdc++.so.6.0.18)
==7998==    by 0x40137D: main (in ../a.out)
==7998== 
==7998== 43 bytes in 1 blocks are possibly lost in loss record 2 of 3
==7998==    at 0x4C27CC2: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7998==    by 0x4EEC4F8: std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (in /usr/lib/libstdc++.so.6.0.18)
==7998==    by 0x4EEDC30: char* std::string::_S_construct<char const*>(char const*, char const*, std::allocator<char> const&, std::forward_iterator_tag) (in /usr/lib/libstdc++.so.6.0.18)
==7998==    by 0x4EEE047: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) (in /usr/lib/libstdc++.so.6.0.18)
==7998==    by 0x400EBF: File::File(std::string) (in /home/frank/3Other/Code/Laboratory/c++/c++namedpipe/a.out)
==7998==    by 0x401390: main (in ../a.out)
==7998== 
==7998== 152 bytes in 1 blocks are possibly lost in loss record 3 of 3
==7998==    at 0x4C27730: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7998==    by 0x4E8F8F2: __cxa_allocate_exception (in /usr/lib/libstdc++.so.6.0.18)
==7998==    by 0x400E9B: File::File(std::string) (in /home/frank/3Other/Code/Laboratory/c++/c++namedpipe/a.out)
==7998==    by 0x401390: main (in ../a.out)
==7998== 
==7998== LEAK SUMMARY:
==7998==    definitely lost: 0 bytes in 0 blocks
==7998==    indirectly lost: 0 bytes in 0 blocks
==7998==      possibly lost: 233 bytes in 3 blocks
==7998==    still reachable: 0 bytes in 0 blocks
==7998==         suppressed: 0 bytes in 0 blocks

Edit #2: Fixed exception thrown in Destructor.

Edit #3: Removed FileException class, using std::runtime_error instead.

Edit #4: Added NULL check in deleter.

Answer 1

I see the following problems:

1. fclose(NULL) is not allowed ...
   • but fortunately std::unique_ptr doesn't call the deleter if get() == nullptr, so this should be ok here
2. FileDeleter:operator() is called from a destructor, which means it should never throw. In your specific case, any error returned from fclose will cause std::terminate to be called
3. your exception should store the string by value, not reference. That reference is to a temporary which will have been destroyed by the time you call what().
   • As Vlad & Praetorian pointed out, you can remove the buggy code instead of fixing it, and just let std::runtime_error handle that for you. TBH, unless you're planning to add some file-specific data to the exception, or you plan to catch file exceptions separately, you don't need this class at all.

Edit: with the valgrind output provided, it looks like stack unwinding never completed. Since my initial thought about FileDeleter::operator() being called and throwing looks wrong, a reasonable test would be: what happens if you stick a try/catch inside the body of main?

---

General notes:

1. never throw an exception inside a destructor
2. never call something else that might throw, from a destructor

because if your destructor is invoked during exception handling/stack unwinding, the program will terminate instantly.