Fix: Memory leakage in QkCountOps creation. (Qiskit#14930)

* Fix: Memory leakage in `QkCountOps` creation.
The following commits attempt to fix some memoery issues that the struct `QkOpCounts`. The `qk_opcounts_free` method would segfault if an empty object was passed. The function now checks whether the passed option includes a null pointer or an empty instance. The commits also change how the `data` attribute is generated without using `std::mem::forget`, but instead collection the `OpCount` vec into a boxed_slice and consuming it into a `Box`. Inspired by what was done for Qiskit#14884.

* Docs: Add release note.

* Fix: Ironic memory leakage

* Fix c opcounts free suggestion (#7)

* Rename to _clear, free name

* rm dangling file

* use pointers

* ... forgot to register test

* fix `size_t`

* Fix: Missed some renaming

* add note on clearing opscount

* be explicit about loading Box<[OpCount]>

* fix merge artifact

* Apply suggestions from code review

Co-authored-by: Eli Arbel <[email protected]>

* goto

* happy miri happy life

* Fix merge artifacts

---------

Co-authored-by: Julien Gacon <[email protected]>
Co-authored-by: Julien Gacon <[email protected]>
Co-authored-by: Eli Arbel <[email protected]>

Author: 3 people

crates/cext/src/circuit.rs

@@ -761,16 +761,22 @@ pub unsafe extern "C" fn qk_circuit_unitary(
 /// @ingroup QkCircuit
 /// Return a list of string names for instructions in a circuit and their counts.
 ///
+/// To properly free the memory allocated by the struct, you should call ``qk_opcounts_clear``.
+/// Dropping the ``QkOpCounts`` struct without doing so will leave the stored array of ``QkOpCount``
+/// allocated and produce a memory leak.
+///
 /// @param circuit A pointer to the circuit to get the counts for.
 ///
-/// @return An ``OpCounts`` struct containing the circuit operation counts.
+/// @return An ``QkOpCounts`` struct containing the circuit operation counts.
 ///
 /// # Example
 /// ```c
 ///     QkCircuit *qc = qk_circuit_new(100, 0);
 ///     uint32_t qubits[1] = {0};
 ///     qk_circuit_gate(qc, QkGate_H, qubits, NULL);
 ///     QkOpCounts counts = qk_circuit_count_ops(qc);
+///     // .. once done
+///     qk_opcounts_clear(&counts);
 /// ```
 ///
 /// # Safety
@@ -782,16 +788,18 @@ pub unsafe extern "C" fn qk_circuit_count_ops(circuit: *const CircuitData) -> Op
     // SAFETY: Per documentation, the pointer is non-null and aligned.
     let circuit = unsafe { const_ptr_as_ref(circuit) };
     let count_ops = circuit.count_ops();
-    let mut output: Vec<OpCount> = count_ops
-        .into_iter()
-        .map(|(name, count)| OpCount {
-            name: CString::new(name).unwrap().into_raw(),
-            count,
-        })
-        .collect();
-    let data = output.as_mut_ptr();
+    let output = {
+        let vec: Vec<OpCount> = count_ops
+            .into_iter()
+            .map(|(name, count)| OpCount {
+                name: CString::new(name).unwrap().into_raw(),
+                count,
+            })
+            .collect();
+        vec.into_boxed_slice()
+    };
     let len = output.len();
-    std::mem::forget(output);
+    let data = Box::into_raw(output) as *mut OpCount;
     OpCounts { data, len }
 }
 
@@ -999,7 +1007,7 @@ pub unsafe extern "C" fn qk_circuit_instruction_clear(inst: *mut CInstruction) {
 }
 
 /// @ingroup QkCircuit
-/// Free a circuit op count list.
+/// Clear the content in a circuit operation count list.
 ///
 /// @param op_counts The returned op count list from ``qk_circuit_count_ops``.
 ///
@@ -1008,14 +1016,29 @@ pub unsafe extern "C" fn qk_circuit_instruction_clear(inst: *mut CInstruction) {
 /// Behavior is undefined if ``op_counts`` is not the object returned by ``qk_circuit_count_ops``.
 #[no_mangle]
 #[cfg(feature = "cbinding")]
-pub unsafe extern "C" fn qk_opcounts_free(op_counts: OpCounts) {
-    // SAFETY: Loading data contained in OpCounts as a slice which was constructed from a Vec
-    let data = unsafe { std::slice::from_raw_parts_mut(op_counts.data, op_counts.len) };
-    let data = data.as_mut_ptr();
-    // SAFETY: Loading a box from the slice pointer created above
-    unsafe {
-        let _ = Box::from_raw(data);
+pub unsafe extern "C" fn qk_opcounts_clear(op_counts: *mut OpCounts) {
+    // SAFETY: The user guarantees the input is a valid OpCounts pointer.
+    let op_counts = unsafe { mut_ptr_as_ref(op_counts) };
+
+    if op_counts.len > 0 && !op_counts.data.is_null() {
+        // SAFETY: We load the box from a slice pointer created from
+        // the raw parts from the OpCounts::data attribute.
+        unsafe {
+            let slice: Box<[OpCount]> = Box::from_raw(std::slice::from_raw_parts_mut(
+                op_counts.data,
+                op_counts.len,
+            ));
+            // free the allocated strings in each OpCount
+            for count in slice.iter() {
+                if !count.name.is_null() {
+                    let _ = CString::from_raw(count.name as *mut c_char);
+                }
+            }
+            // the variable vec goes out of bounds and is freed too
+        }
     }
+    op_counts.len = 0;
+    op_counts.data = std::ptr::null_mut();
 }
 
 /// @ingroup QkCircuit

releasenotes/notes/c-api-ops-count-bug-f633d349de315e89.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fixed memory leakage issues during the creation of
+    a :cpp:struct:`QkOpCounts` instance and during any calls to
+    :cpp:func:`qk_opcounts_clear` whenever an empty instance is passed.

test/c/test_circuit.c

@@ -27,8 +27,17 @@ int test_empty(void) {
     uint32_t num_qubits = qk_circuit_num_qubits(qc);
     uint32_t num_clbits = qk_circuit_num_clbits(qc);
     size_t num_instructions = qk_circuit_num_instructions(qc);
+
+    QkOpCounts counts = qk_circuit_count_ops(qc);
+    size_t opcount = counts.len;
+
+    qk_opcounts_clear(&counts);
     qk_circuit_free(qc);
 
+    if (opcount != 0) {
+        printf("The operation count %zu is not 0", opcount);
+        return EqualityError;
+    }
     if (num_qubits != 0) {
         printf("The number of qubits %d is not 0", num_qubits);
         return EqualityError;
@@ -254,6 +263,45 @@ int test_gate_num_params(void) {
     return Ok;
 }
 
+/**
+ * Test edge cases for getting the op counts.
+ */
+int test_get_gate_counts(void) {
+    QkCircuit *qc = qk_circuit_new(3, 3);
+
+    // test empty circuit
+    int result = Ok;
+    QkOpCounts c1 = qk_circuit_count_ops(qc);
+    if (c1.len != 0) {
+        result = EqualityError;
+        qk_opcounts_clear(&c1);
+        goto circuit_cleanup;
+    }
+    qk_opcounts_clear(&c1);
+
+    // add some instructions
+    uint32_t qubits[2] = {1, 0};
+    double params[1] = {0.12};
+    qk_circuit_gate(qc, QkGate_CRX, qubits, params);
+    QkOpCounts c2 = qk_circuit_count_ops(qc);
+
+    if (c2.len != 1) {
+        result = EqualityError;
+        qk_opcounts_clear(&c1);
+        goto circuit_cleanup;
+    }
+    qk_opcounts_clear(&c2);
+
+    // check that after clearing, the object is still valid
+    if (c2.len != 0 || c2.data != NULL) {
+        result = EqualityError;
+    }
+
+circuit_cleanup:
+    qk_circuit_free(qc);
+    return result;
+}
+
 int test_get_gate_counts_bv_no_measure(void) {
     QkCircuit *qc = qk_circuit_new(1000, 1000);
     double *params = NULL;
@@ -306,7 +354,7 @@ int test_get_gate_counts_bv_no_measure(void) {
     }
 cleanup:
     qk_circuit_free(qc);
-    qk_opcounts_free(op_counts);
+    qk_opcounts_clear(&op_counts);
     return result;
 }
 
@@ -373,7 +421,7 @@ int test_get_gate_counts_bv_measures(void) {
     }
 cleanup:
     qk_circuit_free(qc);
-    qk_opcounts_free(op_counts);
+    qk_opcounts_clear(&op_counts);
     return result;
 }
 
@@ -449,13 +497,12 @@ int test_get_gate_counts_bv_barrier_and_measures(void) {
         goto cleanup;
     }
     if (op_counts.data[3].count != 2) {
-        return EqualityError;
         result = EqualityError;
         goto cleanup;
     }
 cleanup:
     qk_circuit_free(qc);
-    qk_opcounts_free(op_counts);
+    qk_opcounts_clear(&op_counts);
     return result;
 }
 
@@ -678,7 +725,7 @@ int test_get_gate_counts_bv_resets_barrier_and_measures(void) {
     }
 cleanup:
     qk_circuit_free(qc);
-    qk_opcounts_free(op_counts);
+    qk_opcounts_clear(&op_counts);
     return result;
 }
 
@@ -714,10 +761,10 @@ int test_unitary_gate(void) {
     if (op_counts.len != 1 || strcmp(op_counts.data[0].name, "unitary") != 0 ||
         op_counts.data[0].count != 1) {
         result = EqualityError;
-        qk_opcounts_free(op_counts);
+        qk_opcounts_clear(&op_counts);
         goto cleanup;
     }
-    qk_opcounts_free(op_counts);
+    qk_opcounts_clear(&op_counts);
 
     QkCircuitInstruction inst;
     qk_circuit_get_instruction(qc, 0, &inst);
@@ -762,10 +809,10 @@ int test_unitary_gate_1q(void) {
     if (op_counts.len != 1 || strcmp(op_counts.data[0].name, "unitary") != 0 ||
         op_counts.data[0].count != 1) {
         result = EqualityError;
-        qk_opcounts_free(op_counts);
+        qk_opcounts_clear(&op_counts);
         goto cleanup;
     }
-    qk_opcounts_free(op_counts);
+    qk_opcounts_clear(&op_counts);
 
     QkCircuitInstruction inst;
     qk_circuit_get_instruction(qc, 0, &inst);
@@ -816,10 +863,10 @@ int test_unitary_gate_3q(void) {
     if (op_counts.len != 1 || strcmp(op_counts.data[0].name, "unitary") != 0 ||
         op_counts.data[0].count != 1) {
         result = EqualityError;
-        qk_opcounts_free(op_counts);
+        qk_opcounts_clear(&op_counts);
         goto cleanup;
     }
-    qk_opcounts_free(op_counts);
+    qk_opcounts_clear(&op_counts);
     QkCircuitInstruction inst;
     qk_circuit_get_instruction(qc, 0, &inst);
     if (strcmp(inst.name, "unitary") != 0 || inst.num_clbits != 0 || inst.num_params != 0 ||
@@ -893,6 +940,7 @@ int test_circuit(void) {
     num_failed += RUN_TEST(test_circuit_copy);
     num_failed += RUN_TEST(test_circuit_copy_with_instructions);
     num_failed += RUN_TEST(test_no_gate_1000_bits);
+    num_failed += RUN_TEST(test_get_gate_counts);
     num_failed += RUN_TEST(test_get_gate_counts_bv_no_measure);
     num_failed += RUN_TEST(test_get_gate_counts_bv_measures);
     num_failed += RUN_TEST(test_get_gate_counts_bv_barrier_and_measures);

test/c/test_elide_permutations.c

@@ -92,7 +92,7 @@ int test_elide_permutations_swap_result(void) {
     }
 
 ops_cleanup:
-    qk_opcounts_free(op_counts);
+    qk_opcounts_clear(&op_counts);
 result_cleanup:
     qk_transpile_layout_free(pass_result);
 cleanup:

test/c/test_optimize_1q_sequences.c

@@ -48,7 +48,7 @@ int inner_optimize_h_gates(QkTarget *target, char **gates, uint32_t *freq, int n
     if (!compare_gate_counts(&counts, gates, freq, num_gates)) {
         result = EqualityError;
     }
-    qk_opcounts_free(counts);
+    qk_opcounts_clear(&counts);
     qk_circuit_free(circuit);
     qk_target_free(target);
     return result;
@@ -182,7 +182,7 @@ int test_optimize_error_over_target_3(void) {
     }
 
 cleanup:
-    qk_opcounts_free(counts);
+    qk_opcounts_clear(&counts);
     qk_target_free(target);
     qk_circuit_free(circuit);
     return result;
@@ -305,4 +305,4 @@ int test_optimize_1q_sequences(void) {
     fprintf(stderr, "=== Number of failed subtests: %i\n", num_failed);
 
     return num_failed;
-}
+}

test/c/test_sabre_layout.c

@@ -121,7 +121,7 @@ int test_sabre_layout_applies_layout(void) {
     qk_circuit_free(expected_circuit);
 
 cleanup:
-    qk_opcounts_free(op_counts);
+    qk_opcounts_clear(&op_counts);
     qk_transpile_layout_free(layout_result);
     qk_circuit_free(qc);
     qk_target_free(target);

test/c/test_split_2q_unitaries.c

@@ -50,7 +50,7 @@ int test_split_2q_unitaries_no_unitaries(void) {
         }
     }
 ops_cleanup:
-    qk_opcounts_free(counts);
+    qk_opcounts_clear(&counts);
 cleanup:
     qk_circuit_free(qc);
     return result;
@@ -107,7 +107,7 @@ int test_split_2q_unitaries_x_y_unitary(void) {
     }
 
 ops_cleanup:
-    qk_opcounts_free(counts);
+    qk_opcounts_clear(&counts);
 cleanup:
     qk_circuit_free(qc);
     return result;
@@ -174,7 +174,7 @@ int test_split_2q_unitaries_swap_x_y_unitary(void) {
     }
 
 ops_cleanup:
-    qk_opcounts_free(counts);
+    qk_opcounts_clear(&counts);
 cleanup:
     qk_transpile_layout_free(split_result);
     qk_circuit_free(qc);