We should avoid unnecessary if branching.

does src.ptr() + y * (ssize_t)src.step work with UBSan? (to avoid unsigned value)

If no then please put this code under UBSan macro guard.

does src.ptr() + y * (ssize_t)src.step work with UBSan? (to avoid unsigned value)

That would be worse actually. signed overflow is undefined. unsigned overflow (what we have now) is actually well-defined (see my commit message).

If no then please put this code under UBSan macro guard.

I will pursue such a solution then...

@opencv-alalek please see my new solution.

signed overflow is undefined

We should not have signed overflow in address arithmetic.

Actually, I've just done more reading, and in fact all pointer overflows are undefined behaviour, both signed and unsigned:

https://wdtz.org/catching-pointer-overflow-bugs.html
https://blog.regehr.org/archives/1395

Unsigned overflow is only well-defined for integers, not pointers.

Here's a simple case:

#include <cstdint>
#include <new>
#include <iostream>
#include <cassert>

int main()
{
    int* ptrStart = new int (1024);
    std::cout << "ptr start  = " << (void*)ptrStart << "\n";
    
    int* ptrEnd = ptrStart + 1024 * sizeof(int);    
    std::cout << "ptr end    = " << (void*)ptrEnd << "\n";
    
    int* ptr = ptrStart + 512; // still inside
    std::cout << "ptr offset = " << (void*)ptr << "\n";
    
    assert(ptr >= ptrStart);
    assert(ptr < ptrEnd);
    
    ptr += UINT64_MAX; // undefined
    std::cout << "ptr wrap   = " << (void*)ptr << "\n";

    assert(ptr >= ptrStart);
    assert(ptr < ptrEnd);
    
    return 0;
}

when run:

% clang++ -fsanitize=pointer-overflow test.cxx; ./a.out
ptr start  = 0x600002d00040
ptr end    = 0x600002d04040
ptr offset = 0x600002d00840
test.cxx:20:9: runtime error: addition of unsigned offset to 0x600002d00840 overflowed to 0x600002d0083c
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior test.cxx:20:9 in
ptr wrap   = 0x600002d0083c

So in fact this should not be suppressed, it should be fixed.

So I've changed this PR back to my original change (with a branch added). I've also done performance measurements and this function does not appear to be in a performance critical path, at least with my usage of it. I'm measuring the time to do:

cv::filter2D(region, imageDerivativeX, CV_64F, kernelX);
cv::filter2D(region, imageDerivativeY, CV_64F, kernelY);

With OpenCV 4.6 built as release, it takes 118 µs on average (measured on about 2000 calls). With this PR (with the branch added) it takes 104 µs on average. That's within error bars I'd guess.

I also checked with a profiler and 99% of FilterEngine__apply runtime is spent in its call to FilterEngine__proceed.

1. Use ssize_t cast to make signed arithmetic.
2. There is no ANY overflow anymore.
3. Profit.

-ptr += UINT64_MAX; // undefined
+size_t step = 4;
+int y = -1;
+ptr += y * (ssize_t)step;

Ah! I see what you mean now. I thought you were trying to argue that signed overflow is well-defined. Sorry for the misunderstanding. See my new commit.

we have tons of such code in OpenCV; are we going to refactor all of it?

Yes, we should. It's a bug. Pointer overflow is disallowed by C++.

I only use a few OpenCV classes, but I have not encountered any other such overflows in my own use. Are you sure there are so many really?

Just src.ptr() + y * (ptrdiff_t)src.step should be enough.

Done