Repository navigation
Conversation
The element access of CPyWrapperMatrixView, CPyWrapperMarkerMatrixView and CPyWrapper3DMatrixView checks zero-based indices with "index > extent", so the first index past the end passes the check. For example, driver.Coordinates()(nPoint, 0) or Solution(iSolver)(0, nVar) returns data instead of raising the "out of bounds" error: - a column index equal to the number of columns reads or writes the first element of the next row (Set with a list one value too long does the same); - a row index equal to the number of rows reads or writes past the end of the buffer; - for the marker views, a vertex index equal to the number of vertices dereferences a pointer past the vertex array and crashes. Compare with ">=" in the three const Access functions. The non-const Access functions and every Get/Set/operator() of the views go through them, so this covers all entry points. Results do not change: valid indices pass the check as before, and no solver path uses these views.
|
Thanks, do you have a testcase that failed and now passes because of the fix, and that can be added to the regression tests? |
…rix views The test runs a small steady heat conduction case on a generated rectangle mesh through the Python wrapper and uses one view of each class: Solution, MarkerSolution and Gradient of the heat solver. For every view and every dimension, a child process accesses the first index past the end and must stop with the "out of bounds" error. The error ends the process (SU2_MPI::Error), so it cannot be caught in the script itself, and each access runs in its own process. After the iterations, the script sets and reads back the last valid element of each view, so the checks must also accept every valid index. If any check fails, the script names it and exits with an error. Without the previous fix, all seven accesses pass the checks: six return a value of another element or from past the end of the buffer, and the marker row past the end crashes. Only a new entry is added, the other regression values are unchanged.
|
Thanks. Not yet, because SU2_MPI::Error ends the process, so a unit test cannot catch the error. I can add a py_wrapper regression test: it runs a few iterations of a small case, then accesses each view with the first index past the end in a separate process and checks that this process stops with the "out of bounds" error. The last valid index must still return the value that was set. On develop the test fails, because these accesses return data without an error. With the fix it passes. I would add it to serial_regression.py. Is that ok for you? |
|
Sure, a pywrapper case is fine. as long as it is fast to run and proves the corrected behavior, and you can add it to serial_regression.py. |
|
Added pywrapper_matrix_view_bounds to serial_regression.py. It runs 11 iterations of a heat case on a 9×5 generated mesh, so it needs no mesh file and runs in a few seconds. On develop all 7 checks fail, and with the fix the test passes. |
|
LGTM. I verified locally that the regression passes with the fix and fails with the original bounds checks. @bigfooted Should we approve the workflows? |
This branch has not been deployed
4 participants
Proposed Changes
The Python matrix views check zero-based indices with
index > extent. The first index past the end passes this check. For example,driver.Solution(iSolver)(0, nVar)returns the first value of the next row and gives no error. A row index equal to the number of rows reads or writes past the end of the buffer. On a marker view it causes a segmentation fault.This PR changes
>to>=in the threeAccessfunctions ofCPyWrapperMatrixView.hpp. AllGet,Setandoperator()forms use these functions. Valid indices work as before. No solver path uses the views, so results do not change.I added my name to
AUTHORS.md.Related Work
No issue. I found the defect when I used the Python wrapper.
PR Checklist
pre-commit run --allto format old commits.Tests
--warnlevel=3: primal and adjoint, with and without MPI, no new warnings.develop.