Move sequence number into ProcessResource
... so that sequence number won't suffer from use-after-free if puid
is reused for a new process.
Bug: b/238792446
Change-Id: I3a452a11f0c4f0b952e7904a17de309a96aecf62
diff --git a/stream-servers/FrameBuffer.cpp b/stream-servers/FrameBuffer.cpp
index 55a0d3a..f20a0c4 100644
--- a/stream-servers/FrameBuffer.cpp
+++ b/stream-servers/FrameBuffer.cpp
@@ -2115,8 +2115,7 @@
void FrameBuffer::createGraphicsProcessResources(uint64_t puid) {
AutoLock mutex(m_lock);
- bool inserted =
- m_procOwnedResources.try_emplace(puid, std::make_unique<ProcessResources>()).second;
+ bool inserted = m_procOwnedResources.try_emplace(puid, ProcessResources::create()).second;
if (!inserted) {
WARN("Failed to create process resource for puid %" PRIu64 ".", puid);
}
@@ -2170,14 +2169,6 @@
}
}
- {
- auto procIte = m_procOwnedSequenceNumbers.find(puid);
- if (procIte != m_procOwnedSequenceNumbers.end()) {
- delete procIte->second;
- m_procOwnedSequenceNumbers.erase(procIte);
- }
- }
-
mutex.unlock();
for (auto handle : colorBuffersToCleanup) {
@@ -3516,13 +3507,7 @@
}
}
- while (m_procOwnedSequenceNumbers.size()) {
- auto it = m_procOwnedSequenceNumbers.begin();
- while (it != m_procOwnedSequenceNumbers.end()) {
- delete it->second;
- it = m_procOwnedSequenceNumbers.erase(it);
- }
- }
+ m_procOwnedResources.clear();
performDelayedColorBufferCloseLocked(true);
@@ -3699,30 +3684,14 @@
callbackMap.erase(key);
}
-void FrameBuffer::registerProcessSequenceNumberForPuid(uint64_t puid) {
+const ProcessResources* FrameBuffer::getProcessResources(uint64_t puid) {
AutoLock mutex(m_lock);
-
- auto procIte = m_procOwnedSequenceNumbers.find(puid);
- if (procIte != m_procOwnedSequenceNumbers.end()) {
- if (procIte->first != puid) {
- GFXSTREAM_ABORT(FatalError(ABORT_REASON_OTHER))
- << "puid for the associated RenderThread already set.";
- }
- return;
+ auto i = m_procOwnedResources.find(puid);
+ if (i == m_procOwnedResources.end()) {
+ ERR("Failed to find process owned resources for puid %" PRIu64 ".", puid);
+ return nullptr;
}
- uint32_t* seqnoPtr = new uint32_t;
- *seqnoPtr = 0;
- m_procOwnedSequenceNumbers[puid] = seqnoPtr;
-}
-
-uint32_t* FrameBuffer::getProcessSequenceNumberPtr(uint64_t puid) {
- AutoLock mutex(m_lock);
-
- auto procIte = m_procOwnedSequenceNumbers.find(puid);
- if (procIte != m_procOwnedSequenceNumbers.end()) {
- return procIte->second;
- }
- return nullptr;
+ return i->second.get();
}
int FrameBuffer::createDisplay(uint32_t *displayId) {
diff --git a/stream-servers/FrameBuffer.h b/stream-servers/FrameBuffer.h
index 7261e72..5c062d7 100644
--- a/stream-servers/FrameBuffer.h
+++ b/stream-servers/FrameBuffer.h
@@ -82,7 +82,22 @@
BufferPtr buffer;
};
-struct ProcessResources {};
+class ProcessResources {
+ public:
+ // We only allow ProcessResources to be created on the heap, because the pointer to
+ // mSequenceNumber shouldn't change until ProcessResources is destroyed.
+ static std::unique_ptr<ProcessResources> create() {
+ return std::unique_ptr<ProcessResources>(new ProcessResources());
+ }
+ DISALLOW_COPY_ASSIGN_AND_MOVE(ProcessResources);
+
+ ~ProcessResources() = default;
+ uint32_t* getSequenceNumberPtr() const { return &mSequenceNumber; }
+
+ private:
+ ProcessResources() : mSequenceNumber(0) {}
+ mutable uint32_t mSequenceNumber;
+};
typedef std::unordered_map<HandleType, std::pair<WindowSurfacePtr, HandleType>>
WindowSurfaceMap;
@@ -107,8 +122,6 @@
typedef std::unordered_map<void*, std::function<void()>> CallbackMap;
typedef std::unordered_map<uint64_t, CallbackMap> ProcOwnedCleanupCallbacks;
-typedef std::unordered_map<uint64_t, uint32_t*> ProcOwnedSequenceNumbers;
-
// A structure used to list the capabilities of the underlying EGL
// implementation that the FrameBuffer instance depends on.
// |has_eglimage_texture_2d| is true iff the EGL_KHR_gl_texture_2D_image
@@ -589,8 +602,7 @@
std::function<void()> callback);
void unregisterProcessCleanupCallback(void* key);
- void registerProcessSequenceNumberForPuid(uint64_t puid);
- uint32_t* getProcessSequenceNumberPtr(uint64_t puid);
+ const ProcessResources* getProcessResources(uint64_t puid);
int createDisplay(uint32_t *displayId);
int createDisplay(uint32_t displayId);
@@ -810,7 +822,6 @@
ProcOwnedEGLImages m_procOwnedEGLImages;
ProcOwnedRenderContexts m_procOwnedRenderContext;
ProcOwnedCleanupCallbacks m_procOwnedCleanupCallbacks;
- ProcOwnedSequenceNumbers m_procOwnedSequenceNumbers;
std::unordered_map<uint64_t, std::unique_ptr<ProcessResources>> m_procOwnedResources;
// Flag set when emulator is shutting down.
diff --git a/stream-servers/RenderControl.cpp b/stream-servers/RenderControl.cpp
index a0a34cf..13088fe 100644
--- a/stream-servers/RenderControl.cpp
+++ b/stream-servers/RenderControl.cpp
@@ -1262,8 +1262,6 @@
static void rcSetPuid(uint64_t puid) {
RenderThreadInfo *tInfo = RenderThreadInfo::get();
tInfo->m_puid = puid;
- FrameBuffer *fb = FrameBuffer::getFB();
- fb->registerProcessSequenceNumberForPuid(puid);
}
static int rcCompose(uint32_t bufferSize, void* buffer) {
diff --git a/stream-servers/RenderThread.cpp b/stream-servers/RenderThread.cpp
index 3a4e353..bc3090a 100644
--- a/stream-servers/RenderThread.cpp
+++ b/stream-servers/RenderThread.cpp
@@ -350,7 +350,7 @@
GfxApiLogger gfxLogger;
auto& metricsLogger = FrameBuffer::getFB()->getMetricsLogger();
- uint32_t* seqnoPtr = nullptr;
+ const ProcessResources* processResources = nullptr;
while (true) {
// Let's make sure we read enough data for at least some processing.
@@ -437,8 +437,8 @@
.setAnnotations(std::move(renderThreadData))
.build();
- if (!seqnoPtr && tInfo.m_puid) {
- seqnoPtr = FrameBuffer::getFB()->getProcessSequenceNumberPtr(tInfo.m_puid);
+ if (!processResources && tInfo.m_puid) {
+ processResources = FrameBuffer::getFB()->getProcessResources(tInfo.m_puid);
}
progress = false;
@@ -458,7 +458,8 @@
.metricsLogger = &metricsLogger,
};
last = tInfo.m_vkInfo->m_vkDec.decode(readBuf.buf(), readBuf.validData(), ioStream,
- seqnoPtr, context);
+ processResources->getSequenceNumberPtr(),
+ context);
if (last > 0) {
readBuf.consume(last);
progress = true;
