fix a double free error

And clarify docs on pointer ownership after an error.

Credit to Dimitrios Tatsis of Cisco Talos.

Author: jcupitt

CHANGELOG.md

@@ -4,6 +4,7 @@
 * improve memory usage [bgilbert]
 * fix docs build with LLVM != 14 [bgilbert]
 * improve thread safety docs [mollyclaretechcyte]
+* fix a double free error and clarify docs on pointer ownership [dtatsis]
 
 ## 1.0.5, 9/10/23
 

include/dicom/dicom.h

@@ -1026,8 +1026,11 @@ DcmDataSet *dcm_dataset_clone(DcmError **error, const DcmDataSet *dataset);
  * :param dataset: Pointer to Data Set
  * :param element: Pointer to Data Element
  *
- * The object takes over ownership of the memory referenced by `element`
- * and frees it when the object is destroyed or if the insert operation fails.
+ * On success, the dataset takes over ownership of `element`
+ * and frees it when the dataset is destroyed.
+ *
+ * If the insert operation fails, ownership does not pass and the caller is
+ * responsible for freeing `element`.
  *
  * :return: Whether insert operation was successful
  */
@@ -1198,8 +1201,11 @@ DcmSequence *dcm_sequence_create(DcmError **error);
  * :param seq: Pointer to Sequence
  * :param item: Data Set item
  *
- * The object takes over ownership of the memory referenced by `item`
- * and frees it when the object is destroyed or if the append operation fails.
+ * On success, the sequence takes over ownership of `item`
+ * and frees it when the sequence is destroyed.
+ *
+ * If the append fails, ownership does not pass and the caller is
+ * responsible for freeing `item`.
  *
  * :return: Whether append operation was successful
  */

src/dicom-data.c

@@ -1391,6 +1391,7 @@ DcmDataSet *dcm_dataset_clone(DcmError **error, const DcmDataSet *dataset)
             return NULL;
         }
         if (!dcm_dataset_insert(error, cloned_dataset, cloned_element)) {
+            dcm_element_destroy(cloned_element);
             dcm_dataset_destroy(cloned_dataset);
             return NULL;
         }
@@ -1435,7 +1436,6 @@ bool dcm_dataset_insert(DcmError **error,
                       "Element already exists",
                       "Inserting Data Element '%08x' into Data Set failed",
                       element->tag);
-        dcm_element_destroy(element);
         return false;
     }
 
@@ -1707,7 +1707,6 @@ DcmDataSet *dcm_sequence_steal(DcmError **error,
     }
 
     DcmDataSet *result = seq_item->dataset;
-    //dcm_dataset_lock(result);
     seq_item->dataset = NULL;
     // this will free the SequenceItem
     utarray_erase(seq->items, index, 1);