git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/1] Make compat/poll safer on Windows
@ 2018-10-31 21:11 Johannes Schindelin via GitGitGadget
  2018-10-31 21:11 ` [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues Steve Hoelzer via GitGitGadget
  0 siblings, 1 reply; 17+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-10-31 21:11 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

This is yet another piece from the Git for Windows cake. It avoids a
wrap-around in the poll emulation on Windows that occurs every 49 days.

Steve Hoelzer (1):
  poll: use GetTickCount64() to avoid wrap-around issues

 compat/poll/poll.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)


base-commit: 4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-64%2Fdscho%2Fmingw-safer-compat-poll-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-64/dscho/mingw-safer-compat-poll-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/64
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-10-31 21:11 [PATCH 0/1] Make compat/poll safer on Windows Johannes Schindelin via GitGitGadget
@ 2018-10-31 21:11 ` Steve Hoelzer via GitGitGadget
  2018-11-01 10:21   ` Johannes Sixt
  0 siblings, 1 reply; 17+ messages in thread
From: Steve Hoelzer via GitGitGadget @ 2018-10-31 21:11 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Steve Hoelzer

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);
       timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
     }
 
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-10-31 21:11 ` [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues Steve Hoelzer via GitGitGadget
@ 2018-11-01 10:21   ` Johannes Sixt
  2018-11-02 14:47     ` Steve Hoelzer
  0 siblings, 1 reply; 17+ messages in thread
From: Johannes Sixt @ 2018-11-01 10:21 UTC (permalink / raw)
  To: Steve Hoelzer via GitGitGadget
  Cc: git, Junio C Hamano, Steve Hoelzer, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-01 10:21   ` Johannes Sixt
@ 2018-11-02 14:47     ` Steve Hoelzer
  2018-11-02 16:43       ` Johannes Sixt
  0 siblings, 1 reply; 17+ messages in thread
From: Steve Hoelzer @ 2018-11-02 14:47 UTC (permalink / raw)
  To: j6t; +Cc: gitgitgadget, git, Junio C Hamano, Johannes Schindelin

On Thu, Nov 1, 2018 at 5:22 AM Johannes Sixt <j6t@kdbg.org> wrote:
>
> 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 ;)

Yep, correct on all counts. I'm in favor of changing the commit message to
only say that this patch removes Warning C28159.

Steve

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-02 14:47     ` Steve Hoelzer
@ 2018-11-02 16:43       ` Johannes Sixt
  2018-11-02 17:18         ` Steve Hoelzer
                           ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Johannes Sixt @ 2018-11-02 16:43 UTC (permalink / raw)
  To: Steve Hoelzer; +Cc: gitgitgadget, git, Junio C Hamano, Johannes Schindelin

Am 02.11.18 um 15:47 schrieb Steve Hoelzer:
> On Thu, Nov 1, 2018 at 5:22 AM Johannes Sixt <j6t@kdbg.org> 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@kdbg.org>
---
 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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-02 16:43       ` Johannes Sixt
@ 2018-11-02 17:18         ` Steve Hoelzer
  2018-11-03  0:39         ` Junio C Hamano
  2018-11-03  8:14         ` Carlo Arenas
  2 siblings, 0 replies; 17+ messages in thread
From: Steve Hoelzer @ 2018-11-02 17:18 UTC (permalink / raw)
  To: j6t; +Cc: gitgitgadget, git, Junio C Hamano, Johannes Schindelin

On Fri, Nov 2, 2018 at 11:43 AM Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 02.11.18 um 15:47 schrieb Steve Hoelzer:
> > On Thu, Nov 1, 2018 at 5:22 AM Johannes Sixt <j6t@kdbg.org> 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@kdbg.org>
> ---
>  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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-02 16:43       ` Johannes Sixt
  2018-11-02 17:18         ` Steve Hoelzer
@ 2018-11-03  0:39         ` Junio C Hamano
  2018-11-03  0:49           ` Junio C Hamano
  2018-11-03  8:14         ` Carlo Arenas
  2 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2018-11-03  0:39 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Steve Hoelzer, gitgitgadget, git, Johannes Schindelin

Johannes Sixt <j6t@kdbg.org> writes:

>> 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?

Isn't that already in 'next'?  I didn't check, though.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-03  0:39         ` Junio C Hamano
@ 2018-11-03  0:49           ` Junio C Hamano
  0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2018-11-03  0:49 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Steve Hoelzer, gitgitgadget, git, Johannes Schindelin

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Sixt <j6t@kdbg.org> writes:
>
>>> 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?
>
> Isn't that already in 'next'?  I didn't check, though.

Well, it turnsout that I already prepared one but not pushed it out
yet.  I'll eject this topic and rebuild the integration branches,
and wait until Dscho says something, to avoid having to redo the
integration cycle again.

Thanks.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-02 16:43       ` Johannes Sixt
  2018-11-02 17:18         ` Steve Hoelzer
  2018-11-03  0:39         ` Junio C Hamano
@ 2018-11-03  8:14         ` Carlo Arenas
  2018-11-03 14:05           ` Johannes Sixt
  2 siblings, 1 reply; 17+ messages in thread
From: Carlo Arenas @ 2018-11-03  8:14 UTC (permalink / raw)
  To: j6t; +Cc: shoelzer, gitgitgadget, git, gitster, johannes.schindelin

On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt <j6t@kdbg.org> wrote:
>
> +      timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);

nitpick: cast to DWORD instead of int

Carlo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-03  8:14         ` Carlo Arenas
@ 2018-11-03 14:05           ` Johannes Sixt
  2018-11-04 23:26             ` Junio C Hamano
  0 siblings, 1 reply; 17+ messages in thread
From: Johannes Sixt @ 2018-11-03 14:05 UTC (permalink / raw)
  To: Carlo Arenas; +Cc: shoelzer, gitgitgadget, git, gitster, johannes.schindelin

Am 03.11.18 um 09:14 schrieb Carlo Arenas:
> On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt <j6t@kdbg.org> wrote:
>>
>> +      timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
> 
> nitpick: cast to DWORD instead of int

No; timeout is of type int; after an explicit type cast we don't want to 
have another implicit conversion.

-- Hannes

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-03 14:05           ` Johannes Sixt
@ 2018-11-04 23:26             ` Junio C Hamano
  2018-11-04 23:44               ` Randall S. Becker
                                 ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Junio C Hamano @ 2018-11-04 23:26 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Carlo Arenas, shoelzer, gitgitgadget, git, johannes.schindelin

Johannes Sixt <j6t@kdbg.org> writes:

> Am 03.11.18 um 09:14 schrieb Carlo Arenas:
>> On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt <j6t@kdbg.org> wrote:
>>>
>>> +      timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
>>
>> nitpick: cast to DWORD instead of int
>
> No; timeout is of type int; after an explicit type cast we don't want
> to have another implicit conversion.
>
> -- Hannes

OK, thanks.  It seems that the relative silence after this message
is a sign that the resulting patch after squashing is what everybody
is happey with?

-- >8 --
From: Steve Hoelzer <shoelzer@gmail.com>
Date: Wed, 31 Oct 2018 14:11:36 -0700
Subject: [PATCH] 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@kdbg.org>
Acked-by: Steve Hoelzer <shoelzer@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 compat/poll/poll.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/compat/poll/poll.c b/compat/poll/poll.c
index ad5dcde439..4459408c7d 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, 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,8 +618,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
 
   if (!rc && orig_timeout && timeout != INFTIM)
     {
-      elapsed = GetTickCount() - 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-816-gcd69ec8cde


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* RE: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-04 23:26             ` Junio C Hamano
@ 2018-11-04 23:44               ` Randall S. Becker
  2018-11-05  3:33               ` Eric Sunshine
  2018-11-05  7:01               ` Johannes Sixt
  2 siblings, 0 replies; 17+ messages in thread
From: Randall S. Becker @ 2018-11-04 23:44 UTC (permalink / raw)
  To: 'Junio C Hamano', 'Johannes Sixt'
  Cc: 'Carlo Arenas', shoelzer, gitgitgadget, git,
	johannes.schindelin

On November 4, 2018 6:26 PM, Junio C Hamano, wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > Am 03.11.18 um 09:14 schrieb Carlo Arenas:
> >> On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt <j6t@kdbg.org> wrote:
> >>>
> >>> +      timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout -
> >>> + elapsed);
> >>
> >> nitpick: cast to DWORD instead of int
> >
> > No; timeout is of type int; after an explicit type cast we don't want
> > to have another implicit conversion.
> >
> > -- Hannes
> 
> OK, thanks.  It seems that the relative silence after this message is a
sign that
> the resulting patch after squashing is what everybody is happey with?

On my platform (HPE NonStop), DWORD is being defined as unsigned int
(32-bit) rather than unsigned long long (64 bit). The definition comes
through the odbc/windows.h include, not the compiler or any core definition.
It's only a nano-quibble (if even that), because GetTickCount64 is not
defined on the platform anyway, so this is probably not a big deal.

Cheers,
Randall



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-04 23:26             ` Junio C Hamano
  2018-11-04 23:44               ` Randall S. Becker
@ 2018-11-05  3:33               ` Eric Sunshine
  2018-11-05  4:02                 ` Junio C Hamano
  2018-11-05  7:01               ` Johannes Sixt
  2 siblings, 1 reply; 17+ messages in thread
From: Eric Sunshine @ 2018-11-05  3:33 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Sixt, carenas, shoelzer, gitgitgadget, Git List,
	Johannes Schindelin

On Sun, Nov 4, 2018 at 6:26 PM Junio C Hamano <gitster@pobox.com> wrote:
> OK, thanks.  It seems that the relative silence after this message
> is a sign that the resulting patch after squashing is what everybody
> is happey with?
>
> -- >8 --
> From: Steve Hoelzer <shoelzer@gmail.com>
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> Acked-by: Steve Hoelzer <shoelzer@gmail.com>

It's not clear from this who the author is.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-05  3:33               ` Eric Sunshine
@ 2018-11-05  4:02                 ` Junio C Hamano
  0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2018-11-05  4:02 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Johannes Sixt, carenas, shoelzer, gitgitgadget, Git List,
	Johannes Schindelin

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sun, Nov 4, 2018 at 6:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>> OK, thanks.  It seems that the relative silence after this message
>> is a sign that the resulting patch after squashing is what everybody
>> is happey with?
>>
>> -- >8 --
>> From: Steve Hoelzer <shoelzer@gmail.com>
>>
>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>> Acked-by: Steve Hoelzer <shoelzer@gmail.com>
>
> It's not clear from this who the author is.

Right.  The latter should be s-o-b and the order swapped, and
probably say "Steve wrote the original version, Johannes extended
it" in the text to match.

THanks.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-04 23:26             ` Junio C Hamano
  2018-11-04 23:44               ` Randall S. Becker
  2018-11-05  3:33               ` Eric Sunshine
@ 2018-11-05  7:01               ` Johannes Sixt
  2018-11-05 20:28                 ` Johannes Sixt
  2018-11-05 22:05                 ` Johannes Schindelin
  2 siblings, 2 replies; 17+ messages in thread
From: Johannes Sixt @ 2018-11-05  7:01 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Carlo Arenas, shoelzer, gitgitgadget, git, johannes.schindelin

Am 05.11.18 um 00:26 schrieb Junio C Hamano:
> OK, thanks.  It seems that the relative silence after this message
> is a sign that the resulting patch after squashing is what everybody
> is happey with?

I'm not 100% happy. I'll resend a squashed patch, but it has to wait as 
I have to catch a train now.

Appologies for the sub-optimal submission process.

-- Hannes

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-05  7:01               ` Johannes Sixt
@ 2018-11-05 20:28                 ` Johannes Sixt
  2018-11-05 22:05                 ` Johannes Schindelin
  1 sibling, 0 replies; 17+ messages in thread
From: Johannes Sixt @ 2018-11-05 20:28 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Carlo Arenas, shoelzer, gitgitgadget, git, johannes.schindelin

Am 05.11.18 um 08:01 schrieb Johannes Sixt:
> Am 05.11.18 um 00:26 schrieb Junio C Hamano:
>> OK, thanks.  It seems that the relative silence after this message
>> is a sign that the resulting patch after squashing is what everybody
>> is happey with?
> 
> I'm not 100% happy.
I see the patch is already in next. Never mind. The patch text is fine, 
I just wanted to modify the commit message a bit.

Thanks,
-- Hannes

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
  2018-11-05  7:01               ` Johannes Sixt
  2018-11-05 20:28                 ` Johannes Sixt
@ 2018-11-05 22:05                 ` Johannes Schindelin
  1 sibling, 0 replies; 17+ messages in thread
From: Johannes Schindelin @ 2018-11-05 22:05 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Junio C Hamano, Carlo Arenas, shoelzer, gitgitgadget, git

Hi Hannes,

On Mon, 5 Nov 2018, Johannes Sixt wrote:

> Am 05.11.18 um 00:26 schrieb Junio C Hamano:
> > OK, thanks.  It seems that the relative silence after this message is
> > a sign that the resulting patch after squashing is what everybody is
> > happey with?
> 
> I'm not 100% happy. I'll resend a squashed patch, but it has to wait as
> I have to catch a train now.

Thank you for running with this.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-11-05 22:05 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-31 21:11 [PATCH 0/1] Make compat/poll safer on Windows Johannes Schindelin via GitGitGadget
2018-10-31 21:11 ` [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues Steve Hoelzer via GitGitGadget
2018-11-01 10:21   ` Johannes Sixt
2018-11-02 14:47     ` Steve Hoelzer
2018-11-02 16:43       ` Johannes Sixt
2018-11-02 17:18         ` Steve Hoelzer
2018-11-03  0:39         ` Junio C Hamano
2018-11-03  0:49           ` Junio C Hamano
2018-11-03  8:14         ` Carlo Arenas
2018-11-03 14:05           ` Johannes Sixt
2018-11-04 23:26             ` Junio C Hamano
2018-11-04 23:44               ` Randall S. Becker
2018-11-05  3:33               ` Eric Sunshine
2018-11-05  4:02                 ` Junio C Hamano
2018-11-05  7:01               ` Johannes Sixt
2018-11-05 20:28                 ` Johannes Sixt
2018-11-05 22:05                 ` Johannes Schindelin

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).