-
Notifications
You must be signed in to change notification settings - Fork 100
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 buffer overflows when accessing data allocated in memdupdec #270
Conversation
I think we should. For the memory tests, but also to just make sure that we make a decision on how to act on them and make sure we think trurl behaves correctly. |
I won't mind to merge this PR first though and we can work on adding more test cases based on this in a separate PR. Let me know what you prefer. |
I think that makes sense - I was mostly looking at these two instances.
specifically the null encoding in the json seems wrong. I would expect it look to like I can open a discussion or something about it this evenin |
I'm certainly not a json wizard, but yes that is my impression. The |
Okay, perfect. I added the tests so this should be all good to go. Something I want to add soon is an action that uses debug symbols from curl, because valgrind and the address sanitizer don't seem to keep track of memory allocations from release versions of libcurl so there have been a few sneaky memory leaks and stuff that have gone unnoticed - though I've addressed as many as I can find. Edit: stupid rebase issues. resolving now. |
eb4fd19
to
ff483f1
Compare
Okay, rebase conflicts are all resolved, this should now be good to pull in |
Thanks! |
This is a culmination of work to address memory errors found by @Fusl in #265, #266 and #267. Unsure if we need to add explicit tests for the cases used for testing -- mostly because I'm not sure how trurl should handle these weird urls, but we can include them for the sake of memory testing if people think its needed.
Fixes #265
Fixes #266
Fixes #267