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 buffer overflows when accessing data allocated in memdupdec #270

Closed
wants to merge 6 commits into from

Conversation

jacobmealey
Copy link
Contributor

@jacobmealey jacobmealey commented Feb 10, 2024

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

@jacobmealey jacobmealey changed the title clean up error handling in memdupdec to fix buffer overflows Fix buffer overflows when accessing data allocated in memdupdec Feb 10, 2024
@bagder
Copy link
Member

bagder commented Feb 19, 2024

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.

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.

@bagder
Copy link
Member

bagder commented Feb 19, 2024

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.

@jacobmealey
Copy link
Contributor Author

jacobmealey commented Feb 19, 2024

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.

$ ./trurl "0?00%000000000000000000000=0000000000"
http://0.0.0.0/?00%000000000000000000000=0000000000
$ ./trurl --json 0?0%000000000000000000000000000000000 
[
  {
    "url": "http://0.0.0.0/?0%000000000000000000000000000000000",
    "parts": {
      "scheme": "http",
      "host": "0.0.0.0",
      "path": "/"
    },
    "params": [
      {
        "key": "0\u00000000000000000000000000000000000",
        "value": ""
      }
    ]
  }
]

specifically the null encoding in the json seems wrong. I would expect it look to like 0\u0000\u0000\u000.... Is it that the first four zeros after the \u are part of the null encoding and all the following 0 are just regular ascii characters?

I can open a discussion or something about it this evenin

@bagder
Copy link
Member

bagder commented Feb 19, 2024

Is it that the first four zeros after the \u are part of the null encoding and all the following 0 are just regular ascii characters?

I'm certainly not a json wizard, but yes that is my impression. The \u is followed by four digits to form a unicode character, then follows regular ascii digits.

@jacobmealey
Copy link
Contributor Author

jacobmealey commented Feb 19, 2024

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.

@jacobmealey
Copy link
Contributor Author

Okay, rebase conflicts are all resolved, this should now be good to pull in

@bagder bagder closed this in d801161 Feb 19, 2024
@bagder
Copy link
Member

bagder commented Feb 19, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment