FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Check for -1 after CURLINFO_LASTSOCKET by alexcrichton · Pull Request #3993 · libgit2/libgit2 · GitHub

Repository navigation

Check for -1 after CURLINFO_LASTSOCKET - #3993

Closed
alexcrichton wants to merge 1 commit into
libgit2:masterfrom
alexcrichton:fix-fault
Closed

alexcrichton wants to merge 1 commit into
libgit2:masterfrom
alexcrichton:fix-fault

Conversation

Copy link
Copy Markdown
Contributor

We're recently trying to upgrade to the current mater of libgit2 in Cargo but
we're unfortunately hitting a segfault in one of our tests. A particular test is
just a small smoke test that https works (e.g. it's configured in libgit2). This
test attempts to clone from a URL which simply immediately drops connections
after they're accepted (e.g. terminate abnormally). We expect to see a standard
error from libgit2 but unfortunately we're seeing a segfault.

This segfault is happening inside of the wait_for function of curl_stream.c
at the line FD_SET(fd, &errfd) because fd is -1. This ends up doing an
out-of-bounds array access that faults the program. I tracked back to where this
-1 came from to the line here (returned by CURLINFO_LASTSOCKET) and added a
check to return an error. This may not be the right fix though, so feedback is
appreciated!

We're recently trying to upgrade to the current mater of libgit2 in Cargo but
we're unfortunately hitting a segfault in one of our tests. A particular test is
just a small smoke test that https works (e.g. it's configured in libgit2). This
test attempts to clone from a URL which simply immediately drops connections
after they're accepted (e.g. terminate abnormally). We expect to see a standard
error from libgit2 but unfortunately we're seeing a segfault.

This segfault is happening inside of the `wait_for` function of `curl_stream.c`
at the line `FD_SET(fd, &errfd)` because `fd` is -1. This ends up doing an
out-of-bounds array access that faults the program. I tracked back to where this
-1 came from to the line here (returned by `CURLINFO_LASTSOCKET`) and added a
check to return an error. This may not be the right fix though, so feedback is
appreciated!

pks-t commented Nov 11, 2016

Copy link
Copy Markdown
Member

Thanks for the fix. I manually merged the commit via b782205 as I've removed the last sentence from the commit message (that is, the bit about not being sure if the fix is right, as it is).

Copy link
Copy Markdown
Contributor Author

Thanks!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

2 participants


Back | FazBrowse Home | New Git URL