Skip to content
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.

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

Prevent memory crash when building Query Nodes #6428

Closed
wants to merge 4 commits into from

Conversation

desistefanova
Copy link
Contributor

@desistefanova desistefanova commented Mar 23, 2023

Having a large query with more than 1000 LogicalNodes crashes and kills the process.
For example query like this:

_id == oid(641c02e5a38d34a988a471ab) OR _id == oid(641c02e5a38d34a988a471ac) OR _id == oid(641c02e5a38d34a988a471ad) OR _id == oid(641c02e5a38d34a988a471ae) OR _id == oid(641c02e5a38d34a988a471af) OR _id == oid(641c02e5a38d34a988a471b0).....

Set limit of QueryNodes depth in the LogicalNode recursion in driver.hpp.
Added MAX_DEPTH_OF_LOGICAL_NODES=1000.

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed.

@cla-bot cla-bot bot added the cla: yes label Mar 23, 2023
@desistefanova desistefanova changed the title Prevent memory crash in Query Parser Prevent memory crash when building Query Nodes Mar 23, 2023
@desistefanova desistefanova force-pushed the ds/query_logical_node_depth branch from 6a068b7 to 64b8e06 Compare March 23, 2023 12:20
@desistefanova desistefanova force-pushed the ds/query_logical_node_depth branch from 64b8e06 to 5a40b57 Compare March 23, 2023 13:03
int exceeded_max_count = 1001;
for (int node_id = 0; node_id <= exceeded_max_count; node_id++) {
std::stringstream node;
node << "_id=obj(" << ObjectId().gen().to_string() << ")"
Copy link
Contributor

Choose a reason for hiding this comment

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

"obj" -> "oid"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor

@nielsenko nielsenko left a comment

Choose a reason for hiding this comment

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

I don't think we should try to fix the issue with a max depth. We don't know that 1000 nodes will be a small enough on all machines at any given time?

Also, this is hardly the only place we can hit a stack overflow. If we want to improve here we should look into SEH (Structured Exception Handling) on Windows, and maybe something like libsigsegv for gcc

@desistefanova desistefanova requested a review from jedelbo March 27, 2023 22:37
@desistefanova
Copy link
Contributor Author

I don't think we should try to fix the issue with a max depth. We don't know that 1000 nodes will be a small enough on all machines at any given time?

Also, this is hardly the only place we can hit a stack overflow. If we want to improve here we should look into SEH (Structured Exception Handling) on Windows, and maybe something like libsigsegv for gcc

I agree that the max depth can not be predicted. It would be better if this code could be rewritten without recursion.

@nicola-cab
Copy link
Member

@ironage PR does that. It is unlikely that we won't be able to cope with a very long list of conditions without recursion.

@jedelbo
Copy link
Contributor

jedelbo commented Mar 28, 2023

I don't think we should merge this PR now that we have the fix done by @ironage

@desistefanova
Copy link
Contributor Author

I don't think we should merge this PR now that we have the fix done by @ironage

Which is the @ironage PR that fixes that? I would like to link it here, before to close this PR.

@ironage
Copy link
Contributor

ironage commented Mar 28, 2023

@desistefanova it is #6444
Thanks for bringing this issue to our attention!

@desistefanova
Copy link
Contributor Author

Handled in #6444

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants