Invalid free() / delete / delete[] / realloc() for fclose()?

Question

I try to run/compile OpenTibia Server on Linux64. Little tweaks, compiled and everything seemed fine. Yet, Valgrind says:

==32360== Invalid free() / delete / delete[] / realloc()
==32360==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32360==    by 0x6074AE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==32360==    by 0x41CF8D: FileLoader::~FileLoader() (fileloader.cpp:49)
==32360==    by 0x45DB1B: Items::loadFromOtb(std::string) (itemloader.h:232)
==32360==    by 0x4067D7: main (otserv.cpp:564)
==32360==  Address 0x8126590 is 0 bytes inside a block of size 568 free'd
==32360==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32360==    by 0x6074AE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
==32360==    by 0x41D268: FileLoader::openFile(char const*, bool, bool) (fileloader.cpp:92)
==32360==    by 0x45DB00: Items::loadFromOtb(std::string) (items.cpp:230)
==32360==    by 0x4067D7: main (otserv.cpp:564)

Now the code goes, for FileLoader (especially destructor):

/*somewhere in the header*/
FILE* m_file;

FileLoader::FileLoader() {
    m_file = NULL;
    m_buffer = new unsigned char[1024];
    //cache, some cache data
    memset(m_cached_data, 0, sizeof(m_cached_data));
}

FileLoader::~FileLoader() {
    if(m_file){
        fclose(m_file);
        m_file = NULL;
     }

    delete[] m_buffer;

    for(int i = 0; i < CACHE_BLOCKS; i++){
        if(m_cached_data[i].data)
            delete m_cached_data[i].data;
        }
}

bool FileLoader::openFile(const char* filename, bool write, bool caching /*= false*/){
    if(write) {/*unimportant*/}
    else {
    unsigned long version;
    m_file = fopen(filename, "rb");
    if(m_file){
        fread(&version, sizeof(unsigned long), 1, m_file);
        if(version > 0){/*version is 0*/}
            else{
                if(caching){
                    m_use_cache = true;
                    fseek(m_file, 0, SEEK_END);
                    int file_size = ftell(m_file);
                    m_cache_size = min(32768, max(file_size/20, 8192)) & ~0x1FFF;
                }
                return true;
            }
        }
        else{
            m_lastError = ERROR_CAN_NOT_OPEN;
            return false;
        }
    }
}

ItemLoader is just an extension to FileLoader:

class ItemLoader : public FileLoader {/*Overrides nothing*/};

Now to the function in Items:

int Items::loadFromOtb(std::string file) {
    ItemLoader f;
    if(!f.openFile(file.c_str(), false, true)){return f.getError();}

    //...Loading, processing, reading from file and stuff...

    //delete &f; //I tried this but didn't change anything
    return ERROR_NONE;
}

The question is, does Valgrind point to problem with fclose or rather something else? Also note that the application uses libboost (if this has anything to do). I tried to be as specific as possible

Answer 1

Vagrind shows you the problem directly -- you're calling fclose on the same FILE descriptor twice:

            ==32360== Invalid free() / delete / delete[] / realloc()
            ==32360==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
second call ==32360==    by 0x6074AE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
--------->> ==32360==    by 0x41CF8D: FileLoader::~FileLoader() (fileloader.cpp:49)
            ==32360==    by 0x45DB1B: Items::loadFromOtb(std::string) (itemloader.h:232)
            ==32360==    by 0x4067D7: main (otserv.cpp:564)
            ==32360==  Address 0x8126590 is 0 bytes inside a block of size 568 free'd
            ==32360==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
first call  ==32360==    by 0x6074AE4: fclose@@GLIBC_2.2.5 (iofclose.c:85)
--------->> ==32360==    by 0x41D268: FileLoader::openFile(char const*, bool, bool) (fileloader.cpp:92)
            ==32360==    by 0x45DB00: Items::loadFromOtb(std::string) (items.cpp:230)
            ==32360==    by 0x4067D7: main (otserv.cpp:564)

The second call is in your destructor at line 49, the first in openFile at line 92.

Answer 2

It looks like FileLoader exists to do a form of caching/buffering. There is no need to do your own buffering when using FILE* or IOStreams, they do that for you. FileLoader adds another buffer on top of that.

You may like to refactor FileLoader to drop all buffering and only provide serialization functionality for your classes, while delegating all I/O and associated buffering to FILE* or IOStreams.

Answer 3

The elephant in the room: Ahrrr. This be C++. Why use FILE? fstream will take care of this nonsense for you (well... Some of it) without dropping down into the leftovers of C.

Onto the error.

Looks to me like Valgrind says the program is closing m_file twice. Not a good idea.

Why is the program closing the m_file twice? Most likely answer is FileLoader destructor is being called twice.

Here I wander into undefined territory because all I can do is infer information left out of the question from reading between the lines. Can't even close this question and point it to What is The Rule of Three? because we can't be sure. It can be closed for being unclear and unanswerable though, so before it is...

Here are my assumptions:

1. m_file is a class member of FileLoader. If it isn't and it's global, that's a bad design not worth trying to fix.
2. somewhere in //...Loading, processing, reading from file and stuff... f is being copied. Please give f a real, descriptive name. The debugging time you save could be your own.
3. FileLoader and ItemLoader violate the Rule of Three. If you don't know what I'm talking about, read the above link about the Rule of Three. Seriously. Read it. Even if I'm wrong, read it.

How this can happen:

Say you have function void doesStuff(ItemLoader loader) and it gets called in the black hole of //...Loading, processing, reading from file and stuff.... Note the pass by value of loader. This means it's going to be copied and become a temporary variable with its lifetime limited by the scope of the function.

Because FileLoader is not Rule of Three compliant, loader gets a copy of f's m_file and whatever other members FileLoader has.

doesStuff does stuff and returns. loader goes out of scope and is destroyed. ~FileLoader() is called. m_file is not null, the file is closed and m_file is set to null. No point setting it to null, by the way. It's about to vanish.

We return to the calling function where f now has an invalid FILE pointer because the copy just closed the file.

How to test the theory:

Put std::cout << "~FileLoader() called. m_file = " << m_file << std::endl; at the top of FileLoader's destructor to see now many times it gets called compared to the number of times you thought you opened it.

The fix:

Make your objects Rule of Three compliant OR make them uncopyable and always pass by reference.

Making FileLoader Rule of Three compliant is non-trivial effort. FILE pointers and do not copy well, and this leave you playing games with weak pointers to ensure the sucker isn't closed before everyone is done with it.

It also makes a good case for deleting the copy constructor and assignment operator so FileLoader can't be copied. That way the compiler can warn you when you're trying to do something dumb like pass by value when you want to pass by reference.

fstream does not copy at all, preventing you from getting in this mess in the first place. But it does give an arcane stream of error messages if you don't know what a deleted function is.

Here is a blob of test code to show what I think is happening:

#include <iostream>
#include <cstdio>
class FILETest
{
public:
    FILE* filep;
    FILETest(): filep(NULL)
    {
        std::cout << "FILETest constructor" << std::endl;
        
    }
    ~FILETest()
    {
        std::cout << "FILETest destructor" << std::endl;
    }
};

void func(FILETest t)
{
    (void)t;
}
int main(int argc, char** argv)
{
    (void) argc;
    (void) argv;

    FILETest t;

    func(t);
    return 0;
}

And how fstream would have prevented this:

#include <iostream>
#include <fstream>


class fstreamtest
{
public:
    std::fstream file;
    fstreamtest()
    {
        std::cout << "fstreamtest constructor" << std::endl;

    }
    ~fstreamtest()
    {
        std::cout << "fstreamtest destructor" << std::endl;
    }
};

void func(fstreamtest t)
{
    (void)t;
}
int main(int argc, char** argv)
{
    (void) argc;
    (void) argv;

    fstreamtest t;

    func(t);
    return 0;
}

Answer 4

Okay, disregard the previous. Sorry for misinforming, because I forgot to tell about the metaimportant facts:

1. Code is 100% working for Win32. I am compiling it on Linux64.
2. The destructor of FileLoader (is virtual) and fires.

3. I added the following

   FileLoader::~FileLoader() {
      std::cout << "destructor fired\n";
      if(m_file){
          fclose(m_file);
          m_file = NULL;
          std::cout << "destructor closed file\n";
       }
   
       delete[] m_buffer;
       std::cout << "destructor deleted buffer\n";
   
      for(int i = 0; i < CACHE_BLOCKS; i++){
           if(m_cached_data[i].data)
              delete m_cached_data[i].data;
           }
      std::cout << "destructor deleted cache\n";
   }
   

   And the output was:

   destructor fired
   <valgrind crying>
   destructor closed file
   

   So it didn't have anything to do with buffers. I found the problem, the version actually mismatched, closed file, but didn't NULL it. Thus was actually a bug in original code.

The real solution to the problem was that Win32 long = Linux64 int. That solved part of the issues.