Fix ORB integer overflow by AleksandrPanov · Pull Request #20823 · opencv/opencv

AleksandrPanov · 2021-10-06T18:40:52Z

Fixes opencv/opencv-python#537 "segmentation fault using ORB"

Problem in HarrisResponses(const Mat& img, const std::vector& layerinfo, std::vector& pts, int blockSize, float harris_k) from orb.cpp
An integer overflow has occurred in this line:
const uchar* ptr0 = ptr00 + (y0 - r + layerinfo[z].y)*step + x0 - r + layerinfo[z].x;
To fix this problem, integer variable step was declared as size_t.
Also I ran performance tests: PERF_TEST_P(feature2d, detectAndExtract, testing::Combine(testing::Values(DETECTORS_EXTRACTORS), TEST_IMAGES))
And the performance remained almost unchanged:
test_new_version.txt
test_old_version.txt

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

alalek

Thank you!

alalek · 2021-10-06T19:24:05Z

modules/features2d/src/orb.cpp


    const uchar* ptr00 = img.ptr<uchar>();
-    int step = (int)(img.step/img.elemSize1());
+    size_t step = img.step/img.elemSize1();


BTW, This would not help for 32-bit builds (sizeof(size_t) == 4).
We don't really want to support them (due to limited of available memory for such configuration).
So could you please to add CV_Check to catch overflow case below.

integer overflow happens here:
const uchar* ptr0 = ptr00 + (y0 - r + layerinfo[z].y)*step + x0 - r + layerinfo[z].x;

ptr00 is pointer to begin image, (y0 - r + layerinfo[z].y)*step + x0 - r + layerinfo[z].x is offset, and offset is less then image size.

Сan 32 bit systems create a image larger than max(size_t)?
// sizeof(size_t) == 4 byte, image has one CV_8U channel

If a 32-bit system cannot create the image larger than max(size_t), then the offset will not overflow.

OK, probably it would fail earlier on image creation.

alalek · 2021-10-06T19:24:09Z

modules/features2d/test/test_orb.cpp

+    applyTestTag(CV_TEST_TAG_LONG);
+    applyTestTag(CV_TEST_TAG_DEBUG_VERYLONG);
+    applyTestTag(CV_TEST_TAG_MEMORY_6GB);


Apply tags in one call.

alalek · 2021-10-06T19:26:06Z

modules/features2d/test/test_orb.cpp

+        point1 = {border + i * 100, border + i * 100};
+        point2 = {width - border - i * 100, height - border * i * 100};


C++98 doesn't support initializer list. Use ctors:

Point point1(border + i * 100, border + i * 100); Point point2(width - border - i * 100, height - border * i * 100);

Use Point instead of cv::Vec2i

alalek · 2021-10-06T19:27:51Z

modules/features2d/test/test_orb.cpp

+    Ptr<ORB> orbPtr = ORB::create(31);
+    std::vector<KeyPoint> kps;
+    Mat fv;


Declare variable where are they used (this significantly increases code readability).
Right before detectAndCompute() call.

alalek · 2021-10-07T12:07:37Z

Please eliminate Windows warnings.

AleksandrPanov · 2021-10-07T15:41:29Z

Please eliminate Windows warnings.

There is problem in this lines:

int Ix = (ptr[1] - ptr[-1])*2 + (ptr[-step+1] - ptr[-step-1]) + (ptr[step+1] - ptr[step-1]);
int Iy = (ptr[step] - ptr[-step])*2 + (ptr[step-1] - ptr[-step-1]) + (ptr[step+1] - ptr[-step+1]);

size_t_step and offset added to fix this problem:

size_t offset = (y0 - r + layerinfo[z].y)*size_t_step + (x0 - r + layerinfo[z].x);
const uchar* ptr0 = ptr00 + offset;

step was defined as int again (added CV_Check to prevent overflow).

alalek · 2021-10-07T15:48:49Z

modules/features2d/src/orb.cpp

    const uchar* ptr00 = img.ptr<uchar>();
-    int step = (int)(img.step/img.elemSize1());
+    size_t size_t_step = img.step/img.elemSize1();
+    CV_Check(size_t_step, size_t_step <= INT_MAX, "step in img > INT_MAX");


This check doesn't prevent overflow below with step+1 expressions.

BTW, UBSAN can detect these overflows, but it dislikes negative indexing.

Thx, fixed:
CV_Check(size_t_step, size_t_step + 1 <= INT_MAX, "step in img > INT_MAX");

One more step usage is in ofs calculation.

BTW, step in test case is 25072

I will add commit with checks.

…ations

alalek

Thank you 👍

AleksandrPanov added 2 commits October 6, 2021 21:20

set size_t step to fix integer overflow in ptr0 offset

7fa8137

added issue_537 test

dce5746

AleksandrPanov changed the title ~~Fix orb integer overflow~~ Fix ORB integer overflow Oct 6, 2021

AleksandrPanov added affected: 3.4 bug category: features2d labels Oct 6, 2021

alalek reviewed Oct 6, 2021

View reviewed changes

minor fix tags, points

d7fbeb8

AleksandrPanov force-pushed the fix_orb_integer_overflow branch from a7ac94d to d7fbeb8 Compare October 7, 2021 10:03

AleksandrPanov self-assigned this Oct 7, 2021

alalek reviewed Oct 7, 2021

View reviewed changes

AleksandrPanov force-pushed the fix_orb_integer_overflow branch from c2b34c0 to e79c128 Compare October 7, 2021 15:53

added size_t_step and offset to remove mixed unsigned and signed oper…

ee98a00

…ations

AleksandrPanov force-pushed the fix_orb_integer_overflow branch from e79c128 to ee98a00 Compare October 7, 2021 15:54

features2d: update ORB checks

7406eaa

alalek approved these changes Oct 7, 2021

View reviewed changes

alalek unassigned AleksandrPanov Oct 7, 2021

alalek merged commit dfc94c5 into opencv:3.4 Oct 7, 2021

alalek mentioned this pull request Oct 8, 2021

(4.x) Merge 3.4 #20844

Merged

alalek mentioned this pull request Oct 15, 2021

(5.x) Merge 4.x #20886

Merged

		point1 = {border + i * 100, border + i * 100};
		point2 = {width - border - i * 100, height - border * i * 100};

Uh oh!

Conversation

AleksandrPanov commented Oct 6, 2021 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AleksandrPanov Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalek commented Oct 7, 2021

Uh oh!

AleksandrPanov commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AleksandrPanov Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AleksandrPanov commented Oct 6, 2021 •

edited by alalek

Loading

AleksandrPanov Oct 7, 2021 •

edited

Loading

AleksandrPanov commented Oct 7, 2021 •

edited

Loading

AleksandrPanov Oct 7, 2021 •

edited

Loading