On Fri, Nov 2, 2018 at 11:43 AM Johannes Sixt <j6t@xxxxxxxx> wrote:
>
> Am 02.11.18 um 15:47 schrieb Steve Hoelzer:
> > On Thu, Nov 1, 2018 at 5:22 AM Johannes Sixt <j6t@xxxxxxxx> wrote:
> >>
> >> Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
> >>> @@ -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 ;)
> >
> > Yep, correct on all counts. I'm in favor of changing the commit message to
> > only say that this patch removes Warning C28159.
>
> How about this fixup instead?
>
> ---- 8< ----
> squash! poll: use GetTickCount64() to avoid wrap-around issues
>
> The value of timeout starts as an int value, and for this reason it
> cannot overflow unsigned long long aka ULONGLONG. The unsigned version
> of this initial value is available in orig_timeout. The difference
> (orig_timeout - elapsed) cannot wrap around because it is protected by
> a conditional (as can be seen in the patch text). Hence, the ULONGLONG
> difference can only have values that are smaller than the initial
> timeout value and truncation to int cannot overflow.
>
> Signed-off-by: Johannes Sixt <j6t@xxxxxxxx>
> ---
> compat/poll/poll.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/compat/poll/poll.c b/compat/poll/poll.c
> index 4abbfcb6a4..4459408c7d 100644
> --- a/compat/poll/poll.c
> +++ b/compat/poll/poll.c
> @@ -452,7 +452,7 @@ 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, elapsed, orig_timeout = 0;
> + DWORD ret, wait_timeout, nhandles, orig_timeout = 0;
> ULONGLONG start = 0;
> fd_set rfds, wfds, xfds;
> BOOL poll_again;
> @@ -618,8 +618,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
>
> if (!rc && orig_timeout && timeout != INFTIM)
> {
> - elapsed = (DWORD)(GetTickCount64() - start);
> - timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
> + ULONGLONG elapsed = GetTickCount64() - start;
> + timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
> }
>
> if (!rc && timeout)
> --
> 2.19.1.406.g1aa3f475f3
I like it. This still removes the warning and avoids overflow issues.
Steve
