Menu
▾
▴
#731 Race Condition in Curl_resolv(hostip.c) (dns cache)
Hello.
I've noticed a bug in Curl_resolv which can lead to crash of curl(or bad behaviour)(I've checked versions 7.16.1 and 7.18.0, but I think older versions are also buggy). Look at hostip.c:423 (curl 7.18.0). Consider we're sharing dns cache between 2 threads(A and B).
A: locks dns cache
A: gets dns from cache
A: unlocks dns cache
B: locks dns cache
B: gets dns from cache
B: unlock dns cache
A: checks remove_entry_if_stale and it returns true
.
So dns entry is removed from cache and thread A knows it, so it sets dns = NULL; but thread B doesn't know it and it has dns variable pointed to nobody knows what...
My solution for that, diff hostip_old.c hostip.c (v7.18.0) is attached. Not sure it's ok, but it worked on 7.16.1 (at least I didn't get curl crashed).
Regards, Andrew..
diff hostip_old.c hostip.c
Logged In: YES
user_id=1110
Originator: NO
Thanks a lot, but can you please make the diff with -u and submit such a version?
diff -u hostip_old.c hostip.c
Logged In: YES
user_id=2032292
Originator: YES
Not sure it's ok though. It just works in my case..
File Added: mydiff
Logged In: YES
user_id=1110
Originator: NO
Unfortunately, your approach is not good enough either since it also uses 'dns' outside the locks without having it "bumped" so there's still a small chance that the other thread prunes that entry exactly at that point.
I did a change that makes sure to do inuse++ within the lock so that the race can be avoided. It makes test 506 fail, but no other and 506 is expected to fail since this patch changes the number of locks used.
Can you please try this version and tell us how it works?
File Added: Curl_resolv-race.patch
Logged In: YES
user_id=2032292
Originator: YES
Seems ok to me
Logged In: YES
user_id=1110
Originator: NO
Great. If you tell me your full name, I'll give you proper credit for this finding in the changelog!
Logged In: YES
user_id=1110
Originator: NO
Fix committed to CVS just now. Thanks for the report. Case closed!