From 6935e1da64fd25d5132644b060d228402bd612ce Mon Sep 17 00:00:00 2001 From: Jürgen Knödlseder Date: Thu, 4 Aug 2016 22:09:45 +0200 Subject: [PATCH] Fix invalid pointer problem in GLog copies (#1823) --- ChangeLog | 1 + NEWS | 7 +++++++ src/app/GLog.cpp | 20 +++++++++++--------- test/test_GApplication.py | 220 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------------------------------------- 4 files changed, 146 insertions(+), 102 deletions(-) diff --git a/ChangeLog b/ChangeLog index 8235a78..ad61609 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,7 @@ * Version 1.2.0 released ========================= + Fix invalid pointer problem in GLog copies (#1823) Fix GSkyMap::publish() bug for empty sky maps (#1835) Fix loss of application name and version (#1835) Add methods for logging in applications (#1828) diff --git a/NEWS b/NEWS index 9f0ebba..1c9ee61 100644 --- a/NEWS +++ b/NEWS @@ -62,6 +62,13 @@ The application name and version and now conserved in a GApplication::clear() method call and the application is properly initialised like in the call of a constructor (#1835). +The GLog::copy_members() method simply copied the file pointer, hence after +a copy of a log instance there were two instances holding the same pointer +to the file, and as soon as one instance closes the log file the file +pointer in the second instance became invalid. This has been fixed by +duplicating the file descriptor and opening the log file in the copied +instance using the duplicated descriptor. This solves the problems (#1823). + 5. Base module -------------- diff --git a/src/app/GLog.cpp b/src/app/GLog.cpp index 9dc15d3..0e24512 100644 --- a/src/app/GLog.cpp +++ b/src/app/GLog.cpp @@ -36,6 +36,7 @@ #include #include #include +#include // dup #include "GLog.hpp" #include "GTools.hpp" @@ -552,7 +553,7 @@ void GLog::init_members(void) /***********************************************************************//** * @brief Copy class members * - * @param[in] log Object from which members which should be copied. + * @param[in] log Logger. ***************************************************************************/ void GLog::copy_members(const GLog& log) { @@ -563,12 +564,20 @@ void GLog::copy_members(const GLog& log) m_stderr = log.m_stderr; m_use_date = log.m_use_date; m_linestart = log.m_linestart; - m_file = log.m_file; m_filename = log.m_filename; m_name = log.m_name; m_buffer = log.m_buffer; m_chatter = log.m_chatter; + // Copy file pointer by duplicating the file descriptor and opening + // the duplicated file descriptor + if (log.m_file != NULL) { + m_file = fdopen(dup(fileno(log.m_file)), "a"); + } + else { + m_file = NULL; + } + // Return return; } @@ -820,10 +829,3 @@ void GLog::append(std::string arg) // Return return; } - - -/*========================================================================== - = = - = Friends = - = = - ==========================================================================*/ diff --git a/test/test_GApplication.py b/test/test_GApplication.py index 7c5dd74..46e5765 100755 --- a/test/test_GApplication.py +++ b/test/test_GApplication.py @@ -1,7 +1,7 @@ # ========================================================================== -# This module performs unit tests for the GammaLib application module. +# This module performs unit tests for the GammaLib application module # -# Copyright (C) 2012-2015 Juergen Knoedlseder +# Copyright (C) 2012-2016 Juergen Knoedlseder # # This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -25,150 +25,184 @@ import gammalib # ========================================== # class Test(gammalib.GPythonTestSuite): """ - Test class for GammaLib application module. + Test class for GammaLib application module """ # Constructor def __init__(self): """ - Constructor. + Constructor """ # Call base class constructor gammalib.GPythonTestSuite.__init__(self) # Set members - self.logfile = "test_logger.log" + self._logfile = 'test_logger.log' # Return return - # Set test functions + + # Private methods + def _check_log_file(self, ref, calls): + """ + Check log file versus a reference + + Parameters + ---------- + ref : list of str + Reference lines + calls : list of str + Methods that are tested + """ + # Open log file, read all line and close file + file = open(self._logfile, 'r') + lines = file.readlines() + file.close() + + # Check all lines versus reference. There is a special kluge in case + # that the reference is "true\n" as in some Python/SWIG version this + # is displayed by "1\n". Both variants are tested to avoid version + # dependent test results. + for i, line in enumerate(lines): + if ref[i] == 'true\n': + self.test_assert(line == ref[i] or line == '1\n', + 'Check '+calls[i]+': '+line) + else: + self.test_value(line, ref[i], 'Check '+calls[i]+': '+line) + + # Return + return + + + # Public methods def set(self): """ - Set all test functions. + Set all test functions """ # Set test name - self.name("app") + self.name('app') # Append tests - self.append(self.test_log, "Test GLog") - self.append(self.test_pars, "Test GApplicationPars") + self.append(self.test_log, 'Test GLog') + self.append(self.test_pars, 'Test GApplicationPars') # Return return - # Test GLog def test_log(self): """ - Test GLog. + Test GLog """ # Allocate logger - log = gammalib.GLog(self.logfile, True) + log = gammalib.GLog(self._logfile, True) # Test print methods - log("Test __call__(std::string)") - log("\n") + log('Test __call__(std::string)\n') log(True) - log("\n") + log('\n') log(41) - log("\n") + log('\n') log(3.1415) - log("\n") - log.parformat("This is a parameter") - log("\n") - log.toupper("upper case") - log("\n") - log.tolower("LOWER CASE") - log("\n") - log.fill("Higgs", 3) - log("\n") - log.left("Left", 10) - log("\n") - log.right("Right", 10) - log("\n") - log.centre("Centre", 10) - log("\n") - log.header0("Header 0") - log("\n") - log.header1("Header 1") - log.header2("Header 2") - log.header3("Header 3") + log('\n') + log.parformat('This is a parameter') + log('\n') + log.toupper('upper case') + log('\n') + log.tolower('LOWER CASE') + log('\n') + log.fill('Higgs', 3) + log('\n') + log.left('Left', 10) + log('\n') + log.right('Right', 10) + log('\n') + log.centre('Centre', 10) + log('\n') + log.header0('Header 0') + log('\n') + log.header1('Header 1') + log.header2('Header 2') + log.header3('Header 3') # Close logger log.close() # Set reference ref = [] - ref.append("Test __call__(std::string)\n") - #ref.append("1\n") # Old swig version - ref.append("true\n") # New swig version (3.x.y) - ref.append("41\n") - ref.append("3.1415\n") - ref.append(" This is a parameter .......: \n") - ref.append("UPPER CASE\n") - ref.append("lower case\n") - ref.append("HiggsHiggsHiggs\n") - ref.append("Left \n") - ref.append(" Right\n") - ref.append(" Centre \n") - ref.append("\n") - ref.append("+==========+\n") - ref.append("| Header 1 |\n") - ref.append("+==========+\n") - ref.append("+----------+\n") - ref.append("| Header 2 |\n") - ref.append("+----------+\n") - ref.append("=== Header 3 ===\n") + ref.append('Test __call__(std::string)\n') + ref.append('true\n') # New swig version (3.x.y) + ref.append('41\n') + ref.append('3.1415\n') + ref.append(' This is a parameter .......: \n') + ref.append('UPPER CASE\n') + ref.append('lower case\n') + ref.append('HiggsHiggsHiggs\n') + ref.append('Left \n') + ref.append(' Right\n') + ref.append(' Centre \n') + ref.append('\n') + ref.append('+==========+\n') + ref.append('| Header 1 |\n') + ref.append('+==========+\n') + ref.append('+----------+\n') + ref.append('| Header 2 |\n') + ref.append('+----------+\n') + ref.append('=== Header 3 ===\n') # Set calls for error messages calls = [] - calls.append("__call__(std::string)") - calls.append("__call__(bool)") - calls.append("__call__(int)") - calls.append("__call__(double)") - calls.append("parformat()") - calls.append("toupper()") - calls.append("tolower()") - calls.append("fill()") - calls.append("left()") - calls.append("right()") - calls.append("centre()") - calls.append("header0()") - calls.append("header1()") - calls.append("header1()") - calls.append("header1()") - calls.append("header2()") - calls.append("header2()") - calls.append("header2()") - calls.append("header3()") - - # Check file - file = open(self.logfile, "r") - lines = file.readlines() - file.close() - for i, line in enumerate(lines): - if ref[i] == "true\n": - self.test_assert(line == ref[i] or line == "1\n", - "Test "+calls[i]+": "+line) - else: - self.test_assert(line == ref[i], "Test "+calls[i]+": "+line) + calls.append('__call__(std::string)') + calls.append('__call__(bool)') + calls.append('__call__(int)') + calls.append('__call__(double)') + calls.append('parformat()') + calls.append('toupper()') + calls.append('tolower()') + calls.append('fill()') + calls.append('left()') + calls.append('right()') + calls.append('centre()') + calls.append('header0()') + calls.append('header1()') + calls.append('header1()') + calls.append('header1()') + calls.append('header2()') + calls.append('header2()') + calls.append('header2()') + calls.append('header3()') + + # Check log file + self._check_log_file(ref, calls) + + # Create copy of logger + cpy_log = log.copy() + + # Print one more line into copy + cpy_log.header3('Header 3bis') + + # Append reference and call for error message + ref.append('=== Header 3bis ===\n') + calls.append('header3()') + + # Check log file again + self._check_log_file(ref, calls) # Return return - # Test GApplicationPars def test_pars(self): """ - Test GApplicationPars. + Test GApplicationPars """ # Test GApplicationPars constructor with bad filename. - self.test_try("Test GApplicationPars constructor with bad filename") + self.test_try('Test GApplicationPars constructor with bad filename') try: - pars = gammalib.GApplicationPars("testme.par") - self.test_try_failure("Exception not thrown.") + pars = gammalib.GApplicationPars('testme.par') + self.test_try_failure('Exception not thrown') except: self.test_try_success() else: - self.test_try_failure("This should never happen.") + self.test_try_failure('This should never happen') # Return return -- libgit2 0.21.4