Re: [sleuthkit-developers] memory leak and a patch

[Stefan P. <ste...@gm...>]

Re-sending a smaller data.tar.gz containing the files mentioned in this thread.

On Tue, May 06, 2014 at 04:04:08AM +0300, Stefan Petrea wrote:
> Patch attached
> 
> On Tue, May 06, 2014 at 04:01:42AM +0300, Stefan Petrea wrote:
> > Sending a new patch. Forgot to add a line in the previous one.
> > I've opened a github pull request for this here https://github.com/sleuthkit/sleuthkit/pull/329
> > 
> > On Tue, May 06, 2014 at 03:41:09AM +0300, Stefan Petrea wrote:
> > > Hi,
> > > 
> > > I'm using Sleuthkit and encountered a memory leak in TskFsDir::getFile. On a 20GB disk image,
> > > with ~10k files this leads to unallocated memory of ~2.5GB according to Valgrind.
> > >     
> > > I'm writing to the mailing list to provide details on the leak and a patch for it.
> > > 
> > > I wrote a small testcase that reproduces the leak in isolated.cpp
> > > 
> > > The output of Valgrind before the patch was applied( commit a9e2aa0e39cd8eeffd4cccf951b7b91a40f5e8c0 ), 
> > > can be found in valgrind-output-before-patch.txt and the output of Valgrind after the patch 
> > > was applied can be found in valgrind-output-after-patch.txt
> > > 
> > > The leaks reported by Valgrind all originate in TskFsDir::getFile. The unfreed objects
> > > were all of type TskFsFile.
> > > 
> > > The script find-leak.py automates GDB 7.6 and is designed to automatically debug isolated.cpp and
> > > at the same time collect information about the callgraph and pointers that cause the leak
> > > (Note: GDB 7.7 has a different Python API).
> > > 
> > > find-leak.py keeps track of all the pointers allocated through tsk_fs_file_alloc and all
> > > the pointers deallocated through tsk_fs_file_close which can be seen in the callgraph
> > > gdb-all-paths.png
> > > 
> > > The aim of this script (find-leak.py ) is to track the unfreed objects and find the call paths that lead
> > > to their allocation. This can be seen in the callgraph gdb-paths-that-caused-leak.png 
> > > which confirms Valgrind's report. The difference between this callgraph and the previous one is
> > > that this one only tracks the objects that have not been deallocated.
> > > 
> > > The patch that is attached to this e-mail contains a fix for this. It works by making TskFsDir a friend
> > > class of TskFsFile in order for TskFsDir::getFile to be able to set the TskFsFile::m_opened attribute
> > > to true, since if that attribute is not set to true, TskFsFile::~TskFsFile will not call
> > > tsk_fs_file_close which will cause the leak to occur.
> > > 
> > > I will also make a pull-request on github for this.
> > > 
> > > I look forward to your opinion on this patch. 
> > > 
> > > Best regards,
> > > Stefan
> > > 
> > > P.S.
> > > 
> > > As a sidenote, Valgrind also reported a leak found in tsk/base/tsk_error.c , originating from a call to
> > > tsk_error_get_info. I've noticed that there is a destructor handler free_error_info which deallocates
> > > the memory allocated by tsk_error_get_info, but that destructor is only called
> > > upon pthread_exit. The valgrind leak report for this can be found in valgrind-leak-tsk-error.txt
> > > To fix this, isolated.cpp also calls pthread_exit which solves that problem.

> diff --git a/tsk/fs/tsk_fs.h b/tsk/fs/tsk_fs.h
> index 39e0608..db4ef85 100644
> --- a/tsk/fs/tsk_fs.h
> +++ b/tsk/fs/tsk_fs.h
> @@ -2655,6 +2655,7 @@ class TskFsMeta {
>  * undefined. See TSK_FS_FILE for more details. 
>  */
>  class TskFsFile {
> +  friend class TskFsDir;
>    private:
>      TSK_FS_FILE * m_fsFile;
>      bool m_opened;
> @@ -2972,9 +2973,11 @@ class TskFsDir {
>       */
>      TskFsFile *getFile(size_t a_idx) const {
>          TSK_FS_FILE *fs_file = tsk_fs_dir_get(m_fsDir, a_idx);
> -        if (fs_file != NULL)
> -             return new TskFsFile(fs_file);
> -        else
> +        if (fs_file != NULL) {
> +             TskFsFile *f = new TskFsFile(fs_file);
> +             f->m_opened = true;
> +             return f;
> +        } else
>               return NULL;
>      };
>