From 9bda96d39e7d63da97ef38a0dcb04ff9d0670a01 Mon Sep 17 00:00:00 2001 From: Dale Phurrough Date: Tue, 24 Aug 2021 20:06:36 +0200 Subject: [PATCH 1/2] add test case --- modules/core/test/test_umat.cpp | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/modules/core/test/test_umat.cpp b/modules/core/test/test_umat.cpp index c2bd6eceba..c323d17c06 100644 --- a/modules/core/test/test_umat.cpp +++ b/modules/core/test/test_umat.cpp @@ -1419,4 +1419,37 @@ TEST(UMat, resize_Mat_issue_13577) cv::ocl::setUseOpenCL(useOCL); // restore state } +TEST(UMat, exceptions_refcounts_issue_20594) +{ + if (!cv::ocl::useOpenCL()) + { + // skip test, difficult to create exception scenario without OpenCL + std::cout << "OpenCL is not enabled. Skip test" << std::endl; + return; + } + + UMat umat1(10, 10, CV_8UC1); + EXPECT_EQ(0, umat1.u->refcount); + + // cause exception in underlying allocator + void* const original_handle = umat1.u->handle; + umat1.u->handle = NULL; + try + { + Mat mat1 = umat1.getMat(ACCESS_RW); + } + catch (...) + { + // nothing + } + + // check for correct refcount, and no change of intentional bad handle + EXPECT_EQ(0, umat1.u->refcount); + EXPECT_EQ(NULL, umat1.u->handle); + + // reset UMat to good state + umat1.u->refcount = 0; + umat1.u->handle = original_handle; +} + } } // namespace opencv_test::ocl From 54a9e009703567c03d8fccbee7bb43f945b396a1 Mon Sep 17 00:00:00 2001 From: Dale Phurrough Date: Tue, 24 Aug 2021 18:56:25 +0200 Subject: [PATCH 2/2] fix opencv/opencv#20594 - exception handling with refcounts --- modules/core/src/matrix.cpp | 5 ++- modules/core/src/ocl.cpp | 26 +++++++++++++-- modules/core/src/umatrix.cpp | 63 +++++++++++++++++++++++------------- 3 files changed, 66 insertions(+), 28 deletions(-) diff --git a/modules/core/src/matrix.cpp b/modules/core/src/matrix.cpp index 4db7c7a89e..1deff6b450 100644 --- a/modules/core/src/matrix.cpp +++ b/modules/core/src/matrix.cpp @@ -749,18 +749,17 @@ Mat::Mat(const Mat& m, const Rect& roi) data += roi.x*esz; CV_Assert( 0 <= roi.x && 0 <= roi.width && roi.x + roi.width <= m.cols && 0 <= roi.y && 0 <= roi.height && roi.y + roi.height <= m.rows ); - if( u ) - CV_XADD(&u->refcount, 1); if( roi.width < m.cols || roi.height < m.rows ) flags |= SUBMATRIX_FLAG; step[0] = m.step[0]; step[1] = esz; updateContinuityFlag(); + addref(); if( rows <= 0 || cols <= 0 ) { - release(); rows = cols = 0; + release(); } } diff --git a/modules/core/src/ocl.cpp b/modules/core/src/ocl.cpp index 8b1377eec6..2d85a32366 100644 --- a/modules/core/src/ocl.cpp +++ b/modules/core/src/ocl.cpp @@ -2774,19 +2774,33 @@ struct Kernel::Impl void cleanupUMats() { + bool exceptionOccurred = false; for( int i = 0; i < MAX_ARRS; i++ ) + { if( u[i] ) { if( CV_XADD(&u[i]->urefcount, -1) == 1 ) { u[i]->flags |= UMatData::ASYNC_CLEANUP; - u[i]->currAllocator->deallocate(u[i]); + try + { + u[i]->currAllocator->deallocate(u[i]); + } + catch(const std::exception& exc) + { + // limited by legacy before C++11, therefore log and + // remember some exception occurred to throw below + CV_LOG_ERROR(NULL, "OCL: Unexpected C++ exception in OpenCL Kernel::Impl::cleanupUMats(): " << exc.what()); + exceptionOccurred = true; + } } u[i] = 0; } + } nu = 0; haveTempDstUMats = false; haveTempSrcUMats = false; + CV_Assert(!exceptionOccurred); } void addUMat(const UMat& m, bool dst) @@ -2817,8 +2831,16 @@ struct Kernel::Impl void finit(cl_event e) { CV_UNUSED(e); - cleanupUMats(); isInProgress = false; + try + { + cleanupUMats(); + } + catch(...) + { + release(); + throw; + } release(); } diff --git a/modules/core/src/umatrix.cpp b/modules/core/src/umatrix.cpp index 94f828ba60..da01f3785f 100644 --- a/modules/core/src/umatrix.cpp +++ b/modules/core/src/umatrix.cpp @@ -540,13 +540,26 @@ UMat Mat::getUMat(int accessFlags, UMatUsageFlags usageFlags) const CV_XADD(&(u->refcount), 1); CV_XADD(&(u->urefcount), 1); } - hdr.flags = flags; - setSize(hdr, dims, size.p, step.p); - finalizeHdr(hdr); - hdr.u = new_u; - hdr.offset = 0; //data - datastart; - hdr.addref(); - return hdr; + try + { + hdr.flags = flags; + setSize(hdr, dims, size.p, step.p); + finalizeHdr(hdr); + hdr.u = new_u; + hdr.offset = 0; //data - datastart; + hdr.addref(); + return hdr; + } + catch(...) + { + if (u != NULL) + { + CV_XADD(&(u->refcount), -1); + CV_XADD(&(u->urefcount), -1); + } + new_u->currAllocator->deallocate(new_u); + throw; + } } void UMat::create(int d, const int* _sizes, int _type, UMatUsageFlags _usageFlags) @@ -692,18 +705,17 @@ UMat::UMat(const UMat& m, const Rect& roi) offset += roi.x*esz; CV_Assert( 0 <= roi.x && 0 <= roi.width && roi.x + roi.width <= m.cols && 0 <= roi.y && 0 <= roi.height && roi.y + roi.height <= m.rows ); - if( u ) - CV_XADD(&(u->urefcount), 1); if( roi.width < m.cols || roi.height < m.rows ) flags |= SUBMATRIX_FLAG; step[0] = m.step[0]; step[1] = esz; updateContinuityFlag(); + addref(); if( rows <= 0 || cols <= 0 ) { - release(); rows = cols = 0; + release(); } } @@ -969,24 +981,29 @@ Mat UMat::getMat(int accessFlags) const // TODO Support ACCESS_READ (ACCESS_WRITE) without unnecessary data transfers accessFlags |= ACCESS_RW; UMatDataAutoLock autolock(u); - if(CV_XADD(&u->refcount, 1) == 0) - u->currAllocator->map(u, accessFlags); - if (u->data != 0) + try { - Mat hdr(dims, size.p, type(), u->data + offset, step.p); - hdr.flags = flags; - hdr.u = u; - hdr.datastart = u->data; - hdr.data = u->data + offset; - hdr.datalimit = hdr.dataend = u->data + u->size; - return hdr; + if(CV_XADD(&u->refcount, 1) == 0) + u->currAllocator->map(u, accessFlags); + if (u->data != 0) + { + Mat hdr(dims, size.p, type(), u->data + offset, step.p); + hdr.flags = flags; + hdr.u = u; + hdr.datastart = u->data; + hdr.data = u->data + offset; + hdr.datalimit = hdr.dataend = u->data + u->size; + return hdr; + } } - else + catch(...) { CV_XADD(&u->refcount, -1); - CV_Assert(u->data != 0 && "Error mapping of UMat to host memory."); - return Mat(); + throw; } + CV_XADD(&u->refcount, -1); + CV_Assert(u->data != 0 && "Error mapping of UMat to host memory."); + return Mat(); } void* UMat::handle(int accessFlags) const