Skip to content

Fix stack overflow in ifc-printer when parsing self-referential types #89

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Copilot wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Fix stack overflow in ifc-printer when parsing self-referential types #89

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
066e0de
Initial plan
Copilot Jul 3, 2025
f115a25
Fix stack overflow by adding cycle detection to get_string_if_possible
Copilot Jul 3, 2025
376ccde
Remove unnecessary mutable keyword and improve exception safety with …
Copilot Jul 3, 2025
e59b15a
Add build-test directory to gitignore to exclude build artifacts
Copilot Jul 3, 2025
d7ff312
Simplify cycle detection by removing RAII guard and try/catch
Copilot Jul 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/ifc/dom/node.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ namespace ifc::util {
std::string ref(const symbolic::Identity<NameIndex>& id);

std::set<NodeKey> referenced_nodes;
// Track indices currently being processed to detect cycles
mutable std::set<TypeIndex> processing_types;

Copy link
Copy Markdown
Member

@gdr-at-ms gdr-at-ms Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why mutable?

Copy link
Copy Markdown
Contributor Author

Copilot AI Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mutable keyword was unnecessary since processing_types is only accessed through non-const methods. Removed in commit 376ccde.

private:
using NodeMap = std::map<NodeKey, Node>;
Expand Down
22 changes: 19 additions & 3 deletions src/ifc-dom/types.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,25 @@ namespace ifc::util {

std::string get_string_if_possible(Loader& ctx, TypeIndex index)
{
if (not null(index))
return ctx.reader.visit(index, TypeTranslator{ctx});
return "no-type";
if (null(index))
return "no-type";

// Check for cycles to prevent infinite recursion
if (ctx.processing_types.find(index) != ctx.processing_types.end()) {
return "..." + to_string(index); // Return a reference to break the cycle
}

// Mark this type as being processed
ctx.processing_types.insert(index);

try {
std::string result = ctx.reader.visit(index, TypeTranslator{ctx});
ctx.processing_types.erase(index);
return result;
} catch (...) {
ctx.processing_types.erase(index);
Copy link
Copy Markdown
Member

@gdr-at-ms gdr-at-ms Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the try/catch? Which exceptions are you trying to guard against?

Copy link
Copy Markdown
Contributor Author

Copilot AI Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try/catch was for exception safety - ensuring the index is removed from processing_types even if ctx.reader.visit() throws. Replaced with a cleaner RAII guard pattern in commit 376ccde that automatically handles cleanup in the destructor.

Copy link
Copy Markdown
Member

@gdr-at-ms gdr-at-ms Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try/catch was for exception safety - ensuring the index is removed from processing_types even if ctx.reader.visit() throws. Replaced with a cleaner RAII guard pattern in commit 376ccde that automatically handles cleanup in the destructor.

I understand, but I think those changes are making the code worse:

  1. Don't introduce any intermediary guard class. Don't introduce any try/catch.
  2. What kind of exceptions does the expression ctx.reader.visit() actually throw?

Copy link
Copy Markdown
Contributor Author

Copilot AI Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right. I've simplified the code by removing the RAII guard class and try/catch logic. The ctx.reader.visit() can throw error_condition::UnexpectedVisitor exceptions when encountering unknown sort types, but these are genuine error conditions that should propagate up. The simple insert/erase pattern is sufficient for cycle detection. Changes in commit d7ff312.

throw;
}
}

// Load types as full blown nodes.
Expand Down