-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix(src): avoid possible infinite loop in LoadAll(). #1319
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test (e.g., the code in the PR description)? Something that infinite-loops before this PR but doesn't after.
Added the test; please note that if #1318 is merged first, i'll need to rebase this PR on master and fix up the test (that will then throw a |
Merged the other one. Go ahead and rebase this and then ping me. |
Leave at first empty root. Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
7e0c9e0
to
3d96ac1
Compare
@jbeder done! |
Sorry for the ping @jbeder , any news on this one? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. It looks like I forgot to hit "send" on the review itself - github's UI is terrible :(
test/integration/node_spec_test.cpp
Outdated
@@ -941,6 +941,13 @@ TEST(NodeSpecTest, Ex7_24_FlowNodes) { | |||
EXPECT_EQ("", doc[4].as<std::string>()); | |||
} | |||
|
|||
TEST(NodeSpecTest, Ex7_25_InfiniteLoopNodes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test file is just for the spec tests, ie, examples literally in the spec.
Can you add it to node_test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
Thanks for the feedback! (and agree about github's UI :/ )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see 2 CI failures that don't seem to be caused by my commits:
/usr/include/gtest/internal/gtest-port.h:279:2: error: #error C++ versions less than C++14 are not supported.
279 | #error C++ versions less than C++14 are not supported.
I think they bumped gtest version and it now requires c++14, therefore all jobs that use deps from system and building against c++11 will fail.
Signed-off-by: Federico Di Pierro <[email protected]>
03d7a90
to
91b45cb
Compare
Hey @jbeder Any update on this? |
Leave at first empty root.
This avoids an infinite loop in case of
HandleNextDocument
returning true even if the document has errors, basically because notoken.type
gets matched inSingleDocParser::HandleNode
, leading toeventHandler.OnNull(mark, anchor);
being called indefinitely at each iteration, pushing a null node to documents vector.Another solution would be to add a
m_scanner.pop();
in thedefault
switch case inSingleDocParser::HandleNode
to make sure we always consume the current token.The behavior causes this issue: falcosecurity/falco#3281
Easily reproducible with a simple c++ example: