Ali Polatel
2016-10-03 20:18:25 UTC
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 <***@exherbo.org>
---
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
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 <***@exherbo.org>
---
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
2.10.0