MPS::apply_multi_qubit_gate can crash in some circumstances

[aromanro]

Informations

• Qiskit Aer version: Latest development version
• Python version: Not relevant as qiskit aer was used from C++ when noticing it
• Operating system: Windows 11

What is the current behavior?

I was testing the MPS simulator with some randomly generated circuits... with some I've got crashes.
This one I noticed after fixing #2291

The call chain was: AerState::apply_measure -> AerState::flush_ops -> Base::apply_ops -> MatrixProductState::State::apply_op -> MPS::apply_diagonal_matrix -> MPS::apply_matrix -> MPS::apply_multi_qubit_gate.

It crashed on this line:

qiskit-aer/src/simulators/matrix_product_state/matrix_product_state_internal.cpp

Line 798 in 582407a

new_mat(new_vec[row], new_vec[col]) = mat(row, col);

is_diagonal was true (as it should be by looking at the call chain), mat had 1 row and 8 columns (being a diagonal matrix, only the diagonal is given, so it's actually a vector)... but at that line it's used with mat(row, col) which is generating the crash.

Steps to reproduce the problem

Again, this might be quite tough to reproduce, I was generating some random circuits with more than 100 ops to test some things and maybe one in 100 circuits crashed.

What is the expected behavior?

No crash.

Suggested solutions

The problem lines should probably be replaced by something like:

      if (row == col)
        new_mat(new_vec[row], new_vec[row]) = is_diagonal ? mat(0, row) : mat(row, row);
      else
        new_mat(new_vec[row], new_vec[col]) = is_diagonal ? 0. : mat(row, col);

... or maybe a vector new_mat should be constructed if is_diagonal is true

[aromanro]

It seems that the code that follows expects the matrix to be in a single row format if is_diagonal is true, so probably a better fix would be:

  cmatrix_t new_mat(is_diagonal ? 1 : sidelen, sidelen);

  if (is_diagonal) {
    for (uint_t col = 0; col < sidelen; ++col) {
      new_mat(0, new_vec[col]) =
          mat.GetRows() == 1 ? mat(0, col) : mat(col, col); // this is just in case something passes a matrix
                                                            // instead of a vector even if is_diagonal is true
    }
  } else {
    for (uint_t col = 0; col < sidelen; ++col) {
      for (uint_t row = 0; row < sidelen; ++row) {
        if (row == col)
          new_mat(new_vec[row], new_vec[row]) = mat(row, row);
        else
          new_mat(new_vec[row], new_vec[col]) = mat(row, col);
      }
    }
  }

[aromanro]

if (row == col) is not necessary, so that should be removed as well.

[roytmanpiccoli]

I have look at AI and got this : if (is_diagonal) {
// Only use diagonal elements when is_diagonal is true
if (row == col) {
new_mat(new_vec[row], new_vec[col]) = mat(0, row);
} else {
new_mat(new_vec[row], new_vec[col]) = 0.0; // Off-diagonal elements are 0
}
} else {
// For non-diagonal matrices, access mat(row, col) normally
new_mat(new_vec[row], new_vec[col]) = mat(row, col);
}

[aromanro]

That would be the case if a full matrix is constructed if is_diagonal is true. Looking down on the call path it seems that other functions expect a vector if is_diagonal is true.

I'll do a pull request with the fix.

Usually moving the tests outside the for is beneficial for performance, too, but in this case it doesn't matter much as the matrices/vectors are small.