From 151b2854e32b338e153ca722438416e61e130583 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Tue, 16 Apr 2024 23:47:20 +0200 Subject: [PATCH 1/2] Fix potential client crash when console backlog is full Fix backlog corruption in `CConsole::PumpBacklogPending` when many backlog entries are allocated at the same time. When allocating many entries from the `m_Backlog` ringbuffer at the same time, the first entries being allocated may already have been recycled again, so the pointers to them being stored in the temporary vector of new backlog entries were pointing arbitrarily into the ringbuffer data, which could cause corruption of the structural and user data of the ringbuffer. Now, we iterate over the entire backlog and only update uninitialized entries instead of storing the new entries separately. This was sometimes caught as a misaligned access with UBSan: ``` src/engine/shared/ringbuffer.cpp:160:14: runtime error: member access within misaligned address 0x00014126f4df for type 'struct CItem', which requires 8 byte alignment 0x00014126f4df: note: pointer points here 0 0x5825349a6a1c in CRingBufferBase::Prev(void*) src/engine/shared/ringbuffer.cpp:160 1 0x5825334e8934 in CTypedRingBuffer::Prev(CGameConsole::CInstance::CBacklogEntry*) src/engine/shared/ringbuffer.h:59 2 0x5825334d13e6 in CGameConsole::OnRender() src/game/client/components/console.cpp:1259 3 0x582533bce058 in CGameClient::OnRender() src/game/client/gameclient.cpp:715 4 0x582532f3cc44 in CClient::Render() src/engine/client/client.cpp:894 5 0x582532f9d236 in CClient::Run() src/engine/client/client.cpp:2971 6 0x582533002e5e in main src/engine/client/client.cpp:4523 ``` --- src/game/client/components/console.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/game/client/components/console.cpp b/src/game/client/components/console.cpp index 0805f4c5c..ab5f19d7b 100644 --- a/src/game/client/components/console.cpp +++ b/src/game/client/components/console.cpp @@ -236,7 +236,7 @@ void CGameConsole::CInstance::ClearBacklog() void CGameConsole::CInstance::UpdateBacklogTextAttributes() { // Pending backlog entries are not handled because they don't have text attributes yet. - for(CInstance::CBacklogEntry *pEntry = m_Backlog.First(); pEntry; pEntry = m_Backlog.Next(pEntry)) + for(CBacklogEntry *pEntry = m_Backlog.First(); pEntry; pEntry = m_Backlog.Next(pEntry)) { UpdateEntryTextAttributes(pEntry); } @@ -244,27 +244,29 @@ void CGameConsole::CInstance::UpdateBacklogTextAttributes() void CGameConsole::CInstance::PumpBacklogPending() { - std::vector vpEntries; { // We must ensure that no log messages are printed while owning // m_BacklogPendingLock or this will result in a dead lock. const CLockScope LockScopePending(m_BacklogPendingLock); - for(CInstance::CBacklogEntry *pPendingEntry = m_BacklogPending.First(); pPendingEntry; pPendingEntry = m_BacklogPending.Next(pPendingEntry)) + for(CBacklogEntry *pPendingEntry = m_BacklogPending.First(); pPendingEntry; pPendingEntry = m_BacklogPending.Next(pPendingEntry)) { const size_t EntrySize = sizeof(CBacklogEntry) + pPendingEntry->m_Length; CBacklogEntry *pEntry = m_Backlog.Allocate(EntrySize); mem_copy(pEntry, pPendingEntry, EntrySize); - vpEntries.push_back(pEntry); } m_BacklogPending.Init(); } + // Update text attributes and count number of added lines m_pGameConsole->Ui()->MapScreen(); - for(CInstance::CBacklogEntry *pEntry : vpEntries) + for(CBacklogEntry *pEntry = m_Backlog.First(); pEntry; pEntry = m_Backlog.Next(pEntry)) { - UpdateEntryTextAttributes(pEntry); - m_NewLineCounter += pEntry->m_LineCount; + if(pEntry->m_LineCount == -1) + { + UpdateEntryTextAttributes(pEntry); + m_NewLineCounter += pEntry->m_LineCount; + } } } From fbe559ea7947b80101b7c11e57c48dd6a8f554ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Sun, 21 Apr 2024 12:48:41 +0200 Subject: [PATCH 2/2] Fix console not keeping scroll position when backlog is full The console was not keeping its current scoll position if entries from the backlog are removed due to being recycled when new entries are added. For this purpose, a callback function is added to the ringbuffer to handle popped items, so the scrolling position of the console can be updated based on the number of lines of the removed backlog entries. --- src/engine/shared/ringbuffer.cpp | 10 ++++++++++ src/engine/shared/ringbuffer.h | 11 +++++++++++ src/game/client/components/console.cpp | 11 ++++++++++- 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/engine/shared/ringbuffer.cpp b/src/engine/shared/ringbuffer.cpp index f9872fead..c8697942e 100644 --- a/src/engine/shared/ringbuffer.cpp +++ b/src/engine/shared/ringbuffer.cpp @@ -123,11 +123,21 @@ void *CRingBufferBase::Allocate(int Size) return (void *)(pBlock + 1); } +void CRingBufferBase::SetPopCallback(std::function PopCallback) +{ + m_PopCallback = std::move(PopCallback); +} + int CRingBufferBase::PopFirst() { if(m_pConsume->m_Free) return 0; + if(m_PopCallback) + { + m_PopCallback(m_pConsume + 1); + } + // set the free flag m_pConsume->m_Free = 1; diff --git a/src/engine/shared/ringbuffer.h b/src/engine/shared/ringbuffer.h index 8d089260a..11af0fa32 100644 --- a/src/engine/shared/ringbuffer.h +++ b/src/engine/shared/ringbuffer.h @@ -5,6 +5,8 @@ #include +#include + class CRingBufferBase { class CItem @@ -25,6 +27,8 @@ class CRingBufferBase int m_Size; int m_Flags; + std::function m_PopCallback = nullptr; + CItem *NextBlock(CItem *pItem); CItem *PrevBlock(CItem *pItem); CItem *MergeBack(CItem *pItem); @@ -39,6 +43,7 @@ protected: void Init(void *pMemory, int Size, int Flags); int PopFirst(); + void SetPopCallback(const std::function PopCallback); public: enum @@ -55,6 +60,12 @@ class CTypedRingBuffer : public CRingBufferBase public: T *Allocate(int Size) { return (T *)CRingBufferBase::Allocate(Size); } int PopFirst() { return CRingBufferBase::PopFirst(); } + void SetPopCallback(std::function PopCallback) + { + CRingBufferBase::SetPopCallback([PopCallback](void *pCurrent) { + PopCallback((T *)pCurrent); + }); + } T *Prev(T *pCurrent) { return (T *)CRingBufferBase::Prev(pCurrent); } T *Next(T *pCurrent) { return (T *)CRingBufferBase::Next(pCurrent); } diff --git a/src/game/client/components/console.cpp b/src/game/client/components/console.cpp index ab5f19d7b..a01e272bc 100644 --- a/src/game/client/components/console.cpp +++ b/src/game/client/components/console.cpp @@ -208,6 +208,13 @@ CGameConsole::CInstance::CInstance(int Type) m_IsCommand = false; + m_Backlog.SetPopCallback([this](CBacklogEntry *pEntry) { + if(pEntry->m_LineCount != -1) + { + m_NewLineCounter -= pEntry->m_LineCount; + } + }); + m_Input.SetClipboardLineCallback([this](const char *pStr) { ExecuteLine(pStr); }); m_CurrentMatchIndex = -1; @@ -1135,7 +1142,7 @@ void CGameConsole::OnRender() } pConsole->PumpBacklogPending(); - if(pConsole->m_NewLineCounter > 0) + if(pConsole->m_NewLineCounter != 0) { pConsole->UpdateSearch(); @@ -1145,6 +1152,8 @@ void CGameConsole::OnRender() pConsole->m_BacklogCurLine += pConsole->m_NewLineCounter; pConsole->m_BacklogLastActiveLine += pConsole->m_NewLineCounter; } + if(pConsole->m_NewLineCounter < 0) + pConsole->m_NewLineCounter = 0; } // render console log (current entry, status, wrap lines)