Inifinite loop inside libcurl if time_t is defined to unsigned type. #2004

Closed
peterpiekarski opened this Issue Oct 23, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@peterpiekarski

peterpiekarski commented Oct 23, 2017

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;

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 23, 2017

Member

Ack.

I think the major mistake is that the two internal time diff functions return time_t, when they should rather return another type that is guaranteed to be signed and as wide as time_t...

difftime() returns a double and I find that inconvenient.

Member

bagder commented Oct 23, 2017

Ack.

I think the major mistake is that the two internal time diff functions return time_t, when they should rather return another type that is guaranteed to be signed and as wide as time_t...

difftime() returns a double and I find that inconvenient.

bagder added a commit that referenced this issue Oct 23, 2017

timediff: return timediff_t from the time diff functions
... 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
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 23, 2017

Member

First take at a fix is now in #2005!

Member

bagder commented Oct 23, 2017

First take at a fix is now in #2005!

@jruffin

This comment has been minimized.

Show comment
Hide comment
@jruffin

jruffin Oct 23, 2017

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 :)

jruffin commented Oct 23, 2017

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 :)

@peterpiekarski

This comment has been minimized.

Show comment
Hide comment
@peterpiekarski

peterpiekarski Oct 24, 2017

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.

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.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 24, 2017

Member

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

Member

bagder commented Oct 24, 2017

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

jruffin added a commit to jruffin/curl that referenced this issue Oct 24, 2017

timediff: build fix for #2005.
A small extension of the patch in #2005 that builds and passes tests.

Fixes #2004

bagder added a commit that referenced this issue Oct 24, 2017

timediff: return timediff_t from the time diff functions
... 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
@peterpiekarski

This comment has been minimized.

Show comment
Hide comment
@peterpiekarski

peterpiekarski Oct 24, 2017

I can confirm that the changes you made so far will fix the issue.

I can confirm that the changes you made so far will fix the issue.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 24, 2017

Member

Thanks for verifying!

Member

bagder commented Oct 24, 2017

Thanks for verifying!

@bagder bagder closed this in b9d25f9 Oct 25, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018

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