From 7d2421dd0aaebf75a2cbe5d6a0827f1d064c0c3f Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Thu, 4 Apr 2024 13:37:17 +0200 Subject: hardening parsepackagemessage (#38) * hardening of ParsePackageMessage and extended details when malformed attachments are detected --- src/zenutil/packageformat.cpp | 101 +++++++++++++++++++++++++++++------------- 1 file changed, 70 insertions(+), 31 deletions(-) (limited to 'src/zenutil') diff --git a/src/zenutil/packageformat.cpp b/src/zenutil/packageformat.cpp index 7c284a4e6..2e0f2dc7c 100644 --- a/src/zenutil/packageformat.cpp +++ b/src/zenutil/packageformat.cpp @@ -357,6 +357,11 @@ ParsePackageMessage(IoBuffer Payload, std::function PartialFileBuffers; - // TODO: Throwing before this loop completes could result in leaking handles as we might not have picked up all the handles in the - // message + std::vector> MalformedAttachments; + for (uint32_t i = 0; i < ChunkCount; ++i) { const CbAttachmentEntry& Entry = AttachmentEntries[i]; @@ -438,30 +443,34 @@ ParsePackageMessage(IoBuffer Payload, std::functionPayloadByteOffset, - AttachRefHdr->PayloadByteSize)); - } - - IoBuffer ChunkReference = AttachRefHdr->PayloadByteOffset == 0 && AttachRefHdr->PayloadByteSize == FullFileBuffer.GetSize() - ? FullFileBuffer - : IoBuffer(FullFileBuffer, AttachRefHdr->PayloadByteOffset, AttachRefHdr->PayloadByteSize); + IoBuffer ChunkReference = AttachRefHdr->PayloadByteOffset == 0 && AttachRefHdr->PayloadByteSize == FullFileBuffer.GetSize() + ? FullFileBuffer + : IoBuffer(FullFileBuffer, AttachRefHdr->PayloadByteOffset, AttachRefHdr->PayloadByteSize); - CompressedBuffer CompBuf(CompressedBuffer::FromCompressedNoValidate(std::move(ChunkReference))); - if (!CompBuf) + CompressedBuffer CompBuf(CompressedBuffer::FromCompressedNoValidate(std::move(ChunkReference))); + if (CompBuf) + { + Attachments.emplace_back(CbAttachment(std::move(CompBuf), Entry.AttachmentHash)); + } + else + { + MalformedAttachments.push_back(std::make_pair(i, + fmt::format("Invalid format in '{}' (offset {}, size {})", + Path, + AttachRefHdr->PayloadByteOffset, + AttachRefHdr->PayloadByteSize))); + } + } + else { - throw std::invalid_argument(fmt::format("invalid format for chunk #{} at '{}' (offset {}, size {})", - i, - Path, - AttachRefHdr->PayloadByteOffset, - AttachRefHdr->PayloadByteSize)); + MalformedAttachments.push_back(std::make_pair(i, + fmt::format("Unable to resolve chunk at '{}' (offset {}, size {})", + Path, + AttachRefHdr->PayloadByteOffset, + AttachRefHdr->PayloadByteSize))); } - Attachments.emplace_back(CbAttachment(std::move(CompBuf), Entry.AttachmentHash)); } else if (Entry.Flags & CbAttachmentEntry::kIsCompressed) { @@ -470,26 +479,39 @@ ParsePackageMessage(IoBuffer Payload, std::function SB; + SB << (uint64_t)MalformedAttachments.size() << " malformed attachments in package message:\n"; + for (const auto& It : MalformedAttachments) + { + SB << " #"sv << It.first << ": " << It.second << "\n"; + } + ZEN_WARN("{}", SB.ToView()); + throw std::invalid_argument(SB.ToString()); + } + return Package; } -- cgit v1.2.3 From 12b7bf1c16f671c83840961c37a339141a7ffbb3 Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Thu, 4 Apr 2024 14:32:40 +0200 Subject: improved assert (#37) - Improvement: Add file and line to ASSERT exceptions - Improvement: Catch call stack when throwing assert exceptions and log/output call stack at important places to provide more context to caller --- src/zenutil/cache/rpcrecording.cpp | 14 +++++++------- src/zenutil/include/zenutil/logging/rotatingfilesink.h | 10 +++++----- src/zenutil/openprocesscache.cpp | 4 ++-- 3 files changed, 14 insertions(+), 14 deletions(-) (limited to 'src/zenutil') diff --git a/src/zenutil/cache/rpcrecording.cpp b/src/zenutil/cache/rpcrecording.cpp index 759af792d..9bef4d1a4 100644 --- a/src/zenutil/cache/rpcrecording.cpp +++ b/src/zenutil/cache/rpcrecording.cpp @@ -73,7 +73,7 @@ struct RecordedRequestsWriter WriteFile(m_BasePath / "rpc_recording_metadata.zcb", Metadata.GetBuffer().AsIoBuffer()); } - catch (std::exception& Ex) + catch (const std::exception& Ex) { ZEN_WARN("caught exception while generating metadata for RPC recording: {}", Ex.what()); } @@ -455,7 +455,7 @@ RecordedRequestsSegmentWriter::EndWrite() WriteFile(m_BasePath / "rpc_segment_info.zcb", Metadata.GetBuffer().AsIoBuffer()); } - catch (std::exception& Ex) + catch (const std::exception& Ex) { ZEN_WARN("caught exception while writing segment metadata for RPC recording: {}", Ex.what()); } @@ -562,7 +562,7 @@ RecordedRequestsSegmentWriter::WriteRequest(const RecordedRequestInfo& RequestIn m_RequestsByteCount.fetch_add(RequestBuffer.GetSize()); } } - catch (std::exception&) + catch (const std::exception&) { RwLock::ExclusiveLockScope _(m_Lock); m_Entries[RequestIndex].Length = 0; @@ -738,7 +738,7 @@ RecordedRequestsWriter::WriterThreadMain() RecordedRequestsSegmentWriter& Writer = EnsureCurrentSegment(); Writer.WriteRequest(Request.RequestInfo, Request.RequestBuffer); } - catch (std::exception&) + catch (const std::exception&) { // TODO: what's the right behaviour here? The most likely cause would // be some I/O error and we probably ought to just shut down recording @@ -867,7 +867,7 @@ RecordedRequestsWriter::WriteRecordingMetadata() WriteFile(m_BasePath / "rpc_recording_info.zcb", Metadata.GetBuffer().AsIoBuffer()); } - catch (std::exception& Ex) + catch (const std::exception& Ex) { ZEN_WARN("caught exception while writing metadata for RPC recording: {}", Ex.what()); } @@ -913,7 +913,7 @@ RecordedRequestsReader::BeginRead(const std::filesystem::path& BasePath, bool In return TotalRequestCount; } - catch (std::exception& Ex) + catch (const std::exception& Ex) { ZEN_WARN("could not read metadata file: {}", Ex.what()); } @@ -950,7 +950,7 @@ RecordedRequestsReader::BeginRead(const std::filesystem::path& BasePath, bool In } } } - catch (std::exception&) + catch (const std::exception&) { } diff --git a/src/zenutil/include/zenutil/logging/rotatingfilesink.h b/src/zenutil/include/zenutil/logging/rotatingfilesink.h index e4a99fc30..ca4649ba8 100644 --- a/src/zenutil/include/zenutil/logging/rotatingfilesink.h +++ b/src/zenutil/include/zenutil/logging/rotatingfilesink.h @@ -60,7 +60,7 @@ public: RwLock::ExclusiveLockScope RotateLock(m_Lock); m_CurrentFile.Close(); } - catch (std::exception&) + catch (const std::exception&) { } } @@ -101,7 +101,7 @@ public: } } } - catch (std::exception&) + catch (const std::exception&) { // Silently eat errors } @@ -116,7 +116,7 @@ public: m_CurrentFile.Flush(); } } - catch (std::exception&) + catch (const std::exception&) { // Silently eat errors } @@ -129,7 +129,7 @@ public: RwLock::ExclusiveLockScope _(m_Lock); m_Formatter = spdlog::details::make_unique(pattern); } - catch (std::exception&) + catch (const std::exception&) { // Silently eat errors } @@ -141,7 +141,7 @@ public: RwLock::ExclusiveLockScope _(m_Lock); m_Formatter = std::move(sink_formatter); } - catch (std::exception&) + catch (const std::exception&) { // Silently eat errors } diff --git a/src/zenutil/openprocesscache.cpp b/src/zenutil/openprocesscache.cpp index 39e4aea90..fb654bde2 100644 --- a/src/zenutil/openprocesscache.cpp +++ b/src/zenutil/openprocesscache.cpp @@ -42,7 +42,7 @@ OpenProcessCache::~OpenProcessCache() } m_Sessions.clear(); } - catch (std::exception& Ex) + catch (const std::exception& Ex) { ZEN_ERROR("OpenProcessCache destructor failed with reason: `{}`", Ex.what()); } @@ -175,7 +175,7 @@ OpenProcessCache::GcWorker() { GCHandles(); } - catch (std::exception& Ex) + catch (const std::exception& Ex) { ZEN_ERROR("gc of open process cache failed with reason: `{}`", Ex.what()); } -- cgit v1.2.3