Skip to content
This repository has been archived by the owner on Apr 16, 2019. It is now read-only.

timer: Fix non-critical integer overflow #129

Merged
merged 1 commit into from
Nov 23, 2016
Merged

timer: Fix non-critical integer overflow #129

merged 1 commit into from
Nov 23, 2016

Conversation

saleemrashid
Copy link
Contributor

@saleemrashid saleemrashid commented Nov 18, 2016

Every 4294967295 milliseconds (2 ^ 32 - 1), system_millis will overflow.
This means that every 49.71 days, system_millis will reset to zero.
Comparisons like system_millis < (system_millis + 1) would fail if the
latter had overflown and the former had not.

This is non-critical because the worst case is that one second could be
skipped or the screen could lock early.

This poses no threat to the exponential backoff used for protection
against brute force.

Thanks to @jhoenicke for the provided solution ❤️

@prusnak
Copy link
Member

prusnak commented Nov 21, 2016

@jhoenicke ack?

@jhoenicke
Copy link
Collaborator

Okay, I checked to make sure. Casting uint32 to int32 is "implementation defined" and gcc defines it the way we want it. In principle a different compiler may throw an implementation defined exception, though.

Since we use gcc it is okay, but for more compatibility something like
(system_millis - start_millis) < delay
(where delay is an unsigned integer or constant) is more portable. The behaviour of unsigned ints is more standardized than signed ints (uint32_t operations are always modulo 2^32 on over-/underflow).

Every 4294967295 milliseconds (2 ^ 32 - 1), system_millis will overflow.
This means that every 49.71 days, system_millis will reset to zero.
Comparisons like `system_millis < (system_millis + 1)` would fail if the
latter had overflown and the former had not.

This is non-critical because the worst case is that one second could be
skipped or the screen could lock early.

This poses no threat to the exponential backoff used for protection
against brute force.
@saleemrashid
Copy link
Contributor Author

saleemrashid commented Nov 23, 2016

@prusnak @jhoenicke Updated

@prusnak prusnak merged commit b4eaf7d into trezor:master Nov 23, 2016
@prusnak
Copy link
Member

prusnak commented Nov 23, 2016

Thanks!

@saleemrashid saleemrashid deleted the timers-overflow branch November 23, 2016 20:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants