Issue 2282: PatternLanuage changes: [Bug] Crash when hovering on a static array

[shewitt-au]

This is an attempt at a fix for issue 2282: [Bug] Crash when hovering on a static array

There are changes to both ImHex and PatternLanguage.
Client changes here

Initial debugging suggested a use after free in the parent chain of patterns (0xbaadf00d).

I replaced Pattern’s parent pointer with a shared_ptr. I had to perform CPR (Compile-Patch-Repeat. I just made that up. I dare you to look at me with a straight face and tell me it’s not funny) numerous times. I was slowed down by the time it takes to compile ImHex with even modest changes. I think there is way too much implementation in header files. But I digress.

Along the way I fixed other issued as I encountered them. Of note:

• shared_prts initialized with unique_ptrs
• cloning of objects used in std::sort, changing the objects during sorting
• patterns newed but not shared_ptred

I think that it was basically a memory management issue.

All that said, I don’t know the code well, and I made heaps of changes. It’s possible I’ve fixed something and broken other stuff. My money is on something being broken by this.

There are some failed checks.

Oh yeah, there's a const_cast. I'll look into getting rid of that. The compiler was fighting back and I was getting over it.

[rsp4jack]

this is the backtrace:

#0  0x00007ffff6cb1e9a in __cxxabiv1::__cxa_throw (obj=0x7fff84473b10, tinfo=0x7ffff6e86388 <typeinfo for std::bad_weak_ptr>, 
    dest=0x7ffff6ce4740 <std::bad_weak_ptr::~bad_weak_ptr()>) at /usr/src/debug/gcc/gcc/libstdc++-v3/libsupc++/eh_throw.cc:81
#1  0x00007ffff7ae4a46 in __wrap___cxa_throw (thrownException=0x7fff84473b10, type=0x7ffff6e86388 <typeinfo for std::bad_weak_ptr>, 
    destructor=0x7ffff6ce4740 <std::bad_weak_ptr::~bad_weak_ptr()>) at /home/rsp4jack/workspace/ImHex/lib/trace/source/exceptions.cpp:25
#2  0x00007ffff738e6fb in std::__throw_bad_weak_ptr () at /usr/include/c++/15.1.1/bits/shared_ptr_base.h:95
#3  0x00007ffff74a1250 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count (this=<optimized out>, __r=...)
    at /usr/include/c++/15.1.1/bits/shared_ptr_base.h:1246
#4  std::__shared_ptr<pl::ptrn::Pattern, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<pl::ptrn::Pattern, void> (this=<optimized out>, __r=...)
    at /usr/include/c++/15.1.1/bits/shared_ptr_base.h:1555
#5  std::shared_ptr<pl::ptrn::Pattern>::shared_ptr<pl::ptrn::Pattern, void> (this=<optimized out>, __r=std::weak_ptr<pl::ptrn::Pattern> (empty) = {...})
    at /usr/include/c++/15.1.1/bits/shared_ptr.h:380
#6  std::enable_shared_from_this<pl::ptrn::Pattern>::shared_from_this (this=0x7fff8446fdd8) at /usr/include/c++/15.1.1/bits/shared_ptr.h:934
#7  pl::ptrn::Pattern::reference (this=0x7fff8446fdd0) at /home/rsp4jack/workspace/ImHex/lib/external/pattern_language/lib/include/pl/patterns/pattern.hpp:119
#8  pl::ptrn::PatternStruct::PatternStruct (this=0x7fff8446fdd0, other=...)
    at /home/rsp4jack/workspace/ImHex/lib/external/pattern_language/lib/include/pl/patterns/pattern_struct.hpp:18
#9  0x00007ffff74a12e8 in pl::ptrn::PatternStruct::clone (this=0x7fff844737a0)
    at /home/rsp4jack/workspace/ImHex/lib/external/pattern_language/lib/include/pl/patterns/pattern_struct.hpp:25
#10 0x00007ffff7424da9 in pl::core::ast::ASTNodeArrayVariableDecl::createStaticArray (this=this@entry=0x7fff840f3020, evaluator=evaluator@entry=0x5555568a38b0, 
    outputPattern=std::shared_ptr<pl::ptrn::Pattern> (empty) = {...})
    at /home/rsp4jack/workspace/ImHex/lib/external/pattern_language/lib/source/pl/core/ast/ast_node_array_variable_decl.cpp:228
#11 0x00007ffff7425305 in pl::core::ast::ASTNodeArrayVariableDecl::createPatterns (this=<optimized out>, evaluator=0x5555568a38b0, 
    resultPatterns=std::vector of length 1, capacity 1 = {...})
    at /home/rsp4jack/workspace/ImHex/lib/external/pattern_language/lib/source/pl/core/ast/ast_node_array_variable_decl.cpp:91
#12 0x00007ffff7481a87 in pl::core::ast::ASTNodeConditionalStatement::createPatterns (this=<optimized out>, evaluator=0x5555568a38b0)
    at /home/rsp4jack/workspace/ImHex/lib/external/pattern_language/lib/source/pl/core/ast/ast_node_conditional_statement.cpp:31
#13 0x00007ffff7481a87 in pl::core::ast::ASTNodeConditionalStatement::createPatterns (this=<optimized out>, evaluator=0x5555568a38b0)
    at /home/rsp4jack/workspace/ImHex/lib/external/pattern_language/lib/source/pl/core/ast/ast_node_conditional_statement.cpp:31
#14 0x00007ffff7481a87 in pl::core::ast::ASTNodeConditionalStatement::createPatterns (this=<optimized out>, evaluator=0x5555568a38b0)
    at /home/rsp4jack/workspace/ImHex/lib/external/pattern_language/lib/source/pl/core/ast/ast_node_conditional_statement.cpp:31
#15 0x00007ffff7481a87 in pl::core::ast::ASTNodeConditionalStatement::createPatterns (this=<optimized out>, evaluator=0x5555568a38b0)
    at /home/rsp4jack/workspace/ImHex/lib/external/pattern_language/lib/source/pl/core/ast/ast_node_conditional_statement.cpp:31
#16 0x00007ffff7481a87 in pl::core::ast::ASTNodeConditionalStatement::createPatterns (this=<optimized out>, evaluator=0x5555568a38b0)
    at /home/rsp4jack/workspace/ImHex/lib/external/pattern_language/lib/source/pl/core/ast/ast_node_conditional_statement.cpp:31
#17 0x00007ffff7481a87 in pl::core::ast::ASTNodeConditionalStatement::createPatterns (this=<optimized out>, evaluator=0x5555568a38b0)
    at /home/rsp4jack/workspace/ImHex/lib/external/pattern_language/lib/source/pl/core/ast/ast_node_conditional_statement.cpp:31
#18 0x00007ffff7481a87 in pl::core::ast::ASTNodeConditionalStatement::createPatterns (this=<optimized out>, evaluator=0x5555568a38b0)
    at /home/rsp4jack/workspace/ImHex/lib/external/pattern_language/lib/source/pl/core/ast/ast_node_conditional_statement.cpp:31

[paxcut]

One simple way to create the problem

#pragma example 00 00 00 00
import std.mem;

std::mem::Bytes<4> b@0;

The pattern will run but the value shown in the pattern data window is bad_weak_ptr

[shewitt-au]

This is a bind... I'll have to think a little on this.

[shewitt-au]

This is a serious issue. Basically we can't call std::shared_from_this from constructors (or any methods called from them). When I suggested std::shared_from_this here I was using a different method which was my preference. Perhaps it should be revisited..

[paxcut]

why cant you call make_shared() to create the initial shared pointer?

[shewitt-au]

As I understand it, even if we used std::make_shared, std::shared_from_this still requires a constructed object. std::make_shared is more efficient as the control block and the object are enplaced in a single allocation. And it's more exception safe, but I'm afraid of no help for this issue.

[rsp4jack]

@shewitt-au How about this? It should avoid the problem. There may not be a suitable constructor or copy assignment, if so, just create one.

[[nodiscard]] std::shared_ptr<Pattern> clone() const override {
    auto result = std::make_shared<PatternStruct>(<something>);
    *result = *this;
    return std::static_pointer_cast<Pattern>(result);
}

[shewitt-au]

I don't think it will help I'm afraid. The problem is that if <something>'s constructor uses std::shared_from_this, directly or indirectly, we're in trouble because the std::shared_ptr hasn't taken ownership yet.

[paxcut]

constructors cannot call shared_from_this()

[rsp4jack]

if 's constructor uses std::shared_from_this

Actually I want to construct an instance of PatternStruct using a constructor which does nothing. If there isn't one, Just create it. This is a member function, so the new constructor could be private.

[shewitt-au]

@rsp4jack Yeah, something like that would seem to be the way to go. Make the constructors trivial and private and move construction to factory methods.

[paxcut]

From what I read it seems that in order to use shared_from_this the object in question must already have been used to create a shared pointer using any of the methods available to do so. Once this initial pointer exists, further instances can be obtained using shared_from_this().

It also seems to me that we could set a member flag that indicates if a shared pointer exists for the object in question initially set to false. Before calling shared_from_this() the flag is checked and if false then the shared pointer object is created and the flag is set to true. This way the pointers are only created when needed. I am probably over simplifying things. this is my first experience with std shared from this

[rsp4jack]

It also seems to me that we could set a member flag that indicates if a shared pointer exists

No need. Just check whether std::enable_shared_from_this::weak_from_this is expired (non null in this case).

[rsp4jack]

I just think up a scenario (seems rare):

auto* m_pattern = new PatternFooBar{*pattern_with_many_children};
// now m_pattern's children have shared_ptr pointing to m_pattern

and...

// kill all the children!
kill_children(m_pattern);

Then m_pattern is gone! (because children are the only owners of m_pattern)

So, since children should not exist if their parent isn't exist, why a pattern must hold a shared_ptr pointing to its parent? A weak_ptr is enough (or even simpler, a bare pointer). Maybe there are some problems behind the scene, where a pattern dies (and why it dies?) with its children still alive.

[shewitt-au]

I am trying out a two stage construction approach. Will take a little time. Small changes all over the place. I'll let you know how it turns out.

[rsp4jack]

I just run the initial pattern (where I found the problem and created WerWolv/ImHex#2282) and there is no problem.

Current code looks well. The construct_shared_object and post_construct is good, I think this is probably the best solution for now to fix this bug.

IMO This PR is almost done. Thanks for your effort in it.

[shewitt-au]

@paxcut

Re the article you provided:

template<typename... T>
static std::shared_ptr<SharedThing> create(T&&... t)
{
    // C++ core guidelines say we shouldn't do this:
    //return std::shared_ptr<SharedThing>(new SharedThing(std::forward<T>(t)...));

    // But this doesn't work because our constructor is private, and we want that
    // for use with enable_shared_from_this
    //return std::make_shared<SharedThing>(std::forward<T>(t)...);

    // So we'll make a "fake" derived class that constructs our shared thing
    // using a public constructor...
    struct EnableMakeShared : public SharedThing {
        EnableMakeShared(T&&... arg) : SharedThing(std::forward<T>(arg)...) {}
    };

    // and use that type with make_shared
    return std::make_shared<EnableMakeShared>(std::forward<T>(t)...);
}

One initial problem occurs to me. The class will now be a EnableMakeShared. Would this make debugging confusing for example? I'm not sure.

Another one, which I don't think would be of real concern, is that it would not work on final classes. Although, like I've done in my code as it stands, templates could probably we used to select an alternate impementation in this case.

I think I'll have to experiment a bit with this and see how it goes.

[shewitt-au]

I think I've found an even better solution while reseaching my concerns above. It can be found here. The chain of authors goes back. The version linked (C++23) seems sound and would result in a single allocation for the object and the control block.

class safe_enable_shared_from_this : 
    public std::enable_shared_from_this<safe_enable_shared_from_this> 
{
protected:
    safe_enable_shared_from_this() noexcept = default;
    safe_enable_shared_from_this(safe_enable_shared_from_this&&) noexcept = default;
    safe_enable_shared_from_this(const safe_enable_shared_from_this&) noexcept = delete;
    safe_enable_shared_from_this& operator=(safe_enable_shared_from_this&&) noexcept = default;
    safe_enable_shared_from_this& operator=(const safe_enable_shared_from_this&) noexcept = delete;

public:
    virtual ~safe_enable_shared_from_this() noexcept = default;

protected:
    template <typename T>
    struct Allocator : public std::allocator<T>
    {  
        template<typename TParent, typename... TArgs>
        void construct(TParent* parent, TArgs&&... args) { 
            ::new((void *)parent) TParent(std::forward<TArgs>(args)...);
        }
    };
    
    template <typename T, typename... TArgs>
    static inline auto create(TArgs&&... args) -> ::std::shared_ptr<T> 
    {
        return std::allocate_shared<T>(Allocator<T>{}, std::forward<TArgs>(args)...);
    }

public:
    template <typename TSelf>
    auto inline shared_from_this(this TSelf&& self) noexcept
    {
        return std::static_pointer_cast<std::remove_reference_t<TSelf>>(
            std::forward<TSelf>(self).std::template enable_shared_from_this<safe_enable_shared_from_this>::shared_from_this());
    }
    
    template <typename TSelf>
    auto inline weak_from_this(this TSelf&& self) noexcept -> ::std::weak_ptr<std::remove_reference_t<TSelf>>
    {
        return std::static_pointer_cast<std::remove_reference_t<TSelf>>(
            std::forward<TSelf>(self).std::template enable_shared_from_this<safe_enable_shared_from_this>::weak_from_this().lock());
    }
};

I'm going to code it up and see how it goes.

[paxcut]

I went back to master code and looked into why the errors occur. There are four examples that generate errors.

1. static array of bitfields,
2. json variable used as reference and
3. custom inspector rows
4. exporting patterns.

There are four classes derived from Pattern that have non-trivial copy constructors. Although there are calls to shared_from_this() inside them, they are never the ones that belong to the this pointer. They are called for members, for parents, for array elements, etc.. and they are always called post construction.
So while it is constructing the pattern for an array, the patterns for all the elements of the array were already constructed previously and it is safe to call shared_from_this for them. There isn't a single case of shared_from_this being called from its own constructor. The major problem with the way the copy constructors are setup currently is that it is very easy to go into infinite loops that overflows the stack.

One important fact is that all the copy constructors are called from clone and only from clone, so instead of creating new functions, I have moved all the code that was on the copy constructors onto clone to avoid infinite loops.

All the errors in master occur because either unique pointers are being used instead of shared or because m_parent was assigned a raw pointer and never turned into a shared pointer before shared_from_this is called or because of infinite loops.

Starting from master i added code to reference() that throws an error if the weak pointer is expired. that finds all the cases when shared pointers need to be created.
I have fixed all the cases related to the four examples above and all the code runs just as before except for maybe the json example where the reference use zeroes it out which occurs in your version as well. I need to investigate further on that.

Sorry about the long post. hopefully it will be my last here.

[shewitt-au]

@paxcut I had a look at your changes.

std::shared_ptr<Pattern> reference() {
    auto weakPtr = weak_from_this();
        if (weakPtr.expired()) {
            core::err::E0001.throwError("Cannot call shared_from_this if this is not shared.");
        }
           
    return shared_from_this();
}

This is just obviously a good idea.

Your code certainly has the advantage of simplicity. There's no extra code like construct_shared_object.hpp in my version.

Many of the fixes are like my code, but instead of moving the code into post_construct it's been moved to clone. You've used two-stage construction implictly instead of explictly, from the perspective of how the code was before the changes. My PR has the advantage of the intent being explicit. If you see a post_construct method you know that the code in it, in a more perfect world, would be in a constructor (and in fact used to be), but can't be because of the limitations of std::enable_shared_from_this. Although I don't want to overstate this: it is fairly obvious that a clone method constructs an object.

There is nothing to stop misuse however. If copy constructors are only used from clone it could be made less error prone by making the copy constructor protected, but then std::make_shared could not be used. If someone copy constructs an object without using clone they no longer get a fully constructed object, much of the the code has been removed from the constructor and into clone. This is the case with my code too (although all my constructors are protected, so the scope of exposed code is reduced), but the presence of a post_construct method makes this explicit.

I have updated my code in response to our conversations. Although we may have butted heads at times, my PR, regardless whether it is adopted or not, is better as a result, and the journey has taught me stuff I didn't know before it.

All the constructors are protected; abuse can only happen from within. The code in construct_shared_object.hpp has been updated so that, even though the constructors are non-public, only a single allocation is used (like with std::make_shared with public constructors). When the shared_from_this or weak_from_this methods are called from derived classes the smart pointers returned now wrap the derived type.

I understand the simplicity argument. And it has merit. My perspective is that if mistakes are harder to make, less mistakes will be made. The bugs fixed by both our verisions of this code were caused by errors that if could have been caught at compile-time, or not introducted because of carity of intent in the code, would not have existed in the first place, and there is no resason to assume that will not be the case in the future.

Anyway, that's it for me on this one. It's out of my hands now. All the cards have been layed on the table.

[paxcut]

I understand all the arguments and the reasons behind the choices made, but there are also arguments and reasons that call for swift action. Maybe this PR started as an attempt to fix issue #2282 but as it stands now it is much more than that. It encompasses a lot more than just fixing the bug, things like safety, error prevention and much more. Although it is true that it fixes the issue it has many goals that are not related to the issue.
Complex refactors of entire subsystems which is what this PR has become, need to wait for the author to approve them. Bugs fixes that are simple and easy to understand don't need to wait. You can keep working on making the pattern creation safer and simpler while you wait for review, but the bug needs to be fixed asap so I am merging the simple changes after doing some more testing to minimize the introduction of new bugs and to verify that the known bugs are fixed. Once merged into master, you will need to merge any conflict that the two sets of changes may incur, but in this case I think that should be a pretty easy task for you and I am always available to help if I can.
The exchanges we have had have taught me a great deal about smart pointers. Maybe I didn't contribute much but it wasn't for lack of effort. I am looking forward to working on new projects in the future.

[shewitt-au]

I understand. Although personally I feel my changes are safer than the alternative, I don't blame you for being skeptical.

Maybe I didn't contribute much

Our conversations led me to the discovery of safe_enable_shared_from_this. It's just a better verison of std::enable_shared_from_this. Full stop. That on its own makes me value your contributions.

[paxcut]

Let me be 100% clear about this. Your changes are obviously safer than mine or the code that was implemented before. It isn't even debatable that it is obvious. That being said, I fail to see how that has any relevance on what needs to be done though. The bug needs to be fixed now but the code doesn't need to be made safer now. It can and it will be made safer later.

[shewitt-au]

@paxcut

Let me be 100% clear about this. Your changes are obviously safer than mine or the code that was implemented before. It isn't even debatable that it is obvious. That being said, I fail to see how that has any relevance on what needs to be done though. The bug needs to be fixed now but the code doesn't need to be made safer now. It can and it will be made safer later.

I already said I understood your skepticism. And if you want to get a fix in as a matter of urgency (which feels safer) I understand that too. But since we're talking about it I'll elaborate on the reasons I went to the effort to make the code safer. To make the kind of mistakes that caused these bugs harder to make in future.

The total entropy of an isolated system always increases

— 2nd law of thermodynamics

It takes effort to keep anything ordered, software being no exception. It's easy to make rapid progress at the expense of long term progress and maintainability. With your changes PatternArrayDynamic, PatternArrayStatic and PatternStruct (from memory) now have (publicly accessbile) broken copy constructors. Too me it feels like fixing the bug, not the code.

Everything should be made as simple as possible, but not simpler

— Einstein

OK. I've satiated the urge to quote. Like I said earlier, I understand that in software development sometimes you need to get fixes out quickly and that may involve suboptimal code. I accept that pragmatism is a virtue.

[paxcut]

I understand that in software development sometimes you need to get fixes out quickly and that may involve suboptimal code. I accept that pragmatism is a virtue.

As long as you understand I won't feel the need to explain things again and I hope that you don't feel the need of having to explain things to ma that I already know either. If you do, I am afraid I will have to explain myself again despite your claims of understanding.

[shewitt-au]

@paxcut

It's not like you beat around the bush. You know I think my code is safer. And you've said you do too. What more would I say? You're immovable, I'm immovable, and I've got no control over the situation.

[paxcut]

@paxcut

It's not like you beat around the bush. You know I think my code is safer. And you've said you do too. What more would I say? You're immovable, I'm immovable, and I've got no control over the situation.

How am I beating around the bush? I have said all I needed to say and you claim to understand it all. You can say anything thats on your mind. Most of the things that affect us escape our control. When we think we have control we are only fooling ourselves.
I make lots of mistakes and I always make sure to dwell on them in order to learn. I have a very open mind and more patience than anybody I know. Do you think of yourself as immovable and is that a good thing?

[shewitt-au]

How am I beating around the bush?

What I meant is that you're not beating around the bush. Sorry if that wasn't clear. I meant that you have made your stance clear.

[shewitt-au]

Do you think of yourself as immovable and is that a good thing?

I meant on this issue. That we both have firm opinions on this PR. And that's fine.

[paxcut]

Lies, lies and more lies. There is no code in that pr that comes from here. Read this post here carefully

[paxcut]

I dont expect you to apologize but have the decency to respond to the evidence provided against your claims. Not only there is a post informing you that the code in my pr was written based on master and not the code of this pr I modified but there are changes that exists in your code that my pr maintains the original master code instead. That is conclusive evidence that your accusations are flawed. If this was a court of law I would have the right to sue you for libel because I have proven my innocence. You , on the other hand, cannot claim to be innocent. You ignored my evidence willingly and knowingly. It makes me wonder why you would go to such extremes and clearly shows to everybody that you can't be trusted.

[paxcut]

The main point of contention, which persists to this day, is that bugs were not created by calling shared_from_this from the constructors. In my investigation on the subject I found no cases where clone() or reference() are called using the object being constructed as the source.

Since the problem is not shared_from _this in constructors the only thing left is m_parent. To fix m_parent we need to make it a weak pointer. that is the only sensible choice. That brings about tons of changes in the code that are mandatory to even compile. After some more testing there are unique pointers where shared pointers need to be a couple of pointers conversions are needed instead of dynamic casts and that's about it. thats the pr. It will obviously look a lot like a subset of your code but it wasn't obtained from it, using it, remembering things about it or any source of information that originated from you.

As I understand by reading your comments, you still blame the bugs on the constructor non trivial bodies and that means that we have a different understanding on the nature of the bugs. There is no discussions about this in the posts. only on things related to shared_from_this that are not related to the bugs like making things safer and more robust.

Trying to obtain some credit requires at minimum some tact. You approach the subject using inquires and feeling your way through. Instead you asserted things that were not true and demanded actions that were not deserved and you still show no interest in my findings on the origin of the bugs.

Sadly this all could have been avoided if you had written the code for it like I originally intended to. Then you would have gotten all the credit and we would have a successful and fruitful collaboration instead.It would have been faster and the bugs would be history by now.

I couldn't express the need to do this work to you is a way that motivated you to get started. I couldn't use your code because there are too many changes and is not clear to me what each change does. I do blame myself for what has come to pass but I feel that things could have gone better somehow although I have no clue as to what that could be.

It isnt a personal attack. It is the natural result of your continued line of accusations while not commenting on the evidence shown against the accusations. If you accuse anybody without just cause they will have a hard time trusting you. You wouldn't trust somebody that is accusing you of something you havent done. It isn't personal, it is only natural.

We need to move on. this is really exhausting.

[shewitt-au]

@paxcut

I'm not even going to bother commenting. This is just too draining, and someone has to break the cycle. I will delete my posts as not to make your PR look like a bar fight. I will not respond, in any way, to any more correspondence on this issue. Don't mistake this for me thinking that you shouldn't have attributed appropriately (as I see it).

After I've deleted my post yours will have no context. Deal with that as you will.

[paxcut]

Credit needs to be based on contributions. what part of this pr did you contribute to and when did you contributed to it? . I have explained code similarity already but you just repeat the demand while ignoring my remarks. And thats after saying that you are not responding to any more posts. So you demand without listening and refuse to reply to any further posts on the subject. You want me to shut up and obey? What we need is not demands or denials we need compromise. exchange what you want with what you can offer for what I want and what I can offer. Not replying and issuing ultimatums cannot achieve compromise.