search

pistacheio / pistache

A high-performance REST toolkit written in C++
https://pistacheio.github.io/pistache/
Apache License 2.0
3.12k stars 685 forks source link

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

Closed tyler92 closed 2 months ago

tyler92 commented 2 months ago

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.

Tachi107 commented 2 months ago

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

tyler92 commented 2 months ago

@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 commented 2 months ago

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

Tachi107 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

The fix looks good to me too. Merging!