Skip to content

Commit 55b67de

Browse files
authored
fix: fixes for various errors related to shared_from_this. (WerWolv#171)
* fix: fixes for various errors related to shared_from_this. A while back there were some changes to the pattern language library that changed the way shared_pointers are created using shared_from_this(). Unfortunatelly the changes were not complete and various bugs were created among them 2234, json type not working, unable to export files, static arrays of bitfields,... The cause of the errors was that in class Pattern the member m_parent was left as a raw pointer and it needs to be handled by shared pointers. Also there were some cases in which share pointers were needed but unique pointers were used instead. Both cause crashes when shared_from_this is used on pointers that are not managed by shared_ptr. Another source of errors were infinite loops of clone and reference that caused stack overflow. The fixes include making m_parent a weak pointer, turning unique pointers into shared pointers and moving codefrom the copy constructors into clone to break the infinite loops.These changes are the bare minimum needed to bring the pattern language back to the full functionality that it had before shared_from_this was introduced or at least thats the hope. * build: unit tests * I got tired of not knowing what was broken, so I went back to master and looked at the various bugs under the debugger and found two types of problems that made ImHex crash. The bug reported in the issue is created because enable_shared_from_this seems to affect the dynamic cast convertion. I was able to patch both places where the error occurs (one for data window and other for the imhex tooltip) For the other type of problems I turned reference() into a detection tool for invalid uses of shared_from_this and ran about a dozen of patterns and found that there is only one line on each constructor that causes crashes. The constructor can call shared_from_this on any object except the ones that detect and store the parent reference on each of the children. typically they are member->setParent(reference()) or something similar to that. Only about a dozen or so changes to 6 files in 1 repository are needed to fix all the bugs created by the introduction of shared_from_this in the code. Pinpointing the source of the errors allows us also to avoid moving code out of the constructors that doesn't need to be moved which in turn makes it safe to call constructors with the understanding that references to the childrens parent will not be created automatically and needs to be done after construction (most likely in clone()). We obtain the same level of safety and prevent adding bugs in the future with only a fraction of the changes which increases code stability and also reduces the risk of adding new bugs that always comes with massive changes to the code base. * build: unit tests * Needed to set m_parent to weak_ptr in order to fix problems with dynamic cast. Also json types needed to be set to shared_ptrs instead of unique. * build: unit tests * build: unit tests * build: unit tests * unique ptr that should have been shared. * undoing unneeded changes * build: unit tests
1 parent 7a741ce commit 55b67de

12 files changed

Lines changed: 60 additions & 42 deletions

lib/include/pl/api.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ namespace pl::api {
126126
/**
127127
* @brief A function callback called when a custom built-in type is being instantiated
128128
*/
129-
using TypeCallback = std::function<std::unique_ptr<ptrn::Pattern>(core::Evaluator *, const std::vector<core::Token::Literal> &)>;
129+
using TypeCallback = std::function<std::shared_ptr<ptrn::Pattern>(core::Evaluator *, const std::vector<core::Token::Literal> &)>;
130130

131131
/**
132132
* @brief A type representing a function.

lib/include/pl/patterns/pattern.hpp

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,13 @@ namespace pl::ptrn {
116116
}
117117

118118
virtual std::shared_ptr<Pattern> clone() const = 0;
119-
std::shared_ptr<Pattern> reference() { return shared_from_this(); }
119+
std::shared_ptr<Pattern> reference() {
120+
auto weakPtr = weak_from_this();
121+
if (weakPtr.expired()) {
122+
core::err::E0001.throwError("Cannot call shared_from_this if this is not shared.");
123+
}
124+
return shared_from_this();
125+
}
120126

121127
[[nodiscard]] u64 getOffset() const { return this->m_offset; }
122128
[[nodiscard]] virtual u128 getOffsetForSorting() const { return this->getOffset() << 3; }
@@ -538,14 +544,14 @@ namespace pl::ptrn {
538544
}
539545

540546
[[nodiscard]] const Pattern* getParent() const {
541-
return m_parent;
547+
return m_parent.lock().get();
542548
}
543549

544550
[[nodiscard]] Pattern* getParent() {
545-
return m_parent;
551+
return m_parent.lock().get();
546552
}
547553

548-
void setParent(Pattern *parent) {
554+
void setParent(std::shared_ptr<Pattern> parent) {
549555
m_parent = parent;
550556
}
551557

@@ -623,7 +629,7 @@ namespace pl::ptrn {
623629
core::Evaluator *m_evaluator;
624630

625631
std::unique_ptr<std::map<std::string, std::vector<core::Token::Literal>>> m_attributes;
626-
Pattern *m_parent = nullptr;
632+
std::weak_ptr<Pattern> m_parent;
627633
u32 m_line = 0;
628634

629635
std::set<std::string>::const_iterator m_variableName;

lib/include/pl/patterns/pattern_array_dynamic.hpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,11 @@ namespace pl::ptrn {
2020
}
2121

2222
[[nodiscard]] std::shared_ptr<Pattern> clone() const override {
23-
return std::unique_ptr<Pattern>(new PatternArrayDynamic(*this));
23+
auto other = std::make_shared<PatternArrayDynamic>(*this);
24+
for (const auto &entry : other->m_entries)
25+
entry->setParent(other->reference());
26+
27+
return other;
2428
}
2529

2630
void setColor(u32 color) override {
@@ -133,7 +137,6 @@ namespace pl::ptrn {
133137

134138
if (!entry->hasOverriddenColor())
135139
entry->setBaseColor(this->getColor());
136-
entry->setParent(this);
137140

138141
this->m_entries.emplace_back(entry);
139142
}

lib/include/pl/patterns/pattern_array_static.hpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@ namespace pl::ptrn {
1616
}
1717

1818
[[nodiscard]] std::shared_ptr<Pattern> clone() const override {
19-
return std::unique_ptr<Pattern>(new PatternArrayStatic(*this));
19+
auto other = std::make_shared<PatternArrayStatic>(*this);
20+
other->m_template->setParent(other->reference());
21+
return other;
2022
}
2123

2224
[[nodiscard]] std::shared_ptr<Pattern> getEntry(size_t index) const override {
@@ -153,7 +155,6 @@ namespace pl::ptrn {
153155

154156
void setEntries(std::shared_ptr<Pattern> &&templatePattern, size_t count) {
155157
this->m_template = std::move(templatePattern);
156-
this->m_template->setParent(this);
157158
this->m_highlightTemplates.push_back(this->m_template->clone());
158159
this->m_entryCount = count;
159160

lib/include/pl/patterns/pattern_bitfield.hpp

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ namespace pl::ptrn {
5555
public:
5656
PatternBitfieldField(core::Evaluator *evaluator, u64 offset, u8 bitOffset, u8 bitSize, u32 line, PatternBitfieldMember *parentBitfield = nullptr)
5757
: PatternBitfieldMember(evaluator, offset, (bitOffset + bitSize + 7) / 8, line), m_bitOffset(bitOffset % 8), m_bitSize(bitSize) {
58-
this->setParent(parentBitfield);
58+
if (parentBitfield != nullptr)
59+
this->setParent(parentBitfield->reference());
5960
}
6061

6162
PatternBitfieldField(const PatternBitfieldField &other) : PatternBitfieldMember(other) {
@@ -282,7 +283,10 @@ namespace pl::ptrn {
282283
}
283284

284285
[[nodiscard]] std::shared_ptr<Pattern> clone() const override {
285-
return std::unique_ptr<Pattern>(new PatternBitfieldArray(*this));
286+
auto other = std::make_shared<PatternBitfieldArray>(*this);
287+
for (const auto &entry : other->m_entries)
288+
entry->setParent(other->reference());
289+
return other;
286290
}
287291

288292
[[nodiscard]] u8 getBitOffset() const override {
@@ -422,8 +426,6 @@ namespace pl::ptrn {
422426
if (!entry->hasOverriddenColor())
423427
entry->setBaseColor(this->getColor());
424428

425-
entry->setParent(this);
426-
427429
this->m_sortedEntries.push_back(entry.get());
428430
}
429431

@@ -643,7 +645,7 @@ namespace pl::ptrn {
643645
this->setBaseColor(this->m_fields.front()->getColor());
644646

645647
for (const auto &field : this->m_fields) {
646-
field->setParent(this);
648+
field->setParent(this->reference());
647649
this->m_sortedFields.push_back(field.get());
648650
}
649651
}

lib/include/pl/patterns/pattern_struct.hpp

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,17 @@ namespace pl::ptrn {
1515
for (const auto &member : other.m_members) {
1616
auto copy = member->clone();
1717

18-
copy->setParent(this);
1918
this->m_sortedMembers.push_back(copy.get());
2019
this->m_members.push_back(std::move(copy));
2120
}
2221
}
2322

2423
[[nodiscard]] std::shared_ptr<Pattern> clone() const override {
25-
return std::unique_ptr<Pattern>(new PatternStruct(*this));
24+
auto other = std::make_shared<PatternStruct>(*this);
25+
for (const auto &member : other->m_members)
26+
member->setParent(other->reference());
27+
28+
return other;
2629
}
2730

2831
[[nodiscard]] std::shared_ptr<Pattern> getEntry(size_t index) const override {
@@ -36,7 +39,7 @@ namespace pl::ptrn {
3639
void addEntry(const std::shared_ptr<Pattern> &entry) override {
3740
if (entry == nullptr) return;
3841

39-
entry->setParent(this);
42+
entry->setParent(this->reference());
4043
this->m_sortedMembers.push_back(entry.get());
4144
this->m_members.push_back(entry);
4245
}

lib/include/pl/patterns/pattern_union.hpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,11 @@ namespace pl::ptrn {
2121
}
2222

2323
[[nodiscard]] std::shared_ptr<Pattern> clone() const override {
24-
return std::unique_ptr<Pattern>(new PatternUnion(*this));
24+
auto other = std::make_shared<PatternUnion>(*this);
25+
for (const auto &member : other->m_members)
26+
member->setParent(other->reference());
27+
28+
return other;
2529
}
2630

2731
[[nodiscard]] std::shared_ptr<Pattern> getEntry(size_t index) const override {
@@ -35,7 +39,6 @@ namespace pl::ptrn {
3539
void addEntry(const std::shared_ptr<Pattern> &entry) override {
3640
if (entry == nullptr) return;
3741

38-
entry->setParent(this);
3942
this->m_sortedMembers.push_back(entry.get());
4043
this->m_members.push_back(entry);
4144
}

lib/source/pl/core/ast/ast_node_bitfield.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ namespace pl::core::ast {
139139

140140
for (auto &pattern : potentialPatterns) {
141141
if (auto bitfieldMember = dynamic_cast<ptrn::PatternBitfieldMember*>(pattern.get()); bitfieldMember != nullptr) {
142-
bitfieldMember->setParent(bitfieldPattern.get());
142+
bitfieldMember->setParent(bitfieldPattern->reference());
143143
if (!bitfieldMember->isPadding())
144144
fields.push_back(pattern);
145145
} else {

lib/source/pl/core/ast/ast_node_bitfield_array_variable_decl.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ namespace pl::core::ast {
5353
};
5454

5555
auto position = evaluator->getBitwiseReadOffset();
56-
auto arrayPattern = std::make_unique<ptrn::PatternBitfieldArray>(evaluator, position.byteOffset, position.bitOffset, 0, getLocation().line);
56+
auto arrayPattern = std::make_shared<ptrn::PatternBitfieldArray>(evaluator, position.byteOffset, position.bitOffset, 0, getLocation().line);
5757
arrayPattern->setVariableName(this->m_name);
5858
arrayPattern->setSection(evaluator->getSectionId());
5959
arrayPattern->setReversed(evaluator->isReadOrderReversed());
@@ -133,7 +133,7 @@ namespace pl::core::ast {
133133

134134
for (auto &pattern : entries) {
135135
if (auto bitfieldMember = dynamic_cast<ptrn::PatternBitfieldMember*>(pattern.get()); bitfieldMember != nullptr)
136-
bitfieldMember->setParent(arrayPattern.get());
136+
bitfieldMember->setParent(arrayPattern->reference());
137137
}
138138

139139
arrayPattern->setEntries(entries);

lib/source/pl/core/ast/ast_node_builtin_type.cpp

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,23 +23,23 @@ namespace pl::core::ast {
2323
auto size = Token::getTypeSize(this->m_type);
2424
auto offset = evaluator->getReadOffsetAndIncrement(size);
2525

26-
std::unique_ptr<ptrn::Pattern> pattern;
26+
std::shared_ptr<ptrn::Pattern> pattern;
2727
if (Token::isUnsigned(this->m_type))
28-
pattern = std::make_unique<ptrn::PatternUnsigned>(evaluator, offset, size, getLocation().line);
28+
pattern = std::make_shared<ptrn::PatternUnsigned>(evaluator, offset, size, getLocation().line);
2929
else if (Token::isSigned(this->m_type))
30-
pattern = std::make_unique<ptrn::PatternSigned>(evaluator, offset, size, getLocation().line);
30+
pattern = std::make_shared<ptrn::PatternSigned>(evaluator, offset, size, getLocation().line);
3131
else if (Token::isFloatingPoint(this->m_type))
32-
pattern = std::make_unique<ptrn::PatternFloat>(evaluator, offset, size, getLocation().line);
32+
pattern = std::make_shared<ptrn::PatternFloat>(evaluator, offset, size, getLocation().line);
3333
else if (this->m_type == Token::ValueType::Boolean)
34-
pattern = std::make_unique<ptrn::PatternBoolean>(evaluator, offset, getLocation().line);
34+
pattern = std::make_shared<ptrn::PatternBoolean>(evaluator, offset, getLocation().line);
3535
else if (this->m_type == Token::ValueType::Character)
36-
pattern = std::make_unique<ptrn::PatternCharacter>(evaluator, offset, getLocation().line);
36+
pattern = std::make_shared<ptrn::PatternCharacter>(evaluator, offset, getLocation().line);
3737
else if (this->m_type == Token::ValueType::Character16)
38-
pattern = std::make_unique<ptrn::PatternWideCharacter>(evaluator, offset, getLocation().line);
38+
pattern = std::make_shared<ptrn::PatternWideCharacter>(evaluator, offset, getLocation().line);
3939
else if (this->m_type == Token::ValueType::Padding)
40-
pattern = std::make_unique<ptrn::PatternPadding>(evaluator, offset, 1, getLocation().line);
40+
pattern = std::make_shared<ptrn::PatternPadding>(evaluator, offset, 1, getLocation().line);
4141
else if (this->m_type == Token::ValueType::String)
42-
pattern = std::make_unique<ptrn::PatternString>(evaluator, offset, 0, getLocation().line);
42+
pattern = std::make_shared<ptrn::PatternString>(evaluator, offset, 0, getLocation().line);
4343
else if (this->m_type == Token::ValueType::CustomType) {
4444
std::vector<Token::Literal> params;
4545

0 commit comments

Comments
 (0)