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

Bugfix in function erase #1

Merged
merged 1 commit into from
Mar 27, 2020
Merged

Bugfix in function erase #1

merged 1 commit into from
Mar 27, 2020

Conversation

KaruroChori
Copy link

The original code when tested in valgrind was resulting in an access to a memory location beyond the current frame. This should prevent it.

In your original implementation without memory allocation I guess it would allow for some variables on the stack to be affected or corrupted if the root of the tree was deleted.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 97.465% when pulling c46a433 on KaruroChori:master into 1d7d256 on mpaland:master.

@mpaland
Copy link
Owner

mpaland commented Jul 30, 2019

Hello KaruroChori,
thanx a lot for pointing this out.
Do you have a (failing) test case which can force the parent node to be invalid?
Meaning can this practically happen or is it just a theoretical Valgrind assumption?

@KaruroChori
Copy link
Author

KaruroChori commented Jul 30, 2019

I cannot share my own application, moreover it is based on a branched version of your code which is using dynamic allocation and introduces few additional features I need. However I wrote a test case which is also failing with your original code. Let me know if it does work for you as the result may change depending on the optimizations and the relocations of fields that the compiler could perform.

https://pastebin.com/QfMJ1E6b

Please modify the original code for avl_array.h by adding the field "funny":
https://pastebin.com/6BPxbTWQ

And provide a public method size_type f(){return funny;}.

This is just meant to show more easily the system breaking, otherwise the affected variable would be size_.

The program should always output 1234. However in my case it fails. When my edits are applied everything seems ok. This is basically the very same problem I encountered in my application. If you place an assertion in those branches that I edited you will see that they are reachable and by design they access and modify an area of data they are not supposed to touch.

@mpaland
Copy link
Owner

mpaland commented Jul 31, 2019

Thanks a lot. I'll check out your test case and merge your PR afterwards.

I use this module in one of my projects and this memory corruption never lead to a crash, because the last child node in the array was not used (by luck).

@mpaland mpaland merged commit d843758 into mpaland:master Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants