scidvspc-users

[Scidvspc-users] [PATCH] fix memory leak in pgn parser (wrt. character detector)

[Ali P. <al...@ex...>]

Hello everyone,

I have spotted a memory leak in the pgn parser. This makes Scid pretty much
unusuable for me after working on the tree window for a while.

Below is the valgrind output of the incident:

138,880 bytes in 248 blocks are definitely lost in loss record 774 of 818
   at 0x4C2A0FC: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x44E57D: PgnParser::Reset(char const*) (in /usr/bin/tkscid)
   by 0x45358E: StoredLine::Init() (in /usr/bin/tkscid)
   by 0x453748: StoredLine::StoredLine(Position*) (in /usr/bin/tkscid)
   by 0x42D23C: sc_tree_search(void*, Tcl_Interp*, int, char const**) (in /usr/bin/tkscid)
   by 0x42EBEF: sc_tree(void*, Tcl_Interp*, int, char const**) (in /usr/bin/tkscid)
   by 0x50889B5: TclInvokeStringCommand (in /usr/lib/libtcl8.6.so)
   by 0x508D5F6: TclNRRunCallbacks (in /usr/lib/libtcl8.6.so)
   by 0x508F701: TclEvalEx (in /usr/lib/libtcl8.6.so)
   by 0x508FCB2: Tcl_EvalEx (in /usr/lib/libtcl8.6.so)
   by 0x543DFAD: Tk_BindEvent (in /usr/lib/libtk8.6.so)
   by 0x5441F5C: TkBindEventProc (in /usr/lib/libtk8.6.so)

The problem is PgnParser::Reset() is be called from multiple
different functions and during some cases CharConverter is
already allocated. Overwriting the variable to NULL causes
the previously allocated memory to leak.

To reproduce open a giant scid database in the tree and browse the games
for a while. In about half an hour scid running on a 4G RAM box eats all
the memory and hits OOM.

The change moves the initialisation of CharConverter and CharDetector to
PgnParser::Init(). We do not need to set them to NULL in PgnParser::Reset()
due to the fact that all callers also call CreateCharsetDetector() which takes
care of the initialisation.

I have been running Scid with this patch all day today and it seems to
cause no regressions and the memory usage is OK.

Signed-off-by: Ali Polatel <al...@ex...>
---
 src/pgnparse.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/pgnparse.cpp b/src/pgnparse.cpp
index bdfc4f9..a49e894 100644
--- a/src/pgnparse.cpp
+++ b/src/pgnparse.cpp
@@ -37,6 +37,8 @@ void
 PgnParser::Init ()
 {
     ErrorBuffer = new DString;
+    CharConverter = NULL;
+    CharDetector = NULL;
     Reset();
 }
 
@@ -56,8 +58,6 @@ PgnParser::Reset()
     ResultWarnings = true;
     NewlinesToSpaces = true;
     NumIgnoredTags = 0;
-    CharConverter = NULL;
-    CharDetector = NULL;
 }
 
 void
-- 
2.10.0

Re: [Scidvspc-users] [PATCH] fix memory leak in pgn parser (wrt. character detector)

[Steve A <ste...@gm...>]

Many thanks for that Ali. :)

Steven

On Tue, Oct 4, 2016 at 6:18 AM, Ali Polatel <al...@ex...> wrote:

> Hello everyone,
>
> I have spotted a memory leak in the pgn parser. This makes Scid pretty much
> unusuable for me after working on the tree window for a while.
>
> Below is the valgrind output of the incident:
>
> 138,880 bytes in 248 blocks are definitely lost in loss record 774 of 818
>    at 0x4C2A0FC: operator new(unsigned long) (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>    by 0x44E57D: PgnParser::Reset(char const*) (in /usr/bin/tkscid)
>    by 0x45358E: StoredLine::Init() (in /usr/bin/tkscid)
>    by 0x453748: StoredLine::StoredLine(Position*) (in /usr/bin/tkscid)
>    by 0x42D23C: sc_tree_search(void*, Tcl_Interp*, int, char const**) (in
> /usr/bin/tkscid)
>    by 0x42EBEF: sc_tree(void*, Tcl_Interp*, int, char const**) (in
> /usr/bin/tkscid)
>    by 0x50889B5: TclInvokeStringCommand (in /usr/lib/libtcl8.6.so)
>    by 0x508D5F6: TclNRRunCallbacks (in /usr/lib/libtcl8.6.so)
>    by 0x508F701: TclEvalEx (in /usr/lib/libtcl8.6.so)
>    by 0x508FCB2: Tcl_EvalEx (in /usr/lib/libtcl8.6.so)
>    by 0x543DFAD: Tk_BindEvent (in /usr/lib/libtk8.6.so)
>    by 0x5441F5C: TkBindEventProc (in /usr/lib/libtk8.6.so)
>
> The problem is PgnParser::Reset() is be called from multiple
> different functions and during some cases CharConverter is
> already allocated. Overwriting the variable to NULL causes
> the previously allocated memory to leak.
>
> To reproduce open a giant scid database in the tree and browse the games
> for a while. In about half an hour scid running on a 4G RAM box eats all
> the memory and hits OOM.
>
> The change moves the initialisation of CharConverter and CharDetector to
> PgnParser::Init(). We do not need to set them to NULL in PgnParser::Reset()
> due to the fact that all callers also call CreateCharsetDetector() which
> takes
> care of the initialisation.
>
> I have been running Scid with this patch all day today and it seems to
> cause no regressions and the memory usage is OK.
>
> Signed-off-by: Ali Polatel <al...@ex...>
> ---
>  src/pgnparse.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/pgnparse.cpp b/src/pgnparse.cpp
> index bdfc4f9..a49e894 100644
> --- a/src/pgnparse.cpp
> +++ b/src/pgnparse.cpp
> @@ -37,6 +37,8 @@ void
>  PgnParser::Init ()
>  {
>      ErrorBuffer = new DString;
> +    CharConverter = NULL;
> +    CharDetector = NULL;
>      Reset();
>  }
>
> @@ -56,8 +58,6 @@ PgnParser::Reset()
>      ResultWarnings = true;
>      NewlinesToSpaces = true;
>      NumIgnoredTags = 0;
> -    CharConverter = NULL;
> -    CharDetector = NULL;
>  }
>
>  void
> --
> 2.10.0
>
> ------------------------------------------------------------
> ------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> _______________________________________________
> Scidvspc-users mailing list
> Sci...@li...
> https://lists.sourceforge.net/lists/listinfo/scidvspc-users
>
>