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

Fix potential overflow problem with timevals #646

Closed
wants to merge 1 commit into from

Conversation

@Andersbakken
Copy link
Contributor

@Andersbakken Andersbakken commented Feb 11, 2016

When comparing a null timeval with a current timeval using curlx_tvdiff on a 32-bit linux system there's an overflow happening.

Compiling with -fsanitize=undefined I get this warning on startup of our app using libcurl:

.../curl/lib/timeval.c:120:37: runtime error: signed integer overflow: 5873565 * 1000 cannot be represented in type 'long int'

The attached fix solves the problem by checking for a null timeval before comparing. Strictly speaking the function probably should return something that's guaranteed to be 64-bit but the majority of the usecases actually cast the result to an int and basically just compares two timevals that are close together so I decided against making a bigger change.

current timeval.

Without this I get this warning from -fsanitize=undefined on startup of
our app:

.../curl/lib/timeval.c:120:37: runtime error: signed integer overflow: 5873565 * 1000 cannot be represented in type 'long int'
@bagder
Copy link
Member

@bagder bagder commented Feb 11, 2016

Good catch! But since we use the *_tvdiff() function in quite a large number of places, maybe it would be better to fix that function to not do overflows? I'm thinking like this:

@@ -115,10 +115,17 @@ struct timeval curlx_tvnow(void)
  *
  * Returns: the time difference in number of milliseconds.
  */
 long curlx_tvdiff(struct timeval newer, struct timeval older)
 {
+#if SIZEOF_TIME_T < 8
+  /* for 32bit systems, add precaution to avoid overflow for really big time
+     differences */
+  time_t diff = newer.tv_sec-older.tv_sec;
+  if(diff > (0x7fffffff/1000))
+    return 0x7fffffff;
+#endif
   return (newer.tv_sec-older.tv_sec)*1000+
     (long)(newer.tv_usec-older.tv_usec)/1000;
 }

 /*
@bagder bagder self-assigned this Feb 11, 2016
@Andersbakken
Copy link
Contributor Author

@Andersbakken Andersbakken commented Feb 12, 2016

Yeah. That might be a better fix actually.

@bagder
Copy link
Member

@bagder bagder commented Feb 12, 2016

I did some more math and I think the check needs to be

if(diff >= (0x7fffffff/1000))

... with a greater than or equal, as otherwise it can actually still go over the 31 bit limit. I'll merge this!

@bagder bagder closed this in d6b37d8 Feb 12, 2016
@Andersbakken
Copy link
Contributor Author

@Andersbakken Andersbakken commented Feb 12, 2016

Thanks.

Anders

On Thu, Feb 11, 2016 at 11:13 PM, Daniel Stenberg notifications@github.com
wrote:

Closed #646 #646 via d6b37d8
d6b37d8
.


Reply to this email directly or view it on GitHub
#646 (comment).

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.