core(matrix): Negative values checks#17077
Conversation
modules/core/src/matrix_wrap.cpp
Outdated
| { | ||
| const std::vector<cuda::GpuMat>& vv = *(const std::vector<cuda::GpuMat>*)obj; | ||
| if( i < 0 ) | ||
| return 1; |
There was a problem hiding this comment.
Is that correct? Why not assertion?
There was a problem hiding this comment.
It is used at several similar places, like in conditional that handles STD_ARRAY_MAT. As step (and offset) are declared with -1 to be a default value of i, my guess, based on this, is that if ( i < 0 ) return 1; is preferred over assertion to prevent mat.step() and mat.offset() calls from failing.
There was a problem hiding this comment.
maybe, it should be sizeof(cuda::GpuMat>)?
modules/core/src/matrix_wrap.cpp
Outdated
| { | ||
| const Mat* vv = (const Mat*)obj; | ||
| CV_Assert(i < sz.height); | ||
| CV_Assert(i > 0 && i < sz.height); |
There was a problem hiding this comment.
The same assertion is used also in _InputArray::isContinuous for STD_ARRAY_MAT case above. I am not sure whether > is intentional for STD_ARRAY_MAT case or it is a typo. Maybe >= should be used in both places?
|
@i386x Do have a chance to review notes? |
|
Hi @asmorkalov, I am afraid I not fully understand what notes do you have in your mind. Can you please be more specific? |
b77343e to
33cf9f6
Compare
|
@386x well done! Thanks for the contributed test. |
|
👍 |
24decb6 to
479c7b8
Compare
|
@alalek, the pull request now passes all the checks, I suggest to merge it in |
| { | ||
| const std::vector<Mat>& vv = *(const std::vector<Mat>*)obj; | ||
| CV_Assert((size_t)i < vv.size()); | ||
| CV_Assert(i >= 0 && (size_t)i < vv.size()); |
There was a problem hiding this comment.
Casting to unsigned type is a trick to handle i >= 0 check automatically.
There was a problem hiding this comment.
Oh, ok. Should I revert it than and write a note to comment?
There was a problem hiding this comment.
I believe we can keep this as is.
modules/core/src/matrix_wrap.cpp
Outdated
| { | ||
| const std::vector<UMat>& vv = *(const std::vector<UMat>*)obj; | ||
| if( i < 0 ) | ||
| return 1; |
There was a problem hiding this comment.
Why is 1 here for offset? What is logic here?
I know that this is a copy-paste, but we should not copy mistakes.
IMHO we should ban this method call with negative argument value (through CV_Assert()).
There was a problem hiding this comment.
Why is 1 here for offset? What is logic here?
I tried to find some design documents about _InputArray, but without success.
I know that this is a copy-paste, but we should not copy mistakes.
IMHO we should ban this method call with negative argument value (through
CV_Assert()).
Agree. The current behavior is a bit inconsistent (some kind of matrices raise an error and some return 1). I will fix the patch and tests.
|
@i386x Friendly reminder. |
1 similar comment
|
@i386x Friendly reminder. |
|
@i386x Do you have chance to finish the patch in meantime? |
|
@asmorkalov Sorry, I totally forgot about it. I am now waiting on build and tests to be finished on my machine and then I will submit the updated patch. |
479c7b8 to
a6936ea
Compare
Add checks that prevents indexing an array by negative values.
a6936ea to
ce31c9c
Compare
Add checks that prevents indexing an array by negative values (issues were discovered by static code analysis with coverity).
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.