Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Johannes Sixt <j6t@kdbg.org>]

Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
> From: Steve Hoelzer <shoelzer@gmail.com>
> 
>  From Visual Studio 2015 Code Analysis: Warning C28159 Consider using
> 'GetTickCount64' instead of 'GetTickCount'.
> 
> Reason: GetTickCount() overflows roughly every 49 days. Code that does
> not take that into account can loop indefinitely. GetTickCount64()
> operates on 64 bit values and does not have that problem.
> 
> Note: this patch has been carried in Git for Windows for almost two
> years, but with a fallback for Windows XP, as the GetTickCount64()
> function is only available on Windows Vista and later. However, in the
> meantime we require Vista or later, hence we can drop that fallback.
> 
> Signed-off-by: Steve Hoelzer <shoelzer@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   compat/poll/poll.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/compat/poll/poll.c b/compat/poll/poll.c
> index ad5dcde439..4abbfcb6a4 100644
> --- a/compat/poll/poll.c
> +++ b/compat/poll/poll.c
> @@ -18,6 +18,9 @@
>      You should have received a copy of the GNU General Public License along
>      with this program; if not, see <http://www.gnu.org/licenses/>.  */
>   
> +/* To bump the minimum Windows version to Windows Vista */
> +#include "git-compat-util.h"
> +
>   /* Tell gcc not to warn about the (nfd < 0) tests, below.  */
>   #if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
>   # pragma GCC diagnostic ignored "-Wtype-limits"
> @@ -449,7 +452,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
>     static HANDLE hEvent;
>     WSANETWORKEVENTS ev;
>     HANDLE h, handle_array[FD_SETSIZE + 2];
> -  DWORD ret, wait_timeout, nhandles, start = 0, elapsed, orig_timeout = 0;
> +  DWORD ret, wait_timeout, nhandles, elapsed, orig_timeout = 0;
> +  ULONGLONG start = 0;
>     fd_set rfds, wfds, xfds;
>     BOOL poll_again;
>     MSG msg;
> @@ -465,7 +469,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
>     if (timeout != INFTIM)
>       {
>         orig_timeout = timeout;
> -      start = GetTickCount();
> +      start = GetTickCount64();
>       }
>   
>     if (!hEvent)
> @@ -614,7 +618,7 @@ restart:
>   
>     if (!rc && orig_timeout && timeout != INFTIM)
>       {
> -      elapsed = GetTickCount() - start;
> +      elapsed = (DWORD)(GetTickCount64() - start);

AFAICS, this subtraction in the old code is the correct way to take 
account of wrap-arounds in the tick count. The new code truncates the 64 
bit difference to 32 bits; the result is exactly identical to a 
difference computed from truncated 32 bit values, which is what we had 
in the old code.

IOW, there is no change in behavior. The statement "avoid wrap-around 
issues" in the subject line is not correct. The patch's only effect is 
that it removes Warning C28159.

What is really needed is that all quantities in the calculations are 
promoted to ULONGLONG. Unless, of course, we agree that a timeout of 
more than 49 days cannot happen ;)

>         timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
>       }
>   

-- Hannes