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

fix(server): Crash if remove not existing route #1209

Merged
merged 4 commits into from
Apr 28, 2024

Conversation

tyler92
Copy link
Contributor

@tyler92 tyler92 commented Apr 27, 2024

One more crash was found by fuzzer in router.cc. I've added unit tests that cover the "removeRoute" function, uncommented a corresponding line in the fuzzer, and renamed corpus files to their SHA1 hashes.

Copy link
Member

@Tachi107 Tachi107 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Just one minor request regarding the newly added test. Otherwise, this looks good to me :)

tests/router_test.cc Outdated Show resolved Hide resolved
@Tachi107
Copy link
Member

It seems that this patch is causing test failures on RHEL 9 with the router_test.test_remove_not_existing test.

@tyler92
Copy link
Contributor Author

tyler92 commented Apr 27, 2024

@Tachi107 Thanks for the suggestion with tetsing::ThrowsMessage. I noticed that @kiplingw committed it, and I committed include + fix for the error message. Sometimes we throw Requested does not exist, and sometimes Requested route does not exist. I made this error the same. Please let me know if you have a better suggestion here.

@tyler92
Copy link
Contributor Author

tyler92 commented Apr 27, 2024

Sorry, accidentally pushed my local changes for fuzzing, reverted them.

@Tachi107
Copy link
Member

Sometimes we throw Requested does not exist, and sometimes Requested route does not exist. I made this error the same. Please let me know if you have a better suggestion here.

Yeah making the error message the same is definitely a good choice! Still, the new test is consistently failing on RHEL 9, do you think you know why this is the case?

@tyler92
Copy link
Contributor Author

tyler92 commented Apr 28, 2024

There was an issue with fragment[0] expression in router.cc. Formally it's not a memory issue because it's std::string_view and fragment[0] always points (in our case) to a valid memory region. But for the RHEL environment there was an assert in the standard library that failed the test. I've added a check

#0  0x00007f46873a854c in __pthread_kill_implementation () from /lib64/libc.so.6
#1  0x00007f468735bd06 in raise () from /lib64/libc.so.6
#2  0x00007f468732f7f3 in abort () from /lib64/libc.so.6
#3  0x00007f4688010290 in std::__glibcxx_assert_fail(char const*, int, char const*, char const*) () from /pistache/build/tests/../src/libpistache.so.0
#4  0x00007f4687ecd35e in std::basic_string_view<char, std::char_traits<char> >::operator[] (this=0x7f468544e320, __pos=0)
    at /opt/rh/gcc-toolset-13/root/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/string_view:258
#5  0x00007f4687eae7ef in Pistache::Rest::SegmentTreeNode::getSegmentType (fragment="") at ../src/server/router.cc:89
#6  0x00007f4687eafeb2 in Pistache::Rest::SegmentTreeNode::removeRoute (this=0x7f4685825820, path="/v1/hello") at ../src/server/router.cc:205
#7  0x000000000058cbe8 in router_test_test_remove_not_existing_Test::TestBody()::$_0::operator()() const (this=0x7f46858259e0) at ../tests/router_test.cc:258
...

@Tachi107
Copy link
Member

The fix looks good to me too. Merging!

@Tachi107 Tachi107 merged commit 9331499 into pistacheio:master Apr 28, 2024
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants