Skip to content

Fix out of memory read when using NaN.#21111

Closed
vrabaud wants to merge 2 commits intoopencv:3.4from
vrabaud:3.4_nan
Closed

Fix out of memory read when using NaN.#21111
vrabaud wants to merge 2 commits intoopencv:3.4from
vrabaud:3.4_nan

Conversation

@vrabaud
Copy link
Copy Markdown
Contributor

@vrabaud vrabaud commented Nov 24, 2021

If a NaN is sent to RGB2Lab_f::operator() and is read at

opencv/modules/imgproc/src/color_lab.cpp

Line 2005 in 97c6ec6

float R = clip(src[bIdx]);
. It is then converted to an int when rounding at

opencv/modules/imgproc/src/color_lab.cpp

Line 2009 in 97c6ec6

int iR = cvRound(R*LAB_BASE), iG = cvRound(G*LAB_BASE), iB = cvRound(B*LAB_BASE);
which is implementation dependent but can end up being std::numeric_limits::min() on my machine.
When then calling trilinearInterpolate, the index is definitely outside the LUT at

opencv/modules/imgproc/src/color_lab.cpp

Line 1333 in 97c6ec6

const int16_t* baseLUT = &LUT[3*8*tx + (3*8*LAB_LUT_DIM)*ty + (3*8*LAB_LUT_DIM*LAB_LUT_DIM)*tz];
which is a security issue.

Yes, normal users should not use NaN, but malicious users will.

Here are the other possible solutions:

  • we first scan for any NaN and throw an error is there is any (that should be documented and replicated to the other color conversions)
  • add an Assert but that would be per pixel (a DbgAssert would not be called in production code)
  • add a default behavior like I did which is the smallest change

Please advise.

Pull Request Readiness Checklist

  • 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

alalek
alalek reviewed Nov 24, 2021
View reviewed changes
@vrabaud
Copy link
Copy Markdown
Contributor Author

vrabaud commented Nov 24, 2021

I changed to keeping NaNs as NaNs as some color conversions functions keep NaNs (e.g. RGB2BGR).

alalek
alalek reviewed Nov 24, 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.

Please add simple test for mentioned problem.
Modified code works in non-optimized mode only.
90+% of applications uses SIMD version of the code which should be checked too.

modules/imgproc/src/color_lab.cpp Outdated

for(; i < n; i += 3, src += scn)
{
if (cvIsNaN(src[bIdx]) || cvIsNaN(src[1]) || cvIsNaN(src[bIdx^2])) {
Copy link
Copy Markdown
Member

@alalek alalek Nov 24, 2021

Choose a reason for hiding this comment

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

Such explicit extra checks introduce performance degradation which we want to avoid.

Why is previous conversation closed?

Copy link
Copy Markdown
Contributor Author

@vrabaud vrabaud Nov 25, 2021

Choose a reason for hiding this comment

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

Sorry I closed because my patch was fairly different.
I reduced the NaN comparison to the bare minimum and added a test.

Copy link
Copy Markdown
Member

@alalek alalek Dec 8, 2021

Choose a reason for hiding this comment

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

What is about: cvIsNaN(src[0] + src[1] + src[2])?

Copy link
Copy Markdown
Contributor Author

@vrabaud vrabaud Dec 8, 2021

Choose a reason for hiding this comment

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

Done.
That results in less operations for SIMD.

@vrabaud vrabaud force-pushed the 3.4_nan branch 2 times, most recently from 0b7e352 to c014e5c Compare November 25, 2021 09:11
@vrabaud
Copy link
Copy Markdown
Contributor Author

vrabaud commented Dec 3, 2021

I renamed the test and added partial NaNs to it.

@vrabaud
Fix out of memory read when using NaN.
b68e463 
If a NaN is sent to RGB2Lab_f::operator(), trilinearInterpolate can
be called with arguments that are +-std::numeric_limits<int> which
triggers out of memory reads.
@vpisarev
Copy link
Copy Markdown
Contributor

vpisarev commented Dec 10, 2021

I think, it will be much faster to check the index when accessing LUT. If the LUT size is power-of-two, we can just do idx & (LUT_SIZE-1), which is super-fast and will not affect the performance

@vrabaud
Copy link
Copy Markdown
Contributor Author

vrabaud commented Dec 10, 2021 •
edited
Loading

That was indeed what this patch was doing originally: capping the coordinates to fit in the LUT table (it is not a power of two so there will still be a clamping to do).
The easiest is to change the clip function so that it returns something specific for a NaN:

opencv/modules/imgproc/src/color_lab.cpp

Line 981 in 97c6ec6

#define clip(value) \

We just have to decide on something so that results stay the same independently of the platform (something in [0,1]) as casting a NaN to an int is undefined behavior.

#define clip(value) \
    value < 0.0f ? 0.0f : value > 1.0f ? 1.0f : value >= 0 ? value : SOME_DETERMINISTIC_VALUE_LIKE_0_OR_1;

Then again, if we do so, we lose the semantic: pixel with NaNs in means pixel with NaNs out (which is fine because it is to save on performance and we just have to document it).

@alalek
Copy link
Copy Markdown
Member

alalek commented Dec 10, 2021

I will update clip() function (with 2 checks) till the end of this week (with tests as we don't guarantee NaN output in case of NaN inputs).

@vrabaud vrabaud closed this Dec 13, 2021
@vrabaud vrabaud deleted the 3.4_nan branch December 13, 2021 22:08
@alalek alalek mentioned this pull request Dec 14, 2021
Merged
vrabaud added a commit to vrabaud/opencv that referenced this pull request Dec 16, 2021
@vrabaud
Use NaN-safe clip function.
0d5e0b1 
This is to prevent more NaN to int conversions like in opencv#21111.
vrabaud added a commit to vrabaud/opencv that referenced this pull request Dec 16, 2021
@vrabaud
Use NaN-safe clip function.
3da17c4 
This is to prevent more NaN to int conversions like in opencv#21111.
@vrabaud vrabaud mentioned this pull request Dec 16, 2021
Merged
5 tasks
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: imgproc

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vrabaud @vpisarev @alalek