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

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@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
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
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
Member
bagder commented Mar 13, 2016

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

@bagder
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.
  *
Jaime Fullaondo [win64] fix for broken hash fn on windows 64: ensure modulo performed…
… on curl_socket_t (unsigned).
f9ed0dc
@truthbk
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
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 added a commit that closed this pull request Mar 14, 2016
@bagder Jaime Fullaondo + bagder multi hash: ensure modulo performed on curl_socket_t
Closes #712
c0717a7
@bagder bagder closed this in c0717a7 Mar 14, 2016
@bagder
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 bagder added a commit that referenced this pull request Mar 14, 2016
@bagder bagder multi: introduce sh_getentry() for looking up sockets in the sockhash
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.
8eaf884
@truthbk
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!

@truthbk truthbk referenced this pull request in pycurl/pycurl Mar 14, 2016
Closed

PycURL 💔 Windows & Python 64-bit #395

@bagder
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
truthbk commented Mar 14, 2016

Awesome! Thanks a lot @bagder

@sbmzhcn
sbmzhcn commented Jul 28, 2016

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

@truthbk
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.

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