From 2da19d5d6b7c917400d0688ec8c6aa40fa997377 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Tue, 20 Mar 2018 19:11:41 -0400 Subject: [PATCH] Fix double MapReadCallback trigger when Unmapping in it. When calling unmap on a mapped buffer for which the callback hasn't fired yet, the callback should be called with UNKNOWN. The code marked the callback as called only after calling it, causing problems with re-entrancy where the callback would be called twice. This could also get triggered by destroying the buffer inside the callback. Fix this in backend::Buffer and the WireClient and add test for both. --- generator/templates/wire/WireClient.cpp | 3 +- src/backend/Buffer.cpp | 5 +- src/tests/unittests/WireTests.cpp | 52 +++++++++++++++++++ .../validation/BufferValidationTests.cpp | 30 +++++++++++ 4 files changed, 88 insertions(+), 2 deletions(-) diff --git a/generator/templates/wire/WireClient.cpp b/generator/templates/wire/WireClient.cpp index 9f7f955735..861e27bde9 100644 --- a/generator/templates/wire/WireClient.cpp +++ b/generator/templates/wire/WireClient.cpp @@ -532,6 +532,8 @@ namespace wire { } auto request = requestIt->second; + // Delete the request before calling the callback otherwise the callback could be fired a second time if for example buffer.Unmap() is called inside the callback. + buffer->readRequests.erase(requestIt); //* On success, we copy the data locally because the IPC buffer isn't valid outside of this function if (cmd->status == NXT_BUFFER_MAP_READ_STATUS_SUCCESS) { @@ -553,7 +555,6 @@ namespace wire { request.callback(static_cast(cmd->status), nullptr, request.userdata); } - buffer->readRequests.erase(requestIt); return true; } }; diff --git a/src/backend/Buffer.cpp b/src/backend/Buffer.cpp index d3401dec03..055937adc2 100644 --- a/src/backend/Buffer.cpp +++ b/src/backend/Buffer.cpp @@ -61,8 +61,11 @@ namespace backend { nxtBufferMapReadStatus status, const void* pointer) { if (mMapReadCallback && serial == mMapReadSerial) { - mMapReadCallback(status, pointer, mMapReadUserdata); + // Tag the callback as fired before firing it, otherwise it could fire a second time if + // for example buffer.Unmap() is called inside the application-provided callback. + nxtBufferMapReadCallback callback = mMapReadCallback; mMapReadCallback = nullptr; + callback(status, pointer, mMapReadUserdata); } } diff --git a/src/tests/unittests/WireTests.cpp b/src/tests/unittests/WireTests.cpp index 498027cd50..10a66f5dbb 100644 --- a/src/tests/unittests/WireTests.cpp +++ b/src/tests/unittests/WireTests.cpp @@ -724,3 +724,55 @@ TEST_F(WireBufferMappingTests, MappingErrorWhileAlreadyMappedGetsNullptr) { FlushServer(); } + +// Test that the MapReadCallback isn't fired twice when unmap() is called inside the callback +TEST_F(WireBufferMappingTests, UnmapInsideMapReadCallback) { + nxtCallbackUserdata userdata = 2039; + nxtBufferMapReadAsync(buffer, 40, sizeof(uint32_t), ToMockBufferMapReadCallback, userdata); + + uint32_t bufferContent = 31337; + EXPECT_CALL(api, OnBufferMapReadAsyncCallback(apiBuffer, 40, sizeof(uint32_t), _, _)) + .WillOnce(InvokeWithoutArgs([&]() { + api.CallMapReadCallback(apiBuffer, NXT_BUFFER_MAP_READ_STATUS_SUCCESS, &bufferContent); + })); + + FlushClient(); + + EXPECT_CALL(*mockBufferMapReadCallback, Call(NXT_BUFFER_MAP_READ_STATUS_SUCCESS, Pointee(Eq(bufferContent)), userdata)) + .WillOnce(InvokeWithoutArgs([&]() { + nxtBufferUnmap(buffer); + })); + + FlushServer(); + + EXPECT_CALL(api, BufferUnmap(apiBuffer)) + .Times(1); + + FlushClient(); +} + +// Test that the MapReadCallback isn't fired twice the buffer external refcount reaches 0 in the callback +TEST_F(WireBufferMappingTests, DestroyInsideMapReadCallback) { + nxtCallbackUserdata userdata = 2039; + nxtBufferMapReadAsync(buffer, 40, sizeof(uint32_t), ToMockBufferMapReadCallback, userdata); + + uint32_t bufferContent = 31337; + EXPECT_CALL(api, OnBufferMapReadAsyncCallback(apiBuffer, 40, sizeof(uint32_t), _, _)) + .WillOnce(InvokeWithoutArgs([&]() { + api.CallMapReadCallback(apiBuffer, NXT_BUFFER_MAP_READ_STATUS_SUCCESS, &bufferContent); + })); + + FlushClient(); + + EXPECT_CALL(*mockBufferMapReadCallback, Call(NXT_BUFFER_MAP_READ_STATUS_SUCCESS, Pointee(Eq(bufferContent)), userdata)) + .WillOnce(InvokeWithoutArgs([&]() { + nxtBufferRelease(buffer); + })); + + FlushServer(); + + EXPECT_CALL(api, BufferRelease(apiBuffer)) + .Times(1); + + FlushClient(); +} diff --git a/src/tests/unittests/validation/BufferValidationTests.cpp b/src/tests/unittests/validation/BufferValidationTests.cpp index 1c2b914348..aebd44b9d1 100644 --- a/src/tests/unittests/validation/BufferValidationTests.cpp +++ b/src/tests/unittests/validation/BufferValidationTests.cpp @@ -290,6 +290,36 @@ TEST_F(BufferValidationTest, MapReadUnmapBeforeResultThenMapAgain) { queue.Submit(0, nullptr); } +// Test that the MapReadCallback isn't fired twice when unmap() is called inside the callback +TEST_F(BufferValidationTest, UnmapInsideMapReadCallback) { + nxt::Buffer buf = CreateMapReadBuffer(4); + + nxt::CallbackUserdata userdata = 40678; + buf.MapReadAsync(0, 4, ToMockBufferMapReadCallback, userdata); + + EXPECT_CALL(*mockBufferMapReadCallback, Call(NXT_BUFFER_MAP_READ_STATUS_SUCCESS, Ne(nullptr), userdata)) + .WillOnce(InvokeWithoutArgs([&]() { + buf.Unmap(); + })); + + queue.Submit(0, nullptr); +} + +// Test that the MapReadCallback isn't fired twice the buffer external refcount reaches 0 in the callback +TEST_F(BufferValidationTest, DestroyInsideMapReadCallback) { + nxt::Buffer buf = CreateMapReadBuffer(4); + + nxt::CallbackUserdata userdata = 40679; + buf.MapReadAsync(0, 4, ToMockBufferMapReadCallback, userdata); + + EXPECT_CALL(*mockBufferMapReadCallback, Call(NXT_BUFFER_MAP_READ_STATUS_SUCCESS, Ne(nullptr), userdata)) + .WillOnce(InvokeWithoutArgs([&]() { + buf = nxt::Buffer(); + })); + + queue.Submit(0, nullptr); +} + // Test the success case for Buffer::SetSubData TEST_F(BufferValidationTest, SetSubDataSuccess) { nxt::Buffer buf = CreateSetSubDataBuffer(4);