Skip to content

Fix potential NaN in cv::norm.#20263

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
vrabaud:3.4
Jun 15, 2021
Merged

Fix potential NaN in cv::norm.#20263
opencv-pushbot merged 1 commit intoopencv:3.4from
vrabaud:3.4

Conversation

@vrabaud
Copy link
Copy Markdown
Contributor

@vrabaud vrabaud commented Jun 11, 2021 •
edited by alalek
Loading

There can be an int overflow.
cv::norm( InputArray _src, int normType, InputArray _mask ) is fine,
not cv::norm( InputArray _src1, InputArray _src2, int normType, InputArray _mask ).
The fix and bug were both introduced in 34530da

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
force_builders=linux,docs

@vrabaud
Copy link
Copy Markdown
Contributor Author

vrabaud commented Jun 14, 2021

I can add the following test to core/tests/test_arithm.cpp but it does not fail on the 3.X branch, only on the 4.x branch:

TEST(Core_Norm, NORM_L2_8UC4)
{
    constexpr int kSide = 100;
    cv::Mat4b a(kSide, kSide, cv::Scalar(255, 255, 255, 255));
    cv::Mat4b b = cv::Mat4b::zeros(kSide, kSide);
    constexpr double kNorm = 2.*kSide*255.;
    EXPECT_EQ(kNorm, cv::norm(a, b, NORM_L2));
}

@alalek
Copy link
Copy Markdown
Member

alalek commented Jun 14, 2021

Yes, please add this test.

@vrabaud vrabaud force-pushed the 3.4 branch 4 times, most recently from cb95a0f to 36f94d6 Compare June 14, 2021 13:56
@vrabaud
Copy link
Copy Markdown
Contributor Author

vrabaud commented Jun 14, 2021

I added a test: it fails on 3.x and 4.x as the norm is a NaN.
Just make sure the line in norm.cpp is used (e.g. do not build with IPP).

@vrabaud
Fix potential NaN in cv::norm.
c8268e6 
There can be an int overflow.
cv::norm( InputArray _src, int normType, InputArray _mask ) is fine,
not cv::norm( InputArray _src1, InputArray _src2, int normType, InputArray _mask ).
alalek
alalek approved these changes Jun 15, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you for the fix and test!

Reproduced on both 3.4/master branches.

modules/core/src/norm.cpp
Comment on lines +1174 to 1176
const int intSumBlockSize = (normType == NORM_L1 && depth <= CV_8S ? (1 << 23) : (1 << 15))/cn;
const int blockSize = std::min(total, intSumBlockSize);
int isum = 0;
Copy link
Copy Markdown
Member

@alalek alalek Jun 15, 2021

Choose a reason for hiding this comment

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

(1 << 15)

Parent of the mentioned commit contains the similar code.

The change which introduces regression is:

-unsigned isum = 0;
+int isum = 0

int type is used here to follow the parameter type of used processing functions (see NormDiffFunc)

The proposed change is correct (also fixes 3.4.9 on test with kSize = 200).

Regression is introduced here by adding of block processing: https://github.com/opencv/opencv/blame/ab0f0f26a1d82e71d756a8b08f061a69214a0f10/modules/core/src/stat.cpp#L1214

@opencv-pushbot opencv-pushbot merged commit 8e0baf2 into opencv:3.4 Jun 15, 2021
@alalek alalek mentioned this pull request Jun 19, 2021
Merged
@alalek alalek mentioned this pull request Oct 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek approved these changes

Assignees

@alalek alalek

Labels

bug category: core

Projects

None yet

Milestone

3.4.15

Development

Successfully merging this pull request may close these issues.

3 participants

@vrabaud @alalek @opencv-pushbot