From 087bac4085d1b183f738a4c6b81799015b390124 Mon Sep 17 00:00:00 2001 From: Jamie Reece Wilson Date: Sun, 29 Oct 2023 20:36:11 +0000 Subject: [PATCH] [+] AuByteBuffer::flagNoRealloc [*] Fix IO regression / Critical Bug / Leak and stupid double free --- Include/Aurora/Memory/ByteBuffer.hpp | 3 +- Include/Aurora/Memory/ByteBuffer_Memory.inl | 15 +- Source/IO/Net/AuNetSocketChannel.cpp | 153 ++++++++------------ Source/IO/Net/AuNetSocketChannelOutput.cpp | 1 + Source/IO/Protocol/AuProtocolPiece.cpp | 4 +- Source/IO/Protocol/AuProtocolStack.cpp | 4 +- 6 files changed, 80 insertions(+), 100 deletions(-) diff --git a/Include/Aurora/Memory/ByteBuffer.hpp b/Include/Aurora/Memory/ByteBuffer.hpp index e4cabe62..eaa18a05 100644 --- a/Include/Aurora/Memory/ByteBuffer.hpp +++ b/Include/Aurora/Memory/ByteBuffer.hpp @@ -79,6 +79,7 @@ namespace Aurora::Memory AuUInt8 flagReadError : 1 {}; AuUInt8 flagWriteError : 1 {}; AuUInt8 flagNoFree : 1 {}; + AuUInt8 flagNoRealloc: 1 {}; AuUInt8 flagAlwaysExpandable : 1 {}; // it's a long story from how we got from string views to std::vectors to current day AuByteBuffer. // anyway long story short, if you want a buffered api to write into us linearly and grow, enable me. // if you just want ::Write and similar functions to work keem me false and enable flagExpandable. @@ -277,7 +278,7 @@ namespace Aurora::Memory inline ~ByteBuffer() { - if (this->base) + if (this->base && !this->flagNoFree) { Free(this->base); } diff --git a/Include/Aurora/Memory/ByteBuffer_Memory.inl b/Include/Aurora/Memory/ByteBuffer_Memory.inl index 125ed89c..c0da5ce2 100644 --- a/Include/Aurora/Memory/ByteBuffer_Memory.inl +++ b/Include/Aurora/Memory/ByteBuffer_Memory.inl @@ -11,7 +11,8 @@ namespace Aurora::Memory { bool ByteBuffer::Allocate(AuUInt length) { - if (this->flagNoFree) + if (this->flagNoFree || + this->flagNoRealloc) { return false; } @@ -45,7 +46,8 @@ namespace Aurora::Memory bool ByteBuffer::Allocate(AuUInt length, AuUInt alignment) { - if (this->flagNoFree) + if (this->flagNoFree || + this->flagNoRealloc) { return false; } @@ -80,7 +82,8 @@ namespace Aurora::Memory bool ByteBuffer::SetBuffer(MemoryViewRead readView, bool bMoveWriteHeadForReaders) { - if (this->flagNoFree) + if (this->flagNoFree || + this->flagNoRealloc) { return false; } @@ -134,7 +137,8 @@ namespace Aurora::Memory void ByteBuffer::GC() { - if (this->flagNoFree) + if (this->flagNoFree || + this->flagNoRealloc) { return; } @@ -182,7 +186,8 @@ namespace Aurora::Memory bool ByteBuffer::Resize(AuUInt length) { - if (this->flagNoFree) + if (this->flagNoFree || + this->flagNoRealloc) { return false; } diff --git a/Source/IO/Net/AuNetSocketChannel.cpp b/Source/IO/Net/AuNetSocketChannel.cpp index 625b67a4..b69fa6d3 100644 --- a/Source/IO/Net/AuNetSocketChannel.cpp +++ b/Source/IO/Net/AuNetSocketChannel.cpp @@ -552,6 +552,15 @@ namespace Aurora::IO::Net this->inputChannel.pNetReader.reset(); } + static bool ForceResize(AuByteBuffer &buffer, AuUInt uLength) + { + auto old = buffer.flagNoRealloc; + buffer.flagNoRealloc = false; + bool bRet = buffer.Resize(uLength); + buffer.flagNoRealloc = old; + return bRet; + } + void SocketChannel::DoBufferResizeOnThread(bool bInput, bool bOutput, AuUInt uBytes, @@ -577,49 +586,30 @@ namespace Aurora::IO::Net return; } + bool bHasCurrentSuccess {}; + if (bOutput) { if (this->outputChannel.CanResize()) { - AuByteBuffer newBuffer(uBytes, false, false); - if (!(newBuffer.IsValid())) + if (ForceResize(this->outputChannel.GetByteBuffer(), uBytes)) { - SysPushErrorMemory(); - if (pCallbackOptional) - { - pCallbackOptional->OnFailure((void *)nullptr); - } - return; + bOutput = false; + bHasCurrentSuccess = !bInput; } - - auto &byteBufferRef = this->outputChannel.GetByteBuffer(); - auto oldReadHead = byteBufferRef.readPtr; - - if (!newBuffer.WriteFrom(byteBufferRef)) - { - SysPushErrorMemory(); - - this->outputChannel.GetByteBuffer().readPtr = oldReadHead; - - if (pCallbackOptional) - { - pCallbackOptional->OnFailure((void *)nullptr); - } - - return; - } - - byteBufferRef = AuMove(newBuffer); - - if (pCallbackOptional) - { - pCallbackOptional->OnSuccess((void *)nullptr); - } - - return; } } + if (bHasCurrentSuccess) + { + if (pCallbackOptional) + { + pCallbackOptional->OnSuccess((void *)nullptr); + } + + return; + } + { AU_LOCK_GUARD(this->spinLock); @@ -627,22 +617,31 @@ namespace Aurora::IO::Net { if (this->uBytesOutputBufferRetarget) { - if (this->pRetargetOutput) + if (auto pOutput = AuExchange(this->pRetargetOutput, {})) { - this->pRetargetOutput->OnFailure((void *)nullptr); + pOutput->OnFailure((void *)nullptr); + if (this->pRetargetInput == pOutput) + { + this->pRetargetInput = {}; + } } } this->uBytesOutputBufferRetarget = uBytes; this->pRetargetOutput = pCallbackOptional; } - else + + if (bInput) { if (this->uBytesInputBufferRetarget) { - if (this->pRetargetInput) + if (auto pInput = AuExchange(this->pRetargetInput, {})) { - this->pRetargetInput->OnFailure((void *)nullptr); + pInput->OnFailure((void *)nullptr); + if (this->pRetargetOutput == pInput) + { + this->pRetargetOutput = {}; + } } } @@ -661,43 +660,29 @@ namespace Aurora::IO::Net return; } - AuByteBuffer newBuffer(this->uBytesOutputBufferRetarget, true, false); - if (!(newBuffer.IsValid())) - { - SysPushErrorMemory(); - if (this->pRetargetOutput) - { - this->pRetargetOutput->OnFailure((void *)nullptr); - } - return; - } - - auto &byteBufferRef = this->outputChannel.GetByteBuffer(); - auto oldReadHead = byteBufferRef.readPtr; - auto oldWriteHead = byteBufferRef.writePtr; - - if (!newBuffer.WriteFrom(byteBufferRef)) + if (!ForceResize(this->outputChannel.GetByteBuffer(), uBytesOutputBufferRetarget)) { SysPushErrorMemory(); - this->outputChannel.GetByteBuffer().readPtr = oldReadHead; - this->outputChannel.GetByteBuffer().writePtr = oldWriteHead; - - if (this->pRetargetOutput) + if (auto pOutput = AuExchange(this->pRetargetOutput, {})) { - this->pRetargetOutput->OnFailure((void *)nullptr); + pOutput->OnFailure((void *)nullptr); + if (this->pRetargetInput == pOutput) + { + this->pRetargetInput = {}; + } } - return; } - byteBufferRef = AuMove(newBuffer); - this->uBytesOutputBufferRetarget = 0; - if (this->pRetargetOutput) + if (auto pOutput = AuExchange(this->pRetargetOutput, {})) { - this->pRetargetOutput->OnSuccess((void *)nullptr); + if (this->pRetargetInput != pOutput) + { + pOutput->OnSuccess((void *)nullptr); + } } } @@ -710,43 +695,31 @@ namespace Aurora::IO::Net return; } - AuByteBuffer newBuffer(this->uBytesInputBufferRetarget, true, false); - if (!(newBuffer.IsValid())) - { - SysPushErrorMemory(); - if (this->pRetargetInput) - { - this->pRetargetInput->OnFailure((void *)nullptr); - } - return; - } - - auto byteBufferRef = this->inputChannel.AsReadableByteBuffer(); - auto oldReadHead = byteBufferRef->readPtr; - auto oldWriteHead = byteBufferRef->writePtr; - - if (!newBuffer.WriteFrom(*byteBufferRef.get())) + if (!ForceResize(*this->inputChannel.AsReadableByteBuffer(), uBytesOutputBufferRetarget)) { SysPushErrorMemory(); - byteBufferRef->readPtr = oldReadHead; - byteBufferRef->writePtr = oldWriteHead; - - if (this->pRetargetInput) + if (auto pInput = AuExchange(this->pRetargetInput, {})) { - this->pRetargetInput->OnFailure((void *)nullptr); + pInput->OnFailure((void *)nullptr); + + if (this->pRetargetOutput == pInput) + { + this->pRetargetOutput = {}; + } } return; } - byteBufferRef.get()->operator=(AuMove(newBuffer)); - this->uBytesInputBufferRetarget = 0; - if (this->pRetargetInput) + if (auto pInput = AuExchange(this->pRetargetInput, {})) { - this->pRetargetInput->OnSuccess((void *)nullptr); + if (this->pRetargetOutput != pInput) + { + pInput->OnSuccess((void *)nullptr); + } } } diff --git a/Source/IO/Net/AuNetSocketChannelOutput.cpp b/Source/IO/Net/AuNetSocketChannelOutput.cpp index 42625b6d..c855aa9c 100644 --- a/Source/IO/Net/AuNetSocketChannelOutput.cpp +++ b/Source/IO/Net/AuNetSocketChannelOutput.cpp @@ -27,6 +27,7 @@ namespace Aurora::IO::Net outputBuffer_(kDefaultBufferSize, true) { this->outputWriteQueue_.pBase = pParent; + this->outputBuffer_.flagNoRealloc = true; } bool SocketChannelOutput::IsValid() diff --git a/Source/IO/Protocol/AuProtocolPiece.cpp b/Source/IO/Protocol/AuProtocolPiece.cpp index e1f75d0c..c1c47778 100644 --- a/Source/IO/Protocol/AuProtocolPiece.cpp +++ b/Source/IO/Protocol/AuProtocolPiece.cpp @@ -77,11 +77,11 @@ namespace Aurora::IO::Protocol } } - this->outputBuffer.flagNoFree = false; + this->outputBuffer.flagNoRealloc = false; auto bRet = this->outputBuffer.Resize(uOutputLength); if (!this->uMax.has_value()) { - this->outputBuffer.flagNoFree = true; + this->outputBuffer.flagNoRealloc = true; } return bRet; } diff --git a/Source/IO/Protocol/AuProtocolStack.cpp b/Source/IO/Protocol/AuProtocolStack.cpp index c60d8e82..07a50339 100644 --- a/Source/IO/Protocol/AuProtocolStack.cpp +++ b/Source/IO/Protocol/AuProtocolStack.cpp @@ -120,7 +120,7 @@ namespace Aurora::IO::Protocol return {}; } - pNew->outputBuffer.flagNoFree = true; + pNew->outputBuffer.flagNoRealloc = true; // Circular ref pNew->pOuputWriter = AuMakeShared(AuSPtr(pNew, &pNew->outputBuffer)); @@ -266,7 +266,7 @@ namespace Aurora::IO::Protocol } else { - pNew->outputBuffer.flagNoFree = true; + pNew->outputBuffer.flagNoRealloc = true; } pNew->uStartingSize = uOutputBufferSize;