From 9a23aa773b93b0be8a3d85437f5f267d405bd87c Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Sun, 28 Jul 2024 19:29:27 +0200 Subject: [PATCH] Invocation: analyse usage of buffer metadata entries Code clean-up: mark all buffers with a dedicated tagging type The point in question is: if we work the LocalTag into the type-hash, could it be possible to miss an existing entry in the metadata registry? This could cause two entries to be locked for a single buffer address, leading to data corruption. As far as I can see, in the current usage this would not happen, but unfortunately this problem can not be ruled out, since the BufferProvider API and protocol is designed to be open for various usage patterns. However, the same potentially disastrous pattern could also materialise when registering two different buffer types, and then locking each for the same buffer location. --- src/steam/engine/buffer-metadata.hpp | 21 +-- src/steam/engine/buffer-provider.cpp | 6 +- src/steam/engine/buffer-provider.hpp | 9 +- src/steam/engine/buffhandle.hpp | 2 +- .../engine/tracking-heap-block-provider.cpp | 14 +- .../engine/tracking-heap-block-provider.hpp | 8 +- .../steam/engine/buffer-metadata-test.cpp | 59 ++++---- wiki/thinkPad.ichthyo.mm | 132 +++++++++++++----- 8 files changed, 170 insertions(+), 81 deletions(-) diff --git a/src/steam/engine/buffer-metadata.hpp b/src/steam/engine/buffer-metadata.hpp index 6dfb85ac0..255aa4067 100644 --- a/src/steam/engine/buffer-metadata.hpp +++ b/src/steam/engine/buffer-metadata.hpp @@ -59,6 +59,7 @@ #include "lib/hash-value.h" #include "lib/util-foreach.hpp" #include "include/logging.h" +#include "steam/streamtype.hpp" #include "steam/engine/type-handler.hpp" #include "steam/engine/buffer-local-key.hpp" #include "lib/nocopy.hpp" @@ -76,6 +77,8 @@ namespace engine { namespace error = lumiera::error; namespace metadata { + using Buff = StreamType::ImplFacade::DataBuffer; + class Key; class Entry; } @@ -214,7 +217,7 @@ namespace engine { * For NULL buffer a copy of the parent is returned. */ static Key - forEntry (Key const& parent, const void* bufferAddr, LocalTag const& localTag =LocalTag::UNKNOWN) + forEntry (Key const& parent, const Buff* bufferAddr, LocalTag const& localTag =LocalTag::UNKNOWN) { Key newKey{parent}; // copy of parent as baseline if (nontrivial(localTag)) @@ -270,11 +273,11 @@ namespace engine { : public Key { BufferState state_; - void* buffer_; + Buff* buffer_; protected: Entry (Key const& parent - ,void* bufferPtr =nullptr + ,Buff* bufferPtr =nullptr ,LocalTag const& specialTag =LocalTag::UNKNOWN ) : Key{Key::forEntry (parent, bufferPtr, specialTag)} @@ -313,7 +316,7 @@ namespace engine { return state_; } - void* + Buff* access() { __must_not_be_NIL(); @@ -350,7 +353,7 @@ namespace engine { } Entry& - lock (void* newBuffer) + lock (Buff* newBuffer) { __must_be_FREE(); buffer_ = newBuffer; @@ -362,7 +365,7 @@ namespace engine { { if (buffer_ and invokeDtor) invokeEmbeddedDtor_and_clear(); - buffer_ = 0; + buffer_ = nullptr; state_ = FREE; return *this; } @@ -610,7 +613,7 @@ namespace engine { * @note might create/register a new Entry as a side-effect */ Key const& - key (Key const& parentKey, void* concreteBuffer, LocalTag const& specifics =LocalTag::UNKNOWN) + key (Key const& parentKey, metadata::Buff* concreteBuffer, LocalTag const& specifics =LocalTag::UNKNOWN) { Key derivedKey = Key::forEntry (parentKey, concreteBuffer, specifics); Entry* existing = table_.fetch (derivedKey); @@ -645,7 +648,7 @@ namespace engine { */ Entry& lock (Key const& parentKey - ,void* concreteBuffer + ,metadata::Buff* concreteBuffer ,LocalTag const& specifics =LocalTag::UNKNOWN ,bool onlyNew =false) { @@ -717,7 +720,7 @@ namespace engine { * created, but is marked as FREE */ Entry& - markLocked (Key const& parentKey, void* buffer, LocalTag const& specifics =LocalTag::UNKNOWN) + markLocked (Key const& parentKey, metadata::Buff* buffer, LocalTag const& specifics =LocalTag::UNKNOWN) { if (!buffer) throw error::Fatal{"Attempt to lock for a NULL buffer. Allocation floundered?" diff --git a/src/steam/engine/buffer-provider.cpp b/src/steam/engine/buffer-provider.cpp index 2c759e6a3..0763acdd8 100644 --- a/src/steam/engine/buffer-provider.cpp +++ b/src/steam/engine/buffer-provider.cpp @@ -102,7 +102,7 @@ namespace engine { * actual buffer, which is locked for exclusive use by one client. */ BuffHandle - BufferProvider::buildHandle (HashVal typeID, void* storage, LocalTag const& localTag) + BufferProvider::buildHandle (HashVal typeID, Buff* storage, LocalTag const& localTag) { metadata::Key& typeKey = meta_->get (typeID); metadata::Entry& entry = meta_->markLocked(typeKey, storage, localTag); @@ -189,7 +189,7 @@ namespace engine { try { metadata::Entry& metaEntry = meta_->get (handle.entryID()); metaEntry.mark(FREE); // might invoke embedded dtor function - detachBuffer (metaEntry.parentKey(), metaEntry.localTag()); + detachBuffer (metaEntry.parentKey(), metaEntry.localTag(), *handle); meta_->release (metaEntry); } ERROR_LOG_AND_IGNORE (engine, "releasing a buffer from BufferProvider") @@ -233,7 +233,7 @@ namespace engine { try { metadata::Entry& metaEntry = meta_->get (target.entryID()); metaEntry.invalidate (invokeDtor); - detachBuffer (metaEntry.parentKey(), metaEntry.localTag()); + detachBuffer (metaEntry.parentKey(), metaEntry.localTag(), *target); meta_->release (metaEntry); } ERROR_LOG_AND_IGNORE (engine, "cleanup of buffer metadata while handling an error") diff --git a/src/steam/engine/buffer-provider.hpp b/src/steam/engine/buffer-provider.hpp index be0ced686..a4f2f32e1 100644 --- a/src/steam/engine/buffer-provider.hpp +++ b/src/steam/engine/buffer-provider.hpp @@ -88,13 +88,16 @@ namespace engine { protected: /* === for Implementation by concrete providers === */ + /// placeholder marker type for an actual data buffer + using Buff = StreamType::ImplFacade::DataBuffer; + BufferProvider (Literal implementationID); virtual uint prepareBuffers (uint count, HashVal typeID) =0; virtual BuffHandle provideLockedBuffer (HashVal typeID) =0; - virtual void mark_emitted (HashVal typeID, LocalTag const&) =0; - virtual void detachBuffer (HashVal typeID, LocalTag const&) =0; + virtual void mark_emitted (HashVal, LocalTag const&) =0; + virtual void detachBuffer (HashVal, LocalTag const&, Buff&) =0; public: @@ -131,7 +134,7 @@ namespace engine { size_t getBufferSize (HashVal typeID) const; protected: - BuffHandle buildHandle (HashVal typeID, void* storage, LocalTag const& =LocalTag::UNKNOWN); + BuffHandle buildHandle (HashVal typeID, Buff* storage, LocalTag const& =LocalTag::UNKNOWN); bool was_created_by_this_provider (BuffDescr const&) const; }; diff --git a/src/steam/engine/buffhandle.hpp b/src/steam/engine/buffhandle.hpp index cae55fb9a..c44919fa3 100644 --- a/src/steam/engine/buffhandle.hpp +++ b/src/steam/engine/buffhandle.hpp @@ -119,7 +119,7 @@ namespace engine { */ class BuffHandle { - typedef StreamType::ImplFacade::DataBuffer Buff; + using Buff = StreamType::ImplFacade::DataBuffer; ///< marker type for an actual data buffer BuffDescr descriptor_; Buff* pBuffer_; diff --git a/src/steam/engine/tracking-heap-block-provider.cpp b/src/steam/engine/tracking-heap-block-provider.cpp index 8bc7137b5..bd93ee4f0 100644 --- a/src/steam/engine/tracking-heap-block-provider.cpp +++ b/src/steam/engine/tracking-heap-block-provider.cpp @@ -49,9 +49,18 @@ namespace engine { namespace error = lumiera::error; + using Buff = StreamType::ImplFacade::DataBuffer; + namespace { // implementation helpers... + inline Buff* + asBuffer(void* mem) + {// type tag to mark memory address as Buffer + return static_cast (mem); + } + + using diagn::Block; /** helper to find Block entries @@ -247,7 +256,7 @@ namespace engine { { diagn::BlockPool& blocks = getBlockPoolFor (typeID); diagn::Block& newBlock = blocks.createBlock(); - return buildHandle (typeID, newBlock.accessMemory(), &newBlock); + return buildHandle (typeID, asBuffer(newBlock.accessMemory()), &newBlock); } @@ -265,10 +274,11 @@ namespace engine { /** mark a buffer as officially discarded */ void - TrackingHeapBlockProvider::detachBuffer (HashVal typeID, LocalTag const& specifics) + TrackingHeapBlockProvider::detachBuffer (HashVal typeID, LocalTag const& specifics, Buff& storage) { diagn::Block* block4buffer = locateBlock (typeID, specifics); REQUIRE (block4buffer, "releasing a buffer not allocated through this provider"); + REQUIRE (util::isSameObject(storage, block4buffer->accessMemory())); block4buffer->markReleased(); } diff --git a/src/steam/engine/tracking-heap-block-provider.hpp b/src/steam/engine/tracking-heap-block-provider.hpp index f0e30671b..f46104b13 100644 --- a/src/steam/engine/tracking-heap-block-provider.hpp +++ b/src/steam/engine/tracking-heap-block-provider.hpp @@ -134,10 +134,10 @@ namespace engine { public: /* === BufferProvider interface === */ - virtual uint prepareBuffers (uint count, HashVal typeID); - virtual BuffHandle provideLockedBuffer (HashVal typeID); - virtual void mark_emitted (HashVal entryID, LocalTag const&); - virtual void detachBuffer (HashVal entryID, LocalTag const&); + virtual uint prepareBuffers (uint count, HashVal typeID) override; + virtual BuffHandle provideLockedBuffer (HashVal typeID) override; + virtual void mark_emitted (HashVal, LocalTag const&) override; + virtual void detachBuffer (HashVal, LocalTag const&, Buff&) override; public: TrackingHeapBlockProvider(); diff --git a/tests/core/steam/engine/buffer-metadata-test.cpp b/tests/core/steam/engine/buffer-metadata-test.cpp index 52cb796be..3ad3dca00 100644 --- a/tests/core/steam/engine/buffer-metadata-test.cpp +++ b/tests/core/steam/engine/buffer-metadata-test.cpp @@ -28,12 +28,10 @@ #include "lib/error.hpp" #include "lib/test/run.hpp" #include "lib/test/test-helper.hpp" -#include "lib/util.hpp" #include "steam/engine/buffer-metadata.hpp" #include "steam/engine/testframe.hpp" +#include "lib/util.hpp" -#include -#include #include using std::strncpy; @@ -54,15 +52,6 @@ namespace test { namespace { // Test fixture - const size_t TEST_MAX_SIZE = 1024 * 1024; - - const size_t SIZE_A = 1 + rand() % TEST_MAX_SIZE; - const size_t SIZE_B = 1 + rand() % TEST_MAX_SIZE; - - HashVal JUST_SOMETHING = 123; - void* const SOME_POINTER = &JUST_SOMETHING; - - template TY& accessAs (metadata::Entry& entry) @@ -71,6 +60,23 @@ namespace test { ASSERT (ptr); return *ptr; } + + template + metadata::Buff* + mark_as_Buffer(X& something) + { + return reinterpret_cast (std::addressof(something)); + } + + + const size_t TEST_MAX_SIZE = 1024 * 1024; + + const size_t SIZE_A = 1 + rand() % TEST_MAX_SIZE; + const size_t SIZE_B = 1 + rand() % TEST_MAX_SIZE; + + HashVal JUST_SOMETHING = 123; + auto SOME_POINTER = mark_as_Buffer(JUST_SOMETHING); + }//(End) Test fixture and helpers @@ -214,7 +220,7 @@ namespace test { // embedded into a Buffer Descriptor... // later, when it comes to actually *locking* those buffers... - typedef char RawBuffer[SIZE_B]; + using RawBuffer = std::byte; void* storage = malloc (2*SIZE_B); // do the necessary memory allocations behind the scenes... @@ -222,12 +228,12 @@ namespace test { TestFrame* frames = new TestFrame[3]; // a real-world BufferProvider would use some kind of allocator // track individual buffers by metadata entries - metadata::Entry& f0 = meta_->markLocked(bufferType1, &frames[0]); - metadata::Entry& f1 = meta_->markLocked(bufferType1, &frames[1]); - metadata::Entry& f2 = meta_->markLocked(bufferType1, &frames[2]); + metadata::Entry& f0 = meta_->markLocked(bufferType1, mark_as_Buffer(frames[0])); + metadata::Entry& f1 = meta_->markLocked(bufferType1, mark_as_Buffer(frames[1])); + metadata::Entry& f2 = meta_->markLocked(bufferType1, mark_as_Buffer(frames[2])); - metadata::Entry& r0 = meta_->markLocked(rawBuffType, &rawbuf[0]); - metadata::Entry& r1 = meta_->markLocked(rawBuffType, &rawbuf[1]); + metadata::Entry& r0 = meta_->markLocked(rawBuffType, mark_as_Buffer(rawbuf[ 0])); + metadata::Entry& r1 = meta_->markLocked(rawBuffType, mark_as_Buffer(rawbuf[SIZE_B])); CHECK (LOCKED == f0.state()); CHECK (LOCKED == f1.state()); @@ -241,12 +247,13 @@ namespace test { CHECK (transaction2 == r0.localTag()); CHECK (transaction2 == r1.localTag()); + auto adr =[](auto* x){ return reinterpret_cast(x); }; - CHECK (f0.access() == frames+0); - CHECK (f1.access() == frames+1); - CHECK (f2.access() == frames+2); - CHECK (r0.access() == rawbuf+0); - CHECK (r1.access() == rawbuf+1); + CHECK (adr(f0.access()) == adr(frames+0)); + CHECK (adr(f1.access()) == adr(frames+1)); + CHECK (adr(f2.access()) == adr(frames+2)); + CHECK (adr(r0.access()) == adr(rawbuf+0 )); + CHECK (adr(r1.access()) == adr(rawbuf+SIZE_B)); TestFrame defaultFrame; CHECK (defaultFrame == f0.access()); @@ -359,8 +366,8 @@ namespace test { VERIFY_ERROR (FATAL, entry.mark(NIL) ); // re-use buffer slot, start new lifecycle - void* OTHER_LOCATION = this; - entry.lock (OTHER_LOCATION); + auto* SOME_OTHER_LOCATION = mark_as_Buffer(*this); + entry.lock (SOME_OTHER_LOCATION); CHECK (LOCKED == entry.state()); CHECK (entry.isLocked()); @@ -373,7 +380,7 @@ namespace test { VERIFY_ERROR (FATAL, entry.mark(BLOCKED) ); VERIFY_ERROR (FATAL, entry.mark(NIL) ); - CHECK (OTHER_LOCATION == entry.access()); + CHECK (SOME_OTHER_LOCATION == entry.access()); entry.mark (FREE); CHECK (!entry.isLocked()); diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 4d4c28beb..f58d08831 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -4882,9 +4882,7 @@ - - - +

es wäre denkbar, an dieser Stelle @@ -4923,9 +4921,7 @@ - - - +

denn es erzwingt, @@ -4965,9 +4961,7 @@ - - - +

reine ID-Wirtschaft wäre möglich @@ -5502,9 +5496,7 @@ - - - +

...und Inkscape wird das erhalten, solange man die betr. Features nicht wieder aktiviert. Im Besonderen kann man @@ -6406,9 +6398,7 @@ - - - +

  • @@ -8003,9 +7993,7 @@ - - - +

    Innerhalb einer Kinder-Folge gibt es keine duplikaten IDs. @@ -10273,9 +10261,7 @@ - - - +

    ...das war nur ein unnötiger Fix nach dem Gießkannen-Prinzip. @@ -13652,9 +13638,7 @@ - - - +

    nur eingeschränkt auf die TypID? @@ -52461,9 +52445,7 @@ - - - +

    invocationTral wurde aufgegeben. @@ -52483,9 +52465,7 @@ - - - +

    allerdings, wenn man eine explizite Instanz-ID angibt, @@ -52503,9 +52483,7 @@ - - - +

    das heißt, es geht um die Haupt-Registry für Commands. @@ -88789,9 +88767,97 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
    ...und zwar je nachdem, in welcher Reihenfolge das LocalTag und die Buffer-Adresse gegeben waren: war das LocalTag zuerst da, dann würde das bereits Teil des Hash; wenn dann später jemand mit der gleichen Buffer-Adresse ankommt, aber ohne das LocalTag, dann wird der bestehende (schlimmstenfalls bereits gelockte) Eintrag nicht gefunden ⟹ es wird ein neuer Eintrag erzeugt und somit zweimal gelockt.

    +     + + + + + + + + + + + + + + + + +

    + ...da mit jedem Buffer-Typ auch eine bestimmte Memory-Block-Größe verbunden ist, könnte man dahinter mehrere kachelnde Basis-Allokatoren anordnen — und in einer solchen Struktur wäre es sinnvoll, zu erwartende Buffer-Allokationen im Voraus anzukündigen. Die Steuerung der Pool-Größe könnte dann im Hintergrund erfolgen, was möglicherweise eine relevante Optimierung auf Durchsatz ermöglicht +

    +     +     + + + + + + + + + +

    + anhand der Hash-ID, die zum Entry gehört, welcher beim Erzeugen des Handles gespeichert wurde +

    + + +     +     + + + + + + + + + + + +

    + der Entry wird aus der Metadaten-Tabelle gelöscht +

    + + +     +     +     +     + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +