Prevent unintentional deallocation of arguments (#2340)

Jürgen Knödlseder
1 parent 4edf58aa
Showing 15 changed files with 114 additions and 51 deletions
ChangeLog
NEWS
doc/source/admin/release_history/1.5.1.rst
inst/cta/src/GCTAObservation.cpp
inst/cta/src/GCTAOnOffObservation.cpp
src/fits/GFits.cpp
src/fits/GFitsTable.cpp
src/model/GModelSky.cpp
src/model/GModels.cpp
src/obs/GObservation.cpp
src/opt/GOptimizerPars.cpp
src/sky/GSkyMap.cpp
src/sky/GSkyRegions.cpp
src/test/GTestSuites.cpp
src/xml/GXmlNode.cpp
-2018-02-13
+2018-02-15
         * Bug fix version 1.5.1 released
         ================================
+        Prevent unintentional deallocation of arguments (#2340).
         Correct sky map smoothing.
 New Features and Important Changes in GammaLib 1.5.1
-13 February 2018
+15 February 2018
 1. Introduction
@@ -53,7 +53,10 @@ None
 6. FITS I/O Interface
 ---------------------
-None
+The GFits::set() and GFitsTable::set() methods now only deallocate the previous
+content if the reference to the argument differs from the reference of the
+previous content. Otherwise the method would deallocate an instance that is
+owned by the client (#2340).
 7. Linear algebra module
@@ -63,7 +66,10 @@ None
 8. Model module
 ---------------
-None
+The GModels::set() and GModelSky::spatial(), spectral() and temporal() methods
+now only deallocate the previous content if the reference to the argument
+differs from the reference of the previous content. Otherwise the method
+would deallocate an instance that is owned by the client (#2340).
 9. Numerics module
@@ -73,16 +79,27 @@ None
 10. Observation module
 ----------------------
-None
+The GObservation::events() method now only deallocate the previous content
+if the reference to the argument differs from the reference of the previous
+content. Otherwise the method would deallocate an instance that is owned by
+the client (#2340).
 11. Optimization module
 -----------------------
-None
+The GOptimizerPars::set() method now only deallocate the previous content 
+if the reference to the argument differs from the reference of the previous 
+content. Otherwise the method would deallocate an instance that is owned by 
+the client (#2340).
 12. Sky module
 --------------
+The GSkyMap::projection() and GSkyRegions::set() methods now only deallocate
+the previous content if the reference to the argument differs from the
+reference of the previous content. Otherwise the method would deallocate an
+instance that is owned by the client (#2340).
+
 The smoothing kernel covered before only two sectors, now all four sectors
 are covered. The smoothing now also works correctly in case that more than
 a single sky map is comprised in a GSkyMap object.
@@ -95,7 +112,10 @@ None
 14. XML module
 --------------
-None
+The GXmlNode::set() method now only deallocate the previous content if the
+reference to the argument differs from the reference of the previous content. 
+Otherwise the method would deallocate an instance that is owned by the client 
+(#2340).
 15. VO module
@@ -110,7 +130,10 @@ None
 17. Test module
 ---------------
-None
+The GTestSuite::set() method now only deallocate the previous content if the
+reference to the argument differs from the reference of the previous content.
+Otherwise the method would deallocate an instance that is owned by the client
+(#2340).
 18. Multi-wavelength interface
@@ -125,7 +148,10 @@ None
 20. CTA interface
 -----------------
-None
+The GCTAObservation::response() and GCTAOnOffObservation::response() methods
+now only deallocate the previous content if the reference to the argument
+differs from the reference of the previous content. Otherwise the method would
+deallocate an instance that is owned by the client (#2340).
 21. COMPTEL interface
@@ -10,6 +10,8 @@ GammaLib 1.5.1 is a bug fix release for GammaLib release 1.5.
 The following bugs were fixed:
+* [`2340 <https://cta-redmine.irap.omp.eu/issues/2340>`_] -
+  Prevent unintentional deallocation of arguments
 * Correct sky map smoothing
@@ -335,8 +335,11 @@ void GCTAObservation::response(const GResponse&amp; rsp)
         throw GException::invalid_argument(G_RESPONSE_SET, msg);
     }
-    // Free response
-    if (m_response != NULL) delete m_response;
+    // Free existing response only if it differs from current response. This
+    // prevents unintential deallocation of the argument
+    if ((m_response != NULL) && (m_response != &rsp)) {
+        delete m_response;
+    }
     // Clone response function
     m_response = ptr->clone();
@@ -575,8 +575,11 @@ void GCTAOnOffObservation::response(const GResponse&amp; rsp)
         throw GException::invalid_argument(G_RESPONSE_SET, msg);
     }
-    // Free response
-    if (m_response != NULL) delete m_response;
+    // Free existing response only if it differs from current response. This
+    // prevents unintential deallocation of the argument
+    if ((m_response != NULL) && (m_response != &rsp)) {
+        delete m_response;
+    }
     // Clone response function
     m_response = ptr->clone();
 /***************************************************************************
  *                    GFits.cpp - FITS file access class                   *
  * ----------------------------------------------------------------------- *
- *  copyright (C) 2008-2017 by Juergen Knoedlseder                         *
+ *  copyright (C) 2008-2018 by Juergen Knoedlseder                         *
  * ----------------------------------------------------------------------- *
  *                                                                         *
  *  This program is free software: you can redistribute it and/or modify   *
@@ -601,8 +601,11 @@ GFitsHDU* GFits::set(const int&amp; extno, const GFitsHDU&amp; hdu)
         throw GException::invalid_argument(G_SET1, msg);
     }
-    // Delete any existing HDU
-    if (m_hdu[extno] != NULL) delete m_hdu[extno];
+    // Free existing HDU only if it differs from current HDU. This prevents
+    // unintential deallocation of the argument
+    if ((m_hdu[extno] != NULL) && (m_hdu[extno] != &hdu)) {
+        delete m_hdu[extno];
+    }
     // Assign new HDU by cloning
     m_hdu[extno] = hdu.clone();
 /***************************************************************************
  *                  GFitsTable.cpp - FITS table base class                 *
  * ----------------------------------------------------------------------- *
- *  copyright (C) 2008-2017 by Juergen Knoedlseder                         *
+ *  copyright (C) 2008-2018 by Juergen Knoedlseder                         *
  * ----------------------------------------------------------------------- *
  *                                                                         *
  *  This program is free software: you can redistribute it and/or modify   *
@@ -401,12 +401,6 @@ GFitsTableCol* GFitsTable::set(const std::string&amp;   colname,
  ***************************************************************************/
 GFitsTableCol* GFitsTable::insert(int colnum, const GFitsTableCol& column)
 {
-    // Make sure that 'colnum' is valid. Since we add one column the total
-    // number of columns at return will be m_cols+1, hence the column
-    // index is comprised between [0,m_cols]
-    //if (colnum <      0) colnum = 0;
-    //if (colnum > m_cols) colnum = m_cols;
-
     // Check if column number is valid
     if (colnum < 0 || colnum > ncols()) {
         throw GException::out_of_range(G_INSERT1, "Column number", colnum, ncols()+1);
 /***************************************************************************
  *                    GModelSky.cpp - Sky model class                      *
  * ----------------------------------------------------------------------- *
- *  copyright (C) 2011-2017 by Juergen Knoedlseder                         *
+ *  copyright (C) 2011-2018 by Juergen Knoedlseder                         *
  * ----------------------------------------------------------------------- *
  *                                                                         *
  *  This program is free software: you can redistribute it and/or modify   *
@@ -411,8 +411,11 @@ GModelSky* GModelSky::clone(void) const
  ***************************************************************************/
 void GModelSky::spatial(const GModelSpatial* spatial)
 {
-    // Free spatial model component
-    if (m_spatial  != NULL) delete m_spatial;
+    // Free spatial model component only if it differs from current
+    // component. This prevents unintential deallocation of the argument
+    if ((m_spatial != NULL) && (m_spatial != spatial)) {
+        delete m_spatial;
+    }
     // Clone spatial model component if it exists, otherwise set pointer
     // to NULL
@@ -438,8 +441,11 @@ void GModelSky::spatial(const GModelSpatial* spatial)
  ***************************************************************************/
 void GModelSky::spectral(const GModelSpectral* spectral)
 {
-    // Free spectral model component
-    if (m_spectral  != NULL) delete m_spectral;
+    // Free spectral model component only if it differs from current
+    // component. This prevents unintential deallocation of the argument
+    if ((m_spectral != NULL) && (m_spectral != spectral)) {
+        delete m_spectral;
+    }
     // Clone spectral model component if it exists, otherwise set pointer
     // to NULL
@@ -462,8 +468,11 @@ void GModelSky::spectral(const GModelSpectral* spectral)
  ***************************************************************************/
 void GModelSky::temporal(const GModelTemporal* temporal)
 {
-    // Free temporal model component
-    if (m_temporal  != NULL) delete m_temporal;
+    // Free temporal model component only if it differs from current
+    // component. This prevents unintential deallocation of the argument
+    if ((m_temporal != NULL) && (m_temporal != temporal)) {
+        delete m_temporal;
+    }
     // Clone temporal model component if it exists, otherwise set pointer
     // to NULL
 /***************************************************************************
  *                   GModels.cpp - Model container class                   *
  * ----------------------------------------------------------------------- *
- *  copyright (C) 2009-2017 by Juergen Knoedlseder                         *
+ *  copyright (C) 2009-2018 by Juergen Knoedlseder                         *
  * ----------------------------------------------------------------------- *
  *                                                                         *
  *  This program is free software: you can redistribute it and/or modify   *
@@ -332,8 +332,11 @@ GModel* GModels::set(const int&amp; index, const GModel&amp; model)
         throw GException::invalid_value(G_SET1, msg);
     }
-    // Delete any existing model
-    if (m_models[index] != NULL) delete m_models[index];
+    // Free existing model only if it differs from current model. This
+    // prevents unintential deallocation of the argument
+    if ((m_models[index] != NULL) && (m_models[index] != &model)) {
+        delete m_models[index];
+    }
     // Assign new model by cloning
     m_models[index] = model.clone();
@@ -379,8 +382,11 @@ GModel* GModels::set(const std::string&amp; name, const GModel&amp; model)
         throw GException::invalid_value(G_SET2, msg);
     }
-    // Delete any existing model
-    if (m_models[index] != NULL) delete m_models[index];
+    // Free existing model only if it differs from current model. This
+    // prevents unintential deallocation of the argument
+    if ((m_models[index] != NULL) && (m_models[index] != &model)) {
+        delete m_models[index];
+    }
     // Assign new model by cloning
     m_models[index] = model.clone();
 /***************************************************************************
  *            GObservation.cpp - Abstract observation base class           *
  * ----------------------------------------------------------------------- *
- *  copyright (C) 2008-2017 by Juergen Knoedlseder                         *
+ *  copyright (C) 2008-2018 by Juergen Knoedlseder                         *
  * ----------------------------------------------------------------------- *
  *                                                                         *
  *  This program is free software: you can redistribute it and/or modify   *
@@ -769,8 +769,11 @@ double GObservation::npred_grad(const GModel&amp; model, const GModelPar&amp; par) const
  ***************************************************************************/
 void GObservation::events(const GEvents& events)
 {
-    // Remove an existing event container
-    if (m_events != NULL) delete m_events;
+    // Free existing event container only if it differs from current event
+    // container. This prevents unintential deallocation of the argument
+    if ((m_events != NULL) && (m_events != &events)) {
+        delete m_events;
+    }
     // Clone events
     m_events = events.clone();
 /***************************************************************************
  *         GOptimizerPars.cpp - Optimizer parameter container class        *
  * ----------------------------------------------------------------------- *
- *  copyright (C) 2009-2017 by Juergen Knoedlseder                         *
+ *  copyright (C) 2009-2018 by Juergen Knoedlseder                         *
  * ----------------------------------------------------------------------- *
  *                                                                         *
  *  This program is free software: you can redistribute it and/or modify   *
@@ -346,8 +346,9 @@ GOptimizerPar* GOptimizerPars::set(const int&amp; index, const GOptimizerPar&amp; par)
     }
     #endif
-    // Delete parameter if it has been allocated
-    if (m_alloc[index]) {
+    // Free existing parameter only if it differs from current parameter.
+    // This prevents unintential deallocation of the argument
+    if ((m_alloc[index]) && (m_pars[index] != &par)) {
         delete m_pars[index];
     }
@@ -1744,8 +1744,11 @@ double GSkyMap::solidangle(const GSkyPixel&amp; pixel) const
  ***************************************************************************/
 void GSkyMap::projection(const GSkyProjection& proj)
 {
-    // Free any existing WCS
-    if (m_proj != NULL) delete m_proj;
+    // Free existing projection only if it differs from current projection.
+    // This prevents unintential deallocation of the argument
+    if ((m_proj != NULL) && (m_proj != &proj)) {
+        delete m_proj;
+    }
     // Clone input WCS
     m_proj = proj.clone();
@@ -264,8 +264,11 @@ GSkyRegion* GSkyRegions::set(const int&amp; index, const GSkyRegion&amp; region)
     }
     #endif
-    // Delete any existing region
-    if (m_regions[index] != NULL) delete m_regions[index];
+    // Free existing region only if it differs from current region. This
+    // prevents unintential deallocation of the argument
+    if ((m_regions[index] != NULL) && (m_regions[index] != &region)) {
+        delete m_regions[index];
+    }
     // Assign new region by cloning
     m_regions[index] = region.clone();
 /***************************************************************************
  *              GTestSuites.cpp - Test suite container class               *
  * ----------------------------------------------------------------------- *
- *  copyright (C) 2012-2016 by Jean-Baptiste Cayrou                        *
+ *  copyright (C) 2012-2018 by Jean-Baptiste Cayrou                        *
  * ----------------------------------------------------------------------- *
  *                                                                         *
  *  This program is free software: you can redistribute it and/or modify   *
@@ -248,8 +248,11 @@ GTestSuite* GTestSuites::set(const int&amp; index, const GTestSuite&amp; suite)
     }
     #endif
-    // Delete any existing test suite
-    if (m_testsuites[index] != NULL) delete m_testsuites[index];
+    // Free existing test suite only if it differs from current test suite.
+    // This prevents unintential deallocation of the argument
+    if ((m_testsuites[index] != NULL) && (m_testsuites[index] != &suite)) {
+        delete m_testsuites[index];
+    }
     // Assign new test suite by cloning
     m_testsuites[index] = suite.clone();
 /***************************************************************************
  *                GXmlNode.cpp - Abstract XML node base class              *
  * ----------------------------------------------------------------------- *
- *  copyright (C) 2010-2016 by Juergen Knoedlseder                         *
+ *  copyright (C) 2010-2018 by Juergen Knoedlseder                         *
  * ----------------------------------------------------------------------- *
  *                                                                         *
  *  This program is free software: you can redistribute it and/or modify   *
@@ -253,8 +253,11 @@ GXmlNode* GXmlNode::set(const int&amp; index, const GXmlNode&amp; node)
     }
     #endif
-    // Delete any existing node
-    if (m_nodes[index] != NULL) delete m_nodes[index];
+    // Free existing node only if it differs from current node. This
+    // prevents unintential deallocation of the argument
+    if ((m_nodes[index] != NULL) && (m_nodes[index] != &node)) {
+        delete m_nodes[index];
+    }
     // Assign new child node by cloning
     m_nodes[index] = node.clone();