From ccb10f3c65c3ce45ceae8fa4f73123f4735a8fce Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Mon, 10 Feb 2025 03:15:28 +0100 Subject: [PATCH] Invocation: chase down insidious use-after-free problem MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Just wanted to use a helper function to build a source-data node. However, the resulting node had a corrupted Node-ID spec. Investigation with the debugger showed that the ID was still valid while in construction and shows up corrupted after returning from the helper function. As it turned out, the reason is related to the de-duplication of ProcID data. While the de-duplicated strings themselves are ''not'' affected, the corruption happened by an intermediate instance of ProcID, which was inadvertently created and bound by-value to the builder-λ. The created Port then picks up a reference to this temporary, leading to the use-after-free of the string_view obejcts. Obviously, `ProcID` must not be instantiated other than through the static front-end `ProcID::describe`. Due to the private constructor, I can not make this object non-copyable (because then the hash-set would not be allowed to emplace it). But making it at least move-only will provoke a compiler error whenever binding to a lambda capture by value, which hopefully helps to pinpoint this insidious problem in the future... --- src/steam/engine/node-builder.hpp | 2 +- src/steam/engine/proc-id.hpp | 5 +++ src/steam/engine/proc-node.cpp | 2 +- src/steam/engine/weaving-pattern-builder.hpp | 2 +- tests/core/steam/engine/node-devel-test.cpp | 7 ++++ wiki/thinkPad.ichthyo.mm | 44 +++++++++++++++++++- 6 files changed, 57 insertions(+), 5 deletions(-) diff --git a/src/steam/engine/node-builder.hpp b/src/steam/engine/node-builder.hpp index a34f30528..df24d6906 100644 --- a/src/steam/engine/node-builder.hpp +++ b/src/steam/engine/node-builder.hpp @@ -667,7 +667,7 @@ namespace engine { return NodeBuilder ( static_cast&&> (*this) // slice away PortBulder subclass data , SizMark{} ,// prepare a builder-λ to construct the actual Turnout-object - [procID = ProcID::describe(_Par::symbol_,portSpec,flags) + [&procID = ProcID::describe(_Par::symbol_,portSpec,flags) ,builder = move(blockBuilder_) ,postProc = move(postProcessor_) ,delegate = delegatePort_ diff --git a/src/steam/engine/proc-id.hpp b/src/steam/engine/proc-id.hpp index 5be74e96e..306d547d1 100644 --- a/src/steam/engine/proc-id.hpp +++ b/src/steam/engine/proc-id.hpp @@ -66,6 +66,7 @@ #include "lib/error.hpp" #include "lib/hash-standard.hpp" #include "lib/several.hpp" +#include "lib/nocopy.hpp" #include #include @@ -114,8 +115,12 @@ namespace engine { * @note must be essentially immutable; should ensure that implementation * never changes anything constituent for the \ref hash_value(), * due to de-duplication into a hashtable (see proc-node.cpp). + * @warning be sure always to take a reference to the instance emplaced + * into the `procRegistry` (see proc-node.cpp); inadvertently taking + * a reference to some transient ProcID value leads to insidious errors! */ class ProcID + : util::MoveOnly // ◁—— you must not create instances, use ProcID::describe() { StrView nodeName_; StrView portQual_; diff --git a/src/steam/engine/proc-node.cpp b/src/steam/engine/proc-node.cpp index d9b90aa14..d81bd1170 100644 --- a/src/steam/engine/proc-node.cpp +++ b/src/steam/engine/proc-node.cpp @@ -201,7 +201,7 @@ namespace engine { "Node:%s Spec:%s"} % nodeSymb % portSpec }; - auto res = procRegistry.insert (ProcID{nodeSymb, portSpec.substr(0,p), portSpec.substr(p), extAttrib}); + auto res = procRegistry.emplace (ProcID{nodeSymb, portSpec.substr(0,p), portSpec.substr(p), extAttrib}); ProcID& entry{unConst (*res.first)}; if (res.second) {// new record placed into the registry diff --git a/src/steam/engine/weaving-pattern-builder.hpp b/src/steam/engine/weaving-pattern-builder.hpp index b1885b592..8cbeb2fb0 100644 --- a/src/steam/engine/weaving-pattern-builder.hpp +++ b/src/steam/engine/weaving-pattern-builder.hpp @@ -375,7 +375,7 @@ namespace engine { ,types = move(outTypes.build()) ,prototype = move(prototype_) ,resultIdx = resultSlot - ,procID = ProcID::describe (nodeSymb_,portSpec_) + ,&procID = ProcID::describe (nodeSymb_,portSpec_) ] (PortDataBuilder& portData) mutable -> void { diff --git a/tests/core/steam/engine/node-devel-test.cpp b/tests/core/steam/engine/node-devel-test.cpp index 5fce7ff7f..d7e207d12 100644 --- a/tests/core/steam/engine/node-devel-test.cpp +++ b/tests/core/steam/engine/node-devel-test.cpp @@ -338,12 +338,16 @@ namespace test { makeSrcNode (ont::FraNo frameNr, ont::Flavr flavour) { auto spec = testRand().setupGenerator(); +SHOW_EXPR(spec.nodeID()) return prepareNode(spec.nodeID()) +// ProcNode n{prepareNode(spec.nodeID()) .preparePort() .invoke(spec.procID(), spec.makeFun()) .setParam(frameNr,flavour) .completePort() .build(); +//SHOW_EXPR(watch(n).getNodeName() ); +// return move(n); } @@ -386,6 +390,9 @@ namespace test { // Build a node using this processing-functor... ProcNode nSrc = makeSrcNode (frameNr,flavour); +SHOW_EXPR(watch(nSrc).getNodeName() ); + ProcID& px = ProcID::describe("Test:generate","(TestFrame)"); +SHOW_EXPR(px.genNodeName()) ProcNode nFilt{prepareNode(spec.nodeID()) .preparePort() .invoke(spec.procID(), procFun) diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index d9c0702c9..3cbe625f8 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -105236,9 +105236,34 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) + + + + + + - - + + + + + + +

+ Puh... +

+

+ ...endlos mit dem Debugger beobachtet, sieht immer alles völlig sauber aus; konnte schließlich belegen daß es nicht die de-duplzierten Strings selber sind ⟹ das hat mich dann auf die richtige Fährte gebracht: es muß einer der dazwischen liegenden String-Views sein, der in transientem Speicher liegt — und tatsächlich: in den Builder-λ erzeugen wir eine Proc-ID per Value, binden sie dann aber per Referenz in den neuen Port.... +

+ + + + + + + + + @@ -105821,6 +105846,21 @@ StM_bind(Builder<R1> b1, Extension<R1,R2> extension) + + + + + + + + + + + + + + +