Skip to content

CV_Assert that function exists in arithm_op#24623

Closed
tozanski wants to merge 1 commit intoopencv:4.xfrom
tozanski:tomoz/assert-arithm_op-CV_16F
Closed

CV_Assert that function exists in arithm_op#24623
tozanski wants to merge 1 commit intoopencv:4.xfrom
tozanski:tomoz/assert-arithm_op-CV_16F

Conversation

@tozanski
Copy link
Copy Markdown
Contributor

@tozanski tozanski commented Nov 30, 2023

Arithmetic like addition and subtraction on CV_16F matrices lead to SEGFAULT due to a call of NULL function, added CV_Assert to change it into an exception

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 another license that is incompatible with OpenCV
  • [ ✅ ] The PR is proposed to the proper branch
  • [ ❌ ] There is a reference to the original bug report and related work
    • no bug report found
  • [ ❌ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
    • added a regression test
  • [ ❌ ] The feature is well documented and sample code can be built with the project CMake
    • probably not documented, perhaps the expected behavior is to perform the operation instead of raising an exception

@tozanski tozanski force-pushed the tomoz/assert-arithm_op-CV_16F branch from 8186816 to ebdf7c9 Compare November 30, 2023 10:01
@tozanski tozanski force-pushed the tomoz/assert-arithm_op-CV_16F branch from ebdf7c9 to 1136d8d Compare November 30, 2023 10:03
asmorkalov
asmorkalov requested changes Nov 30, 2023
View reviewed changes
modules/core/src/arithm.cpp
Mat src1 = psrc1->getMat(), src2 = psrc2->getMat(), dst = _dst.getMat();
Size sz = getContinuousSize2D(src1, src2, dst, src1.channels());
tab[depth1](src1.ptr(), src1.step, src2.ptr(), src2.step, dst.ptr(), dst.step, sz.width, sz.height, usrdata);
BinaryFuncC func = tab[depth1];
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if depth1 is not supported, tab[depth1] leads to out-of-bound access and UB in C++. The tab size should be checked before the tab access.

Copy link
Copy Markdown
Contributor

@Kumataro Kumataro Dec 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In OpenCV5, the length of function table array is fixed as CV_DEPTH_MAX. I think this idea is good.

opencv/modules/core/src/arithm.cpp

Lines 872 to 875 in bae435a

static BinaryFuncC* getAddTab()
{
static BinaryFuncC addTab[CV_DEPTH_MAX] =
{

depth1 is calculated with CV_MAT_DEPTH(), so it is smaller than CV_DEPTH_MAX.

However in OpenCV4, this array length is shorten than CV_DEPTH_MAX. I believe it may be fixed.

opencv/modules/core/src/arithm.cpp

Lines 869 to 872 in 408730b

static BinaryFuncC* getAddTab()
{
static BinaryFuncC addTab[] =
{

This is only idea, but instead of checking depth1 value, how about to define this array length ?

@asmorkalov asmorkalov mentioned this pull request Dec 15, 2023
Closed
4 tasks
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Dec 15, 2023

Filed #24703. The PR is closed.

@asmorkalov asmorkalov closed this Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov requested changes

+1 more reviewer

@Kumataro Kumataro Kumataro left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tozanski @asmorkalov @Kumataro