Inifinite loop inside libcurl if time_t is defined to unsigned type. #2004
Comments
|
Ack. I think the major mistake is that the two internal time diff functions return
|
... to cater for systems with unsigned time_t variables. - Renamed the functions to curlx_timediff and Curl_timediff_us. - Added overflow protection for both of them in either direction for both 32 bit and 64 bit time_ts Reported-by: Peter Piekarski Fixes #2004
|
First take at a fix is now in #2005! |
|
We are experiencing the same issue on a platform where time_t is signed - first with a 7.53.0 build, in which the low-speed transfer timeouts were broken because of this, and 7.56.1, in which all transfers just hang. Your fix makes 7.56.1 work! Thank you very much for the impeccable timing :) |
|
Yes, awesome! Thanks for the quick work on this! Since I am not familiar with the processes in this project, please let me know if or when you need me to check or review any fixes you made. |
|
@peterpiekarski: it would be great if you could test my patch and see if it helps fixing the issues for you. It clearly needs some further polishing as it caused CI failures, but I think the logic changes are sound. |
... to cater for systems with unsigned time_t variables. - Renamed the functions to curlx_timediff and Curl_timediff_us. - Added overflow protection for both of them in either direction for both 32 bit and 64 bit time_ts - Reprefixed the curlx_time functions to use Curl_* Reported-by: Peter Piekarski Fixes #2004
|
I can confirm that the changes you made so far will fix the issue. |
|
Thanks for verifying! |
I did this
I updated from libcurl 7.52.1 to 7.56.0 and I found an infinite loop in libcurl, file multi.c, function curl_multi_perform when trying to download a file via libcurl's multi handle approach. The second while loop runs forever on certain systems.
I expected the following
The file should be downloaded.
curl/libcurl version
Issue happens since libcurl version 7.55.0
operating system
QNX 6.5/6.6, maybe others
Detailed problem description
The loop happens on systems where time_t is defined to an unsigned type (e.g. QNX). Differences between unsigned types will never become negative and hence the elements in some time_node tree will not be removed if expired.
It looks like this bug was present for some time now, but with 7.55.0 another fix in multi.c was introduced which made this bug appear. It happens because "Curl_llist_remove(list, e, NULL);" was removed inside add_next_timeout in commit 164a093 which originally made sure (by accident probably) that the list can become empty despite the time_t issue.
Since time_t is used at several places, and since I am not very familiar with libcurl source code, I feel unable to fix this myself or provide a patch.
I would suggest to change the time_t handling (again) to use the standard methods to calculate differences (difftime), or use a custom defined type e.g. "typedef long long cutl_time_t" inside cutltime and hide the actual use of the original time_t type.
Problematic lines are for example in multi.c:
time_t diff; <---- diff is unsigned
node = (struct time_node )e->ptr;
diff = curlx_tvdiff(node->time, now); <---- once note->time has passed, this will never be <= 0
if(diff <= 0)
/ remove outdated entry /
Curl_llist_remove(list, e, NULL); <---- and will never be removed from the list anymore
else
/ the list is sorted so get out on the first mismatch */
break;
The text was updated successfully, but these errors were encountered: