Skip to content

Commit 8316021

Browse files
HoppeMateuszHoppeMateusz
committed
Fix problems in thkWrapper and SharingHandler
- ThkWrapper had uninitialized mFunc member, setting it to nullptr - D3DSurface could dereference null image pointer, adding validateUpdateData method in SharingHandler that may return CL_INVALID_MEM_OBJECT if memObject is invalid Change-Id: Iaa4499bcea47baca156c9d28be4c93ba4f0e1ebb
1 parent 75d497a commit 8316021

File tree

12 files changed

+117
-28
lines changed

12 files changed

+117
-28
lines changed

‎runtime/command_queue/command_queue.cpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,10 @@ cl_int CommandQueue::enqueueAcquireSharedObjects(cl_uint numObjects, const cl_me
313313
return CL_INVALID_MEM_OBJECT;
314314
}
315315

316-
memObject->peekSharingHandler()->acquire(memObject);
316+
int result = memObject->peekSharingHandler()->acquire(memObject);
317+
if (result != CL_SUCCESS) {
318+
return result;
319+
}
317320
memObject->acquireCount++;
318321
}
319322
auto status = enqueueMarkerWithWaitList(

‎runtime/os_interface/windows/thk_wrapper.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2017, Intel Corporation
2+
* Copyright (c) 2017 - 2018, Intel Corporation
33
*
44
* Permission is hereby granted, free of charge, to any person obtaining a
55
* copy of this software and associated documentation files (the "Software"),
@@ -85,7 +85,7 @@ class ThkWrapper {
8585
typedef NTSTATUS(APIENTRY *Func)(Param);
8686

8787
public:
88-
Func mFunc;
88+
Func mFunc = nullptr;
8989

9090
inline NTSTATUS operator()(Param param) const {
9191
if (UseTimer) {

‎runtime/sharings/d3d/d3d_sharing.cpp‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2017, Intel Corporation
2+
* Copyright (c) 2017 - 2018, Intel Corporation
33
*
44
* Permission is hereby granted, free of charge, to any person obtaining a
55
* copy of this software and associated documentation files (the "Software"),
@@ -35,19 +35,23 @@ template <typename D3D>
3535
D3DSharing<D3D>::D3DSharing(Context *context, D3DResource *resource, D3DResource *resourceStaging, unsigned int subresource, bool sharedResource)
3636
: sharedResource(sharedResource), subresource(subresource), resource(resource), resourceStaging(resourceStaging), context(context) {
3737
sharingFunctions = context->getSharing<D3DSharingFunctions<D3D>>();
38-
sharingFunctions->addRef(resource);
39-
sharingFunctions->createQuery(&this->d3dQuery);
40-
sharingFunctions->track(resource, subresource);
38+
if (sharingFunctions) {
39+
sharingFunctions->addRef(resource);
40+
sharingFunctions->createQuery(&this->d3dQuery);
41+
sharingFunctions->track(resource, subresource);
42+
}
4143
};
4244

4345
template <typename D3D>
4446
D3DSharing<D3D>::~D3DSharing() {
45-
sharingFunctions->untrack(resource, subresource);
46-
if (!sharedResource) {
47-
sharingFunctions->release(resourceStaging);
47+
if (sharingFunctions) {
48+
sharingFunctions->untrack(resource, subresource);
49+
if (!sharedResource) {
50+
sharingFunctions->release(resourceStaging);
51+
}
52+
sharingFunctions->release(resource);
53+
sharingFunctions->release(d3dQuery);
4854
}
49-
sharingFunctions->release(resource);
50-
sharingFunctions->release(d3dQuery);
5155
};
5256

5357
template <typename D3D>

‎runtime/sharings/d3d/d3d_surface.cpp‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,9 @@ D3DSurface::D3DSurface(Context *context, cl_dx9_surface_info_khr *surfaceInfo, D
3636
: D3DSharing(context, surfaceInfo->resource, surfaceStaging, plane, sharedResource), adapterType(adapterType),
3737
surfaceInfo(*surfaceInfo), lockable(lockable), plane(plane), oclPlane(oclPlane), d3d9Surface(surfaceInfo->resource),
3838
d3d9SurfaceStaging(surfaceStaging) {
39-
resourceDevice = sharingFunctions->getDevice();
39+
if (sharingFunctions) {
40+
resourceDevice = sharingFunctions->getDevice();
41+
}
4042
};
4143

4244
Image *D3DSurface::create(Context *context, cl_dx9_surface_info_khr *surfaceInfo, cl_mem_flags flags,
@@ -125,7 +127,7 @@ void D3DSurface::synchronizeObject(UpdateData *updateData) {
125127
sharingFunctions->lockRect(d3d9SurfaceStaging, &lockedRect, D3DLOCK_READONLY);
126128
}
127129

128-
auto image = castToObject<Image>(updateData->memObject);
130+
auto image = castToObjectOrAbort<Image>(updateData->memObject);
129131
auto sys = lockedRect.pBits;
130132
auto gpu = context->getMemoryManager()->lockResource(image->getGraphicsAllocation());
131133
auto pitch = static_cast<ULONG>(lockedRect.Pitch);
@@ -148,6 +150,11 @@ void D3DSurface::synchronizeObject(UpdateData *updateData) {
148150

149151
void D3DSurface::releaseResource(MemObj *memObject) {
150152
D3DLOCKED_RECT lockedRect = {};
153+
auto image = castToObject<Image>(memObject);
154+
if (!image) {
155+
return;
156+
}
157+
151158
sharingFunctions->setDevice(resourceDevice);
152159
if (!sharedResource) {
153160
if (lockable) {
@@ -156,7 +163,6 @@ void D3DSurface::releaseResource(MemObj *memObject) {
156163
sharingFunctions->lockRect(d3d9SurfaceStaging, &lockedRect, 0);
157164
}
158165

159-
auto image = castToObject<Image>(memObject);
160166
auto sys = lockedRect.pBits;
161167
auto gpu = context->getMemoryManager()->lockResource(image->getGraphicsAllocation());
162168
auto pitch = static_cast<ULONG>(lockedRect.Pitch);
@@ -306,3 +312,12 @@ cl_int D3DSurface::findImgFormat(D3DFORMAT d3dFormat, cl_image_format &imgFormat
306312
}
307313
return CL_SUCCESS;
308314
}
315+
316+
int D3DSurface::validateUpdateData(UpdateData *updateData) {
317+
UNRECOVERABLE_IF(updateData == nullptr);
318+
auto image = castToObject<Image>(updateData->memObject);
319+
if (!image) {
320+
return CL_INVALID_MEM_OBJECT;
321+
}
322+
return CL_SUCCESS;
323+
}

‎runtime/sharings/d3d/d3d_surface.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2017, Intel Corporation
2+
* Copyright (c) 2017 - 2018, Intel Corporation
33
*
44
* Permission is hereby granted, free of charge, to any person obtaining a
55
* copy of this software and associated documentation files (the "Software"),
@@ -41,6 +41,7 @@ class D3DSurface : public D3DSharing<D3DTypesHelper::D3D9> {
4141
static cl_int findImgFormat(D3DFORMAT d3dFormat, cl_image_format &imgFormat, cl_uint plane, OCLPlane &oclPlane);
4242
void synchronizeObject(UpdateData *updateData) override;
4343
void releaseResource(MemObj *memObject) override;
44+
int validateUpdateData(UpdateData *updateData) override;
4445

4546
cl_dx9_surface_info_khr &getSurfaceInfo() { return surfaceInfo; }
4647
cl_dx9_media_adapter_type_khr &getAdapterType() { return adapterType; }

‎runtime/sharings/sharing.cpp‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2017, Intel Corporation
2+
* Copyright (c) 2017 - 2018, Intel Corporation
33
*
44
* Permission is hereby granted, free of charge, to any person obtaining a
55
* copy of this software and associated documentation files (the "Software"),
@@ -27,21 +27,34 @@
2727
#include <memory>
2828

2929
namespace OCLRT {
30-
void SharingHandler::acquire(MemObj *memObj) {
30+
int SharingHandler::acquire(MemObj *memObj) {
3131
if (acquireCount == 0) {
3232
UpdateData updateData;
3333
auto currentSharedHandle = memObj->getGraphicsAllocation()->peekSharedHandle();
3434
updateData.sharedHandle = currentSharedHandle;
3535
updateData.memObject = memObj;
36-
synchronizeHandler(&updateData);
36+
int result = synchronizeHandler(&updateData);
37+
if (result != CL_SUCCESS) {
38+
return result;
39+
}
3740
DEBUG_BREAK_IF(updateData.synchronizationStatus != SynchronizeStatus::ACQUIRE_SUCCESFUL);
3841
DEBUG_BREAK_IF(currentSharedHandle != updateData.sharedHandle);
3942
}
4043
acquireCount++;
44+
return CL_SUCCESS;
4145
}
4246

43-
void SharingHandler::synchronizeHandler(UpdateData *updateData) {
44-
synchronizeObject(updateData);
47+
int SharingHandler::synchronizeHandler(UpdateData *updateData) {
48+
auto result = validateUpdateData(updateData);
49+
if (result == CL_SUCCESS) {
50+
synchronizeObject(updateData);
51+
}
52+
return result;
53+
}
54+
55+
int SharingHandler::validateUpdateData(UpdateData *updateData) {
56+
UNRECOVERABLE_IF(updateData == nullptr);
57+
return CL_SUCCESS;
4558
}
4659

4760
void SharingHandler::release(MemObj *memObject) {

‎runtime/sharings/sharing.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2017, Intel Corporation
2+
* Copyright (c) 2017 - 2018, Intel Corporation
33
*
44
* Permission is hereby granted, free of charge, to any person obtaining a
55
* copy of this software and associated documentation files (the "Software"),
@@ -50,15 +50,16 @@ class SharingFunctions {
5050

5151
class SharingHandler {
5252
public:
53-
void acquire(MemObj *memObj);
53+
int acquire(MemObj *memObj);
5454
void release(MemObj *memObject);
5555
virtual ~SharingHandler() = default;
5656

5757
virtual void getMemObjectInfo(size_t &paramValueSize, void *&paramValue){};
5858
virtual void releaseReusedGraphicsAllocation(){};
5959

6060
protected:
61-
virtual void synchronizeHandler(UpdateData *updateData);
61+
virtual int synchronizeHandler(UpdateData *updateData);
62+
virtual int validateUpdateData(UpdateData *updateData);
6263
virtual void synchronizeObject(UpdateData *updateData) { updateData->synchronizationStatus = SYNCHRONIZE_ERROR; }
6364
virtual void releaseResource(MemObj *memObject){};
6465
unsigned int acquireCount = 0u;

‎unit_tests/command_queue/command_queue_tests.cpp‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -933,6 +933,24 @@ TEST(CommandQueue, givenEnqueueReleaseSharedObjectsWhenIncorrectArgumentsThenRet
933933
EXPECT_EQ(result, CL_INVALID_MEM_OBJECT);
934934
}
935935

936+
TEST(CommandQueue, givenEnqueueAcquireSharedObjectsCallWhenAcquireFailsThenCorrectErrorIsReturned) {
937+
class MockSharingHandler : public SharingHandler {
938+
int validateUpdateData(UpdateData *data) override {
939+
return CL_INVALID_MEM_OBJECT;
940+
}
941+
};
942+
MockContext context;
943+
CommandQueue cmdQ(&context, nullptr, 0);
944+
auto buffer = std::unique_ptr<Buffer>(BufferHelper<>::create(&context));
945+
946+
MockSharingHandler *handler = new MockSharingHandler;
947+
buffer->setSharingHandler(handler);
948+
cl_mem memObject = buffer.get();
949+
auto retVal = cmdQ.enqueueAcquireSharedObjects(1, &memObject, 0, nullptr, nullptr, 0);
950+
EXPECT_EQ(CL_INVALID_MEM_OBJECT, retVal);
951+
buffer->setSharingHandler(nullptr);
952+
}
953+
936954
HWTEST_F(CommandQueueCommandStreamTest, givenDebugKernelWhenSetupDebugSurfaceIsCalledThenSurfaceStateIsCorrectlySet) {
937955
using RENDER_SURFACE_STATE = typename FamilyType::RENDER_SURFACE_STATE;
938956
MockProgram program;

‎unit_tests/d3d_sharing/d3d9_tests.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2017, Intel Corporation
2+
* Copyright (c) 2017 - 2018, Intel Corporation
33
*
44
* Permission is hereby granted, free of charge, to any person obtaining a
55
* copy of this software and associated documentation files (the "Software"),

‎unit_tests/d3d_sharing/d3d_tests.cpp‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2017, Intel Corporation
2+
* Copyright (c) 2017 - 2018, Intel Corporation
33
*
44
* Permission is hereby granted, free of charge, to any person obtaining a
55
* copy of this software and associated documentation files (the "Software"),
@@ -28,10 +28,12 @@
2828
#include "runtime/sharings/d3d/cl_d3d_api.h"
2929
#include "runtime/sharings/d3d/d3d_sharing.h"
3030
#include "runtime/sharings/d3d/d3d_buffer.h"
31+
#include "runtime/sharings/d3d/d3d_surface.h"
3132
#include "runtime/sharings/d3d/d3d_texture.h"
3233
#include "gtest/gtest.h"
3334
#include "gmock/gmock.h"
3435
#include "test.h"
36+
#include "unit_tests/mocks/mock_buffer.h"
3537
#include "unit_tests/mocks/mock_context.h"
3638
#include "unit_tests/mocks/mock_command_queue.h"
3739
#include "unit_tests/fixtures/platform_fixture.h"
@@ -1385,4 +1387,22 @@ REGISTER_TYPED_TEST_CASE_P(D3DAuxTests,
13851387

13861388
INSTANTIATE_TYPED_TEST_CASE_P(D3DSharingTests, D3DAuxTests, D3DTypes);
13871389

1390+
TEST(D3DSurfaceTest, givenD3DSurfaceWhenInvalidMemObjectIsPassedToValidateUpdateDataThenInvalidMemObjectErrorIsReturned) {
1391+
class MockD3DSurface : public D3DSurface {
1392+
public:
1393+
MockD3DSurface(Context *context, cl_dx9_surface_info_khr *surfaceInfo, D3DTypesHelper::D3D9::D3DTexture2d *surfaceStaging, cl_uint plane,
1394+
OCLPlane oclPlane, cl_dx9_media_adapter_type_khr adapterType, bool sharedResource, bool lockable) : D3DSurface(context, surfaceInfo, surfaceStaging, plane,
1395+
oclPlane, adapterType, sharedResource, lockable) {}
1396+
};
1397+
MockContext context;
1398+
cl_dx9_surface_info_khr surfaceInfo = {};
1399+
OCLPlane oclPlane = OCLPlane::NO_PLANE;
1400+
std::unique_ptr<D3DSurface> surface(new MockD3DSurface(&context, &surfaceInfo, nullptr, 0, oclPlane, 0, false, false));
1401+
1402+
MockBuffer buffer;
1403+
UpdateData updateData;
1404+
updateData.memObject = &buffer;
1405+
auto result = surface->validateUpdateData(&updateData);
1406+
EXPECT_EQ(CL_INVALID_MEM_OBJECT, result);
1407+
}
13881408
} // namespace OCLRT

0 commit comments

Comments
 (0)