From c91c631ae26bc5257c67754ed6b8dc2a67f60915 Mon Sep 17 00:00:00 2001 From: Philipp Schrader Date: Thu, 31 Aug 2023 11:43:53 -0700 Subject: [PATCH] Fix "use after free" issue in `essential_solver.cpp` The address sanitizer highlighted this issue in our code base. It looks like the code is currently grabbing a pointer to a temporary object and then performing operations on it. I printed some information right before the asan crash: eigensolver address: 0x7f0ad95032f0 eigensolver size: 4528 eig_vecs_ ptr: 0x7f0ad95045e0 eig_vecs_ offset: 4848 This shows that `eig_vecs_` points past the end of `eigensolver`. In other words, it points at the temporary object created by the `eigensolver.eigenvectors()` call. Compare the docs for `.eigenvalues()`: https://eigen.tuxfamily.org/dox/classEigen_1_1EigenSolver.html#a0f507ad7ab14797882f474ca8f2773e7 to the docs for `.eigenvectors()`: https://eigen.tuxfamily.org/dox/classEigen_1_1EigenSolver.html#a66288022802172e3ee059283b26201d7 The difference in return types is interesting. `.eigenvalues()` returns a reference. But `.eigenvectors()` returns a matrix. This patch here fixes the problem by saving the temporary object and then grabbing a pointer into it. This is a curated snippet of the original asan failure: ==12==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7fc633704640 at pc 0x7fc64f7f1593 bp 0x7ffe8875fc90 sp 0x7ffe8875fc88 READ of size 8 at 0x7fc633704640 thread T0 #0 0x7fc64f7f1592 in cv::usac::EssentialMinimalSolverStewenius5ptsImpl::estimate(std::__1::vector > const&, std::__1::vector >&) const /proc/self/cwd/external/com_github_opencv_opencv/modules/calib3d/src/usac/essential_solver.cpp:181:48 #1 0x7fc64f915d92 in cv::usac::EssentialEstimatorImpl::estimateModels(std::__1::vector > const&, std::__1::vector >&) const /proc/self/cwd/external/com_github_opencv_opencv/modules/calib3d/src/usac/estimator.cpp:110:46 #2 0x7fc64fa74fb0 in cv::usac::Ransac::run(cv::Ptr&) /proc/self/cwd/external/com_github_opencv_opencv/modules/calib3d/src/usac/ransac_solvers.cpp:152:58 #3 0x7fc64fa6cd8e in cv::usac::run(cv::Ptr const&, cv::_InputArray const&, cv::_InputArray const&, int, cv::Ptr&, cv::_InputArray const&, cv::_InputArray const&, cv::_InputArray const&, cv::_InputArray const&) /proc/self/cwd/external/com_github_opencv_opencv/modules/calib3d/src/usac/ransac_solvers.cpp:1010:16 #4 0x7fc64fa6fb46 in cv::usac::findEssentialMat(cv::_InputArray const&, cv::_InputArray const&, cv::_InputArray const&, int, double, double, cv::_OutputArray const&) /proc/self/cwd/external/com_github_opencv_opencv/modules/calib3d/src/usac/ransac_solvers.cpp:527:9 #5 0x7fc64f3b5522 in cv::findEssentialMat(cv::_InputArray const&, cv::_InputArray const&, cv::_InputArray const&, int, double, double, int, cv::_OutputArray const&) /proc/self/cwd/external/com_github_opencv_opencv/modules/calib3d/src/five-point.cpp:437:16 #6 0x7fc64f3b7e00 in cv::findEssentialMat(cv::_InputArray const&, cv::_InputArray const&, cv::_InputArray const&, int, double, double, cv::_OutputArray const&) /proc/self/cwd/external/com_github_opencv_opencv/modules/calib3d/src/five-point.cpp:486:12 ... Address 0x7fc633704640 is located in stack of thread T0 at offset 17984 in frame #0 0x7fc64f7ed4ff in cv::usac::EssentialMinimalSolverStewenius5ptsImpl::estimate(std::__1::vector > const&, std::__1::vector >&) const /proc/self/cwd/external/com_github_opencv_opencv/modules/calib3d/src/usac/essential_solver.cpp:36 This frame has 63 object(s): [32, 56) 'coefficients' (line 38) [96, 384) 'ee' (line 55) ... [13040, 17568) 'eigensolver' (line 142) [17824, 17840) 'ref.tmp518' (line 143) [17856, 17872) 'ref.tmp523' (line 144) [17888, 19488) 'ref.tmp524' (line 144) <== Memory access at offset 17984 is inside this variable [19616, 19640) 'ref.tmp532' (line 169) ... The crash report says that we're accessing a temporary object from line 144 when we shouldn't be. Line 144 looks like this: https://github.com/opencv/opencv/blob/4.6.0/modules/calib3d/src/usac/essential_solver.cpp#L144 const auto * const eig_vecs_ = (double *) eigensolver.eigenvectors().real().data(); We are using version 4.6.0 for this, but the problem is present on the 4.x branch. Note that I am dropping the .real() call here. I think that is safe because of the code further down (line 277 in the most recent version): const int eig_i = 20 * i + 12; // eigen stores imaginary values too The code appears to expect to have to skip doubles for the imaginary parts of the complex numbers. Admittedly, I couldn't find a test case that exercised this code path to validate correctness. --- modules/calib3d/src/usac/essential_solver.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/calib3d/src/usac/essential_solver.cpp b/modules/calib3d/src/usac/essential_solver.cpp index 504fec6ab5..434db6d373 100644 --- a/modules/calib3d/src/usac/essential_solver.cpp +++ b/modules/calib3d/src/usac/essential_solver.cpp @@ -239,7 +239,8 @@ public: // (5) Compute the left eigenvectors of the action matrix Eigen::EigenSolver> eigensolver(action_mat_eig); const Eigen::VectorXcd &eigenvalues = eigensolver.eigenvalues(); - const auto * const eig_vecs_ = (double *) eigensolver.eigenvectors().real().data(); + const Eigen::MatrixXcd eigenvectors = eigensolver.eigenvectors(); + const auto * const eig_vecs_ = (double *) eigenvectors.data(); #else Matx A = constraint_mat.colRange(0, 10), B = constraint_mat.colRange(10, 20), eliminated_mat;