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

[win64] fix for broken hash fn on windows 64: ensure modulo performed on unsigned ints. #712

Closed
wants to merge 1 commit into from

Conversation

truthbk
Copy link

@truthbk truthbk commented Mar 13, 2016

As discussed here: pycurl/pycurl#395 there is an issue on 64 bit windows that will produce a segmentation fault due to bad indexing on the hash table due to a broken hash function hash_fd():lib/multi.c:237 that would operate over a negative value as a product of casting to int. The negative value result of the function, when returned as size_t represents a very large number, that causes a segfault when indexing the hash table.

Because it's just a hash function, the impact of performing the hash function over a cast to unsigned ints should be none, and would incur in no performance penalty. Alternatively I can provide a fix that would ensure we always operate on positive via bitwise operations that would consume three additional cpu cycles. Let me know what's preferable.

@bagder
Copy link
Member

bagder commented Mar 13, 2016

Thanks. Curious though why this hasn't caused any problems until now. Most 64bit systems have 64 bit size_t and 32 bit ints so why does it happen to you at this time and on win64 only, do you know?

@bagder bagder self-assigned this Mar 13, 2016
@bagder
Copy link
Member

bagder commented Mar 13, 2016

the impact of performing the hash function over a cast to unsigned ints should be none

The type cast change should indeed be fine. For consistency, I'll also change the typecast in the fd_key_compare function just above hash_fd.

@bagder
Copy link
Member

bagder commented Mar 13, 2016

Oh. It really should use a curl_socket_t there, which is supposedly 64bit on win64.

@bagder
Copy link
Member

bagder commented Mar 13, 2016

So my suggested edit would make the patch like this:

diff --git a/lib/multi.c b/lib/multi.c
index 6a1f7c8..77ab45a 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -229,19 +229,19 @@ static void sh_freeentry(void *freethis)

 static size_t fd_key_compare(void *k1, size_t k1_len, void *k2, size_t k2_len)
 {
   (void) k1_len; (void) k2_len;

-  return (*((int *) k1)) == (*((int *) k2));
+  return (*((curl_socket_t *) k1)) == (*((curl_socket_t *) k2));
 }

 static size_t hash_fd(void *key, size_t key_length, size_t slots_num)
 {
-  int fd = *((int *) key);
+  curl_socket_t fd = *((curl_socket_t *) key);
   (void) key_length;

-  return (fd % (int)slots_num);
+  return (fd % slots_num);
 }

 /*
  * sh_init() creates a new socket hash and returns the handle for it.
  *

@truthbk
Copy link
Author

truthbk commented Mar 13, 2016

@bagder we've had this not working on 64-bit windows for a while, but figured it'd be fixed upstream. I just think the problem had largely gone unnoticed by the community, for us it seems like the problem arises strictly when using curl multi. I feel like It might be a compiler issue on win64, but this should address it nonetheless.

I think the case that triggers the segfault is when libcurl tries to put in a curl_socket_t (SOCKET on windows) == INVALID_SOCKET (0xffffffff) in the hash table (I'm not sure why we'd want to do that), but that's what I saw on the debugger. I feel that, coupled with the compiler problem, is at the root of this.

I submitted an amended version. Let me know if you want me to make any additional changes, or touch-up anything else.

@bagder
Copy link
Member

bagder commented Mar 13, 2016

I take you tested your original case with this amended version? It looks fine to me.

About storing INVALID_SOCKET in the hash, I think that's a (separate) mistake but probably totally harmless as no code should ever actually try to retrieve it from the hash.

@bagder bagder closed this in c0717a7 Mar 14, 2016
@bagder
Copy link
Member

bagder commented Mar 14, 2016

Oh, and I don't think it actually stores CURL_SOCKET_BAD in the hash, I think it looks for it when it asks for a hash for that socket value.

bagder added a commit that referenced this pull request Mar 14, 2016
Simplify the code by using a single entry that looks for a socket in the
socket hash. As indicated in #712, the code looked for CURL_SOCKET_BAD
at some point and that is ineffective/wrong and this makes it easier to
avoid that.
@truthbk
Copy link
Author

truthbk commented Mar 14, 2016

@bagder I can confirm the fixed hash function addresses the issue described originally in pycurl/pycurl#395

I can see you closed the PR, do you handle these patches separately?

Thanks a lot!

@bagder
Copy link
Member

bagder commented Mar 14, 2016

It was closed when the commit was merged into the curl repository. I also did a follow-up fix then to make sure we don't store in invalid sockets in the hash.

@truthbk
Copy link
Author

truthbk commented Mar 14, 2016

Awesome! Thanks a lot @bagder

@leadscloud
Copy link

@truthbk now i was use win10 64bit, how to fix this issue ?

@truthbk
Copy link
Author

truthbk commented Mar 16, 2017

I'm sorry @sbmzhcn I didn't see this last summer. You should be good just using libcurl 7.48.0 or greater, this fix was included there.

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants