From 7a38b428b62f7d10fb15052ad6cbd6afd505f899 Mon Sep 17 00:00:00 2001 From: Hiroshi Horii Date: Fri, 2 Jun 2023 17:26:45 +0900 Subject: [PATCH 1/3] validate parameters of each gate in a circuit --- qiskit_aer/backends/aer_compiler.py | 61 ++-------------- .../check_param_length-eb69cd92825bbca4.yaml | 7 ++ src/framework/circuit.hpp | 1 + src/framework/operations.hpp | 72 ++++++++++++++----- .../statevector/statevector_state.hpp | 2 +- .../backends/aer_simulator/test_circuit.py | 28 +++++++- .../aer_simulator/test_standard_gates.py | 59 ++------------- 7 files changed, 104 insertions(+), 126 deletions(-) create mode 100644 releasenotes/notes/check_param_length-eb69cd92825bbca4.yaml diff --git a/qiskit_aer/backends/aer_compiler.py b/qiskit_aer/backends/aer_compiler.py index df5476f6e8..d5f8a82aed 100644 --- a/qiskit_aer/backends/aer_compiler.py +++ b/qiskit_aer/backends/aer_compiler.py @@ -636,62 +636,13 @@ def _assemble_op(aer_circ, inst, qubit_indices, clbit_indices, is_conditional, c copied = True params[i] = 0.0 + # fmt: off if name in { - "ccx", - "ccz", - "cp", - "cswap", - "csx", - "cx", - "cy", - "cz", - "delay", - "ecr", - "h", - "id", - "mcp", - "mcphase", - "mcr", - "mcrx", - "mcry", - "mcrz", - "mcswap", - "mcsx", - "mcu", - "mcu1", - "mcu2", - "mcu3", - "mcx", - "mcx_gray", - "mcy", - "mcz", - "p", - "r", - "rx", - "rxx", - "ry", - "ryy", - "rz", - "rzx", - "rzz", - "s", - "sdg", - "swap", - "sx", - "sxdg", - "t", - "tdg", - "u", - "x", - "y", - "z", - "u1", - "u2", - "u3", - "cu", - "cu1", - "cu2", - "cu3", + "ccx", "ccz", "cp", "cswap", "csx", "cx", "cy", "cz", "delay", "ecr", "h", + "id", "mcp", "mcphase", "mcr", "mcrx", "mcry", "mcrz", "mcswap", "mcsx", + "mcu", "mcu1", "mcu2", "mcu3", "mcx", "mcx_gray", "mcy", "mcz", "p", "r", + "rx", "rxx", "ry", "ryy", "rz", "rzx", "rzz", "s", "sdg", "swap", "sx", "sxdg", + "t", "tdg", "u", "x", "y", "z", "u1", "u2", "u3", "cu", "cu1", "cu2", "cu3", }: aer_circ.gate(name, qubits, params, [], conditional_reg, label if label else name) elif name == "measure": diff --git a/releasenotes/notes/check_param_length-eb69cd92825bbca4.yaml b/releasenotes/notes/check_param_length-eb69cd92825bbca4.yaml new file mode 100644 index 0000000000..41c475c67c --- /dev/null +++ b/releasenotes/notes/check_param_length-eb69cd92825bbca4.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Previously, parameters for gates are not validate in C++. If parameters are shorter than + expected (due to custom gate), segmentaion faults are thrown. This commit adds checks + whether parameter lenght is expceted. This commit will fix issues reported in #1612. + https://github.com/Qiskit/qiskit-aer/issues/1612 diff --git a/src/framework/circuit.hpp b/src/framework/circuit.hpp index b8b33c75e0..50045996db 100644 --- a/src/framework/circuit.hpp +++ b/src/framework/circuit.hpp @@ -130,6 +130,7 @@ class Circuit { const int_t cond_regidx = -1, const std::string label = "") { ops.push_back(Operations::make_gate(name, qubits, params, string_params, cond_regidx, label)); + check_gate_params(ops.back()); } void diagonal(const reg_t &qubits, const cvector_t &vec, diff --git a/src/framework/operations.hpp b/src/framework/operations.hpp index d83af2d00b..d877bdafef 100644 --- a/src/framework/operations.hpp +++ b/src/framework/operations.hpp @@ -349,23 +349,31 @@ inline void check_empty_name(const Op &op) { // Raise an exception if qubits list is empty inline void check_empty_qubits(const Op &op) { if (op.qubits.empty()) - throw std::invalid_argument(R"(Invalid qobj ")" + op.name + - R"(" instruction ("qubits" is empty).)"); + throw std::invalid_argument(R"(Invalid operation ")" + op.name + + R"(" ("qubits" is empty).)"); } // Raise an exception if params is empty inline void check_empty_params(const Op &op) { if (op.params.empty()) - throw std::invalid_argument(R"(Invalid qobj ")" + op.name + - R"(" instruction ("params" is empty).)"); + throw std::invalid_argument(R"(Invalid operation ")" + op.name + + R"(" ("params" is empty).)"); +} + +// Raise an exception if qubits is more than expected +inline void check_length_qubits(const Op &op, const size_t size) { + if (op.qubits.size() < size) + throw std::invalid_argument( + R"(Invalid operation ")" + op.name + + R"(" ("qubits" is incorrect length).)"); } // Raise an exception if params is empty inline void check_length_params(const Op &op, const size_t size) { - if (op.params.size() != size) + if (op.params.size() < size) throw std::invalid_argument( - R"(Invalid qobj ")" + op.name + - R"(" instruction ("params" is incorrect length).)"); + R"(Invalid operation ")" + op.name + + R"(" ("params" is incorrect length).)"); } // Raise an exception if qubits list contains duplications @@ -373,8 +381,42 @@ inline void check_duplicate_qubits(const Op &op) { auto cpy = op.qubits; std::unique(cpy.begin(), cpy.end()); if (cpy != op.qubits) - throw std::invalid_argument(R"(Invalid qobj ")" + op.name + - R"(" instruction ("qubits" are not unique).)"); + throw std::invalid_argument(R"(Invalid operation ")" + op.name + + R"(" ("qubits" are not unique).)"); +} + +inline void check_gate_params(const Op &op) { + const stringmap_t> param_tables( + {{"u1", {1, 1}}, {"u2", {1, 2}}, {"u3", {1, 3}}, + {"u", {1, 3}}, {"U", {1, 3}}, {"CX", {2, 0}}, + {"cx", {2, 0}}, {"cz", {2, 0}}, {"cy", {2, 0}}, + {"cp", {2, 1}}, {"cu1", {2, 1}}, {"cu2", {2, 2}}, + {"cu3", {2, 3}}, {"swap", {2, 0}}, {"id", {0, 0}}, + {"p", {1, 1}}, {"x", {1, 0}}, {"y", {1, 0}}, + {"z", {1, 0}}, {"h", {1, 0}}, {"s", {1, 0}}, + {"sdg", {1, 0}}, {"t", {1, 0}}, {"tdg", {1, 0}}, + {"r", {1, 2}}, {"rx", {1, 1}}, {"ry", {1, 1}}, + {"rz", {1, 1}}, {"rxx", {2, 1}}, {"ryy", {2, 1}}, + {"rzz", {2, 1}}, {"rzx", {2, 1}}, {"ccx", {3, 0}}, + {"cswap", {3, 0}}, {"mcx", {1, 0}}, {"mcy", {1, 0}}, + {"mcz", {1, 0}}, {"mcu1", {1, 1}}, {"mcu2", {1, 2}}, + {"mcu3", {1, 3}}, {"mcswap", {2, 0}}, {"mcphase", {1, 1}}, + {"mcr", {1, 1}}, {"mcrx", {1, 1}}, {"mcry", {1, 1}}, + {"mcrz", {1, 1}}, {"sx", {1, 0}}, {"sxdg", {1, 0}}, + {"csx", {2, 0}}, {"mcsx", {1, 0}}, {"csxdg", {2, 0}}, + {"mcsxdg", {1, 0}}, {"delay", {1, 0}}, {"pauli", {1, 0}}, + {"mcx_gray", {1, 0}}, {"cu", {2, 4}}, {"mcu", {1, 4}}, + {"mcp", {1, 1}}, {"ecr", {2, 0}}}); + + auto it = param_tables.find(op.name); + if (it == param_tables.end()) { + std::stringstream msg; + msg << "Invalid gate name :\"" << op.name << "\"." << std::endl; + throw std::invalid_argument(msg.str()); + } else { + check_length_qubits(op, std::get<0>(it->second)); + check_length_params(op, std::get<1>(it->second)); + } } //------------------------------------------------------------------------------ @@ -1155,12 +1197,8 @@ Op input_to_op_gate(const inputdata_t &input) { check_empty_name(op); check_empty_qubits(op); check_duplicate_qubits(op); - if (op.name == "u1") - check_length_params(op, 1); - else if (op.name == "u2") - check_length_params(op, 2); - else if (op.name == "u3") - check_length_params(op, 3); + check_gate_params(op); + return op; } @@ -1586,8 +1624,8 @@ Op input_to_op_save_expval(const inputdata_t &input, bool variance) { } // Check edge case of all coefficients being empty - // In this case the operator had all coefficients zero, or sufficiently close - // to zero that they were all truncated. + // In this case the operator had all coefficients zero, or sufficiently + // close to zero that they were all truncated. if (op.expval_params.empty()) { std::string pauli(op.qubits.size(), 'I'); op.expval_params.emplace_back(pauli, 0., 0.); diff --git a/src/simulators/statevector/statevector_state.hpp b/src/simulators/statevector/statevector_state.hpp index 405c142376..9a257ef08d 100644 --- a/src/simulators/statevector/statevector_state.hpp +++ b/src/simulators/statevector/statevector_state.hpp @@ -70,7 +70,7 @@ const Operations::OpSet StateOpSet( "sdg", "t", "tdg", "r", "rx", "ry", "rz", "rxx", "ryy", "rzz", "rzx", "ccx", "cswap", "mcx", "mcy", "mcz", "mcu1", "mcu2", "mcu3", "mcswap", "mcphase", - "mcr", "mcrx", "mcry", "mcry", "sx", "sxdg", "csx", + "mcr", "mcrx", "mcry", "mcrz", "sx", "sxdg", "csx", "mcsx", "csxdg", "mcsxdg", "delay", "pauli", "mcx_gray", "cu", "mcu", "mcp", "ecr"}); diff --git a/test/terra/backends/aer_simulator/test_circuit.py b/test/terra/backends/aer_simulator/test_circuit.py index 9c823e875e..6ea37c6a4a 100644 --- a/test/terra/backends/aer_simulator/test_circuit.py +++ b/test/terra/backends/aer_simulator/test_circuit.py @@ -16,7 +16,8 @@ from ddt import ddt import numpy as np from qiskit import ClassicalRegister, QuantumCircuit, QuantumRegister, assemble -from qiskit.circuit import CircuitInstruction +from qiskit.circuit.gate import Gate +from qiskit.circuit.library.standard_gates import HGate from test.terra.reference import ref_algorithms from test.terra.backends.simulator_test_case import SimulatorTestCase, supported_methods @@ -237,3 +238,28 @@ def test_floating_shots(self): shots = int(shots) self.assertSuccess(result) self.assertEqual(sum([result.get_counts()[key] for key in result.get_counts()]), shots) + + def test_invalid_parameters(self): + """Test gates with invalid parameter length.""" + + backend = self.backend() + + class Custom(Gate): + def __init__(self, label=None): + super().__init__("p", 1, [], label=label) + + def _define(self): + q = QuantumRegister(1, "q") + qc = QuantumCircuit(q, name=self.name) + qc._append(HGate(), [q[0]], []) + self.definition = qc + + qc = QuantumCircuit(1) + qc.append(Custom(), [0]) + qc.measure_all() + + try: + backend.run(qc).result() + self.fail("do not reach here") + except Exception as e: + self.assertTrue("\"params\" is incorrect length" in repr(e)) diff --git a/test/terra/backends/aer_simulator/test_standard_gates.py b/test/terra/backends/aer_simulator/test_standard_gates.py index 51e5da6914..cdf54d8b0d 100644 --- a/test/terra/backends/aer_simulator/test_standard_gates.py +++ b/test/terra/backends/aer_simulator/test_standard_gates.py @@ -20,59 +20,14 @@ from qiskit import transpile import qiskit.quantum_info as qi +# fmt: off from qiskit.circuit.library.standard_gates import ( - CXGate, - CYGate, - CZGate, - DCXGate, - HGate, - IGate, - SGate, - SXGate, - SXdgGate, - SdgGate, - SwapGate, - XGate, - YGate, - ZGate, - TGate, - TdgGate, - iSwapGate, - C3XGate, - C4XGate, - CCXGate, - CHGate, - CSXGate, - CSwapGate, - CPhaseGate, - CRXGate, - CRYGate, - CRZGate, - CUGate, - CU1Gate, - CU3Gate, - CUGate, - PhaseGate, - RC3XGate, - RCCXGate, - RGate, - RXGate, - RXXGate, - RYGate, - RYYGate, - RZGate, - RZXGate, - RZZGate, - UGate, - U1Gate, - U2Gate, - U3Gate, - UGate, - MCXGate, - MCPhaseGate, - MCXGrayCode, - ECRGate, -) + CXGate, CYGate, CZGate, DCXGate, HGate, IGate, SGate, SXGate, SXdgGate, + SdgGate, SwapGate, XGate, YGate, ZGate, TGate, TdgGate, iSwapGate, C3XGate, + C4XGate, CCXGate, CHGate, CSXGate, CSwapGate, CPhaseGate, CRXGate, CRYGate, + CRZGate, CUGate, CU1Gate, CU3Gate, CUGate, PhaseGate, RC3XGate, RCCXGate, RGate, + RXGate, RXXGate, RYGate, RYYGate, RZGate, RZXGate, RZZGate, UGate, U1Gate, U2Gate, + U3Gate, UGate, MCXGate, MCPhaseGate, MCXGrayCode, ECRGate) CLIFFORD_GATES = [ From 3e0d7523588eea3946afcec9c3203df775a3b1b8 Mon Sep 17 00:00:00 2001 From: Hiroshi Horii Date: Fri, 2 Jun 2023 22:54:57 +0900 Subject: [PATCH 2/3] fix lint error --- src/framework/operations.hpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/framework/operations.hpp b/src/framework/operations.hpp index d877bdafef..da1f575054 100644 --- a/src/framework/operations.hpp +++ b/src/framework/operations.hpp @@ -363,17 +363,15 @@ inline void check_empty_params(const Op &op) { // Raise an exception if qubits is more than expected inline void check_length_qubits(const Op &op, const size_t size) { if (op.qubits.size() < size) - throw std::invalid_argument( - R"(Invalid operation ")" + op.name + - R"(" ("qubits" is incorrect length).)"); + throw std::invalid_argument(R"(Invalid operation ")" + op.name + + R"(" ("qubits" is incorrect length).)"); } // Raise an exception if params is empty inline void check_length_params(const Op &op, const size_t size) { if (op.params.size() < size) - throw std::invalid_argument( - R"(Invalid operation ")" + op.name + - R"(" ("params" is incorrect length).)"); + throw std::invalid_argument(R"(Invalid operation ")" + op.name + + R"(" ("params" is incorrect length).)"); } // Raise an exception if qubits list contains duplications From 0483e182b5265e21c0e569965243ee297c54c153 Mon Sep 17 00:00:00 2001 From: Hiroshi Horii Date: Tue, 6 Jun 2023 01:39:04 +0900 Subject: [PATCH 3/3] fix lint error --- test/terra/backends/aer_simulator/test_circuit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/terra/backends/aer_simulator/test_circuit.py b/test/terra/backends/aer_simulator/test_circuit.py index 6ea37c6a4a..5fd1ebff72 100644 --- a/test/terra/backends/aer_simulator/test_circuit.py +++ b/test/terra/backends/aer_simulator/test_circuit.py @@ -262,4 +262,4 @@ def _define(self): backend.run(qc).result() self.fail("do not reach here") except Exception as e: - self.assertTrue("\"params\" is incorrect length" in repr(e)) + self.assertTrue('"params" is incorrect length' in repr(e))