CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
Fix "use after free" issue in essential_solver.cpp
#24211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5044908
to
aa97a80
Compare
Sorry for the failing build. I cannot reproduce locally. I will investigate. |
I must have looked at the wrong build because now the build appears to be passing 🤔 |
@ivashmak Could you take a look on the solution? |
I think the new changes are valid, especially, I do skip imaginary part later in the code. |
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<int, std::__1::allocator<int> > const&, std::__1::vector<cv::Mat, std::__1::allocator<cv::Mat> >&) const /proc/self/cwd/external/com_github_opencv_opencv/modules/calib3d/src/usac/essential_solver.cpp:181:48 opencv#1 0x7fc64f915d92 in cv::usac::EssentialEstimatorImpl::estimateModels(std::__1::vector<int, std::__1::allocator<int> > const&, std::__1::vector<cv::Mat, std::__1::allocator<cv::Mat> >&) const /proc/self/cwd/external/com_github_opencv_opencv/modules/calib3d/src/usac/estimator.cpp:110:46 opencv#2 0x7fc64fa74fb0 in cv::usac::Ransac::run(cv::Ptr<cv::usac::RansacOutput>&) /proc/self/cwd/external/com_github_opencv_opencv/modules/calib3d/src/usac/ransac_solvers.cpp:152:58 opencv#3 0x7fc64fa6cd8e in cv::usac::run(cv::Ptr<cv::usac::Model const> const&, cv::_InputArray const&, cv::_InputArray const&, int, cv::Ptr<cv::usac::RansacOutput>&, 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 opencv#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 opencv#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 opencv#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<int, std::__1::allocator<int> > const&, std::__1::vector<cv::Mat, std::__1::allocator<cv::Mat> >&) 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.
aa97a80
to
c91c631
Compare
Did not result in a crash, no. At least, not that I'm aware of. I was not involved in the original discovery of this problem.
Gotcha. Good to know, thanks! I will pass that along. |
Also, I'm happy to rephrase the title. It's not actually a "use after free" in the traditional sense. There's no malloc/free involved here. It's just a use of a temporary object after it's been destroyed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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:
This shows that
eig_vecs_
points past the end ofeigensolver
. Inother 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:
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
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 becauseof the code further down (line 277 in the most recent version):
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.