1

I'm currently in the memory-leak detection stage of debugging, and am running valgrind --leak-check=full --show-leak-kinds=all on my executable. However, I'm getting some confusing output in a loss record:

==26628== 2 bytes in 1 blocks are indirectly lost in loss record 2 of 343
==26628==    at 0x4C2B0E0: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==26628==    by 0x436EF6: void std::vector<unsigned char, std::allocator<unsigned char> >::_M_range_insert<char*>(__gnu_cxx::__normal_iterator<unsigned char*, std::vector<unsig$
==26628==    by 0x4368EF: giga::Page::aux_load() (stl_vector.h:1291)
==26628==    by 0x43499D: cachepp::SerialCache<cachepp::SimpleSerialCacheData, giga::Page>::allocate(std::shared_ptr<giga::Page> const&) (lineinterface.template:34)
...

The problem is in the trace for giga::Page::aux_load -- this function is defined by me, and most definitely not in the standard lib. I'm not sure why the line was not reported on this output line from the appropriate calling file.

I wasn't sure if code was necessary for this, but in case I do --

void giga::Page::aux_load() {
    this->data.clear();
    // deallocate the data vector
    std::vector<uint8_t>().swap(this->data);

    if(!this->get_is_dirty()) {
            // load data into the page
            FILE *fp = fopen(this->get_filename().c_str(), "r");
            if(fseek(fp, this->file_offset, SEEK_SET) == -1) {
                    throw(exceptionpp::RuntimeError("giga::Page::aux_load", "invalid result returned from fseek"));
            }

            char *data = (char *) calloc(this->get_size(), sizeof(char));

            if(fread((void *) data, sizeof(char), this->get_size(), fp) < this->get_size()) {
                    throw(exceptionpp::RuntimeError("giga::Page::aux_load", "invalid result returned from fread"));
            }
            fclose(fp);

            this->data.insert(this->data.end(), data, data + this->get_size());

            free((void *) data);
    }
}

Any help would be greatly appreciated!

mzhang
  • 392
  • 3
  • 12
  • 2
    Your code has a memory leak if you throw the exception `throw(exceptionpp::RuntimeError`. Why didn't you use a `std::vector` instead of using `calloc`? – PaulMcKenzie Aug 24 '14 at 06:45
  • 1
    Using `data` as you local var name and `data` as your member name doesn't exactly help matters much in terms of clarity either. And `2 of 343` tells us there is quite a bit you're *not* showing us, but I'm betting the thing that *owns* that vector (the page) is likewise being leaked. Thus the "indirect" notation in the valgrind report. – WhozCraig Aug 24 '14 at 07:04
  • 3
    Also, you have a dangling open file pointer if you throw an exception. I think it's time you learn RAII techniques, since your code is a classic example of what happens if it isn't used. – PaulMcKenzie Aug 24 '14 at 07:05

2 Answers2

2

This is likely to be caused by valgrind stacktrace not showing inlined function call defined at stl_vector.h:1291 and called somewhere inside aux_load.

The next version of Valgrind has support for showing inlined function, using the option --read-inline-info=yes.

So, get and compile the latest valgrind svn version See http://www.valgrind.org/downloads/repository.html for instruction about how to get, configure and compile the valgrind svn version

phd
  • 3,669
  • 1
  • 11
  • 12
0

This may not be the reason for the leaks in your report, but your code definitely will leak memory, without a doubt. It will leak if you throw an exception:

throw(exceptionpp::RuntimeError("giga::Page::aux_load", "invalid result returned from fread"));

The reason why it is a leak is that you previously called calloc, and the line where you free the data is never reached. You also have a dangling open file pointer if any exception is thrown by your function.

The answer to correct this is short and sweet:

  1. Use std::ifstream, not FILE*. This will guarantee that your file is closed on function return, regardless of the reason for the return.

  2. Use std::vector<char> dataV(get_size()), not calloc. This will guarantee that the memory is deallocated on function return, regardless of the reason for the return (in this case, use &dataV[0] or if using C++11 dataV.data() to get a pointer to the char buffer).

The topic you should read about is RAII: What is meant by Resource Acquisition is Initialization (RAII)?

Rewrite your code using these constructs above, and you will relieve yourself of the leaks in the function you posted.

The other changes you should use:

  1. Check for get_size() == 0. If it's 0, then you can return immediately without having to open files.

  2. There is no need for you to use this-> everywhere. It clutters the code and makes it harder to debug if there is an issue.

Community
  • 1
  • 1
PaulMcKenzie
  • 34,698
  • 4
  • 24
  • 45
  • Thanks, that was very helpful in terms of fixing the leak, and I will definitely refactor my code to be more robust -- however, I'm not sure if that addresses the original question (perhaps I wasn't very clear): why is the line reported in the relevant function not in a user file, but a file from the standard lib? – mzhang Aug 24 '14 at 12:58
  • Also, regarding the use of 'this' in code -- I actually find it helpful because I know immediately that the functions and values were not defined ad hoc, or within the local function, but rather in the class scope -- for me at least, it improves the workflow. I'll read more into this, but I assume from what you suggested that this isn't standard programming practice? – mzhang Aug 24 '14 at 13:24
  • @mzhang Leaks can be caused by not properly destroying `giga` objects in your program. For example, if `giga` is derived from a base class, and you're creating `giga` object dynamically through a base class pointer, the base class needs a virtual destructor. That is only one scenario. Since you didn't post your whole code, given what you did post, I would bet that you're doing things similar to what you were doing in this function -- that is creating objects dynamically, and never getting to the point where these are objects are deallocated properly. – PaulMcKenzie Aug 24 '14 at 18:07