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

Change from heap::increase to heap::decrease in include/util/query_heap.hpp with possible segfault in rare circumstances #6603

Closed
mugr1x opened this issue Apr 17, 2023 · 2 comments

Comments

@mugr1x
Copy link
Contributor

mugr1x commented Apr 17, 2023

Issue

I think the DecreaseKey in include/util/query_heap.hpp wrongly calls the increase instead of decrease function from boost::heap.

Steps to reproduce

See the docs:
https://www.boost.org/doc/libs/1_76_0/doc/html/boost/heap/d_ary_heap.html#id-1_3_18_6_2_1_1_1_22_17-bb
My thoughts behind this are that there is logic in multiple files e.g. include/engine/routing_algorithms/routing_base_mld.hpp that checks for a certain "to_weight" to be smaller than the current "toHeapNode->weight" and then calls DecreaseKey which in turn calls heap.increase with "to_weight".
So the docs say: "The new value is expected to be less than the current one" for decrease not increase which is the other way round. Wouldn't it be better to replace increase with decrease then to fit the docs ?
Also I guess it kinda fixed a segfault for me while testing. (I know for sure it was this one, however might only have emerged with now reverted changes since now not using std::optional instead just pointers)
Am I completely mistaken here ? I still get from A to B though using my setup e.g. with the Germany map.

@SiarheiFedartsou
Copy link
Member

Hey

Also I guess it kinda fixed a segfault for me while testing.

Can you reproduce it? If so, could you please share how I would do that?

@mugr1x
Copy link
Contributor Author

mugr1x commented Apr 19, 2023

Hello, as for the segfault it occured when I tried switching to std::optional. However in the process I also changed pointers or functions singatures a lot. (not really necessary it turns out) See this branch:
https://github.com/mugr1x/osrm/tree/boost_optional_old
If you change the heap::increase call to heap::decrease at least it does not segfault anymore.
Also I probably should do a cleanup.

@mugr1x mugr1x closed this as not planned Won't fix, can't repro, duplicate, stale Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants