Make compat/poll safer on Windows
cover Make compat/poll safer on Windows
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
Signature · 1 line
gitgitgadget
PATCH 1/1 poll: use GetTickCount64() to avoid wrap-around issues
From: Steve Hoelzer <[email protected]>
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.
--- compat/poll/poll.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
compat/poll/poll.c +7 -3
@@ -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; }
Signature · 1 line
gitgitgadget
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
Quoted 54 lines
From: Steve Hoelzer <[email protected]>
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 <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- compat/poll/poll.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)compat/poll/poll.c +6
-2@@ -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)
compat/poll/poll.c restart: · line 621
Quoted 5 lines
compat/poll/poll.c +1
-1@@ -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 ;)
compat/poll/poll.c restart: · line 624
Quoted 3 lines
compat/poll/poll.c +0
-0timeout = elapsed >= orig_timeout ?0 : orig_timeout - elapsed; }
-- Hannes
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Quoted 68 lines · Johannes Sixt
Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
Quoted 54 lines
From: Steve Hoelzer <[email protected]>
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 <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- compat/poll/poll.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)compat/poll/poll.c +6
-2@@ -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)compat/poll/poll.c restart: · line 621
Quoted 5 lines
compat/poll/poll.c +1
-1@@ -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.
Quoted 7 lines
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
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Am 02.11.18 um 15:47 schrieb Steve Hoelzer:
Quoted 20 lines · Johannes Sixt
Quoted 19 lines
Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
Review restart: · line 621
Quoted 6 lines
(quoted diff) +1
-1@@ -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.
Quoted 6 lines
Quoted 3 lines
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.
--- compat/poll/poll.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
compat/poll/poll.c +3 -3
@@ -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)
Signature · 1 line
2.19.1.406.g1aa3f475f3
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Quoted 72 lines · Johannes Sixt
Am 02.11.18 um 15:47 schrieb Steve Hoelzer:
Quoted 20 lines · Johannes Sixt
Quoted 19 lines
Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
compat/poll/poll.c restart: · line 621
Quoted 6 lines
compat/poll/poll.c +1
-1@@ -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.
Quoted 6 lines
Quoted 3 lines
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: useGetTickCount64()to avoid wrap-around issuesThe 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 <[email protected]>--- compat/poll/poll.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)compat/poll/poll.c +3
-4@@ -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) --
Quoted 1 line
2.19.1.406.g1aa3f475f3
I like it. This still removes the warning and avoids overflow issues.
Steve
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
In reply to Johannes SixtQuoted 4 lines · Johannes Sixt
Quoted 2 lines
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.
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Quoted 8 lines · Junio C Hamano
Quoted 4 lines · Johannes Sixt
Quoted 2 lines
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.
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
In reply to Johannes SixtQuoted 2 lines · Johannes Sixt
+ timeout = elapsed >=
orig_timeout? 0 : (int)(orig_timeout - elapsed);
nitpick: cast to DWORD instead of int
Carlo
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Am 03.11.18 um 09:14 schrieb Carlo Arenas:
Quoted 5 lines · Johannes Sixt
Quoted 2 lines
+ 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
Junio C Hamano
Acked-by Steve HoelzerRe: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Quoted 11 lines · Johannes Sixt
Am 03.11.18 um 09:14 schrieb Carlo Arenas:
Quoted 5 lines · Johannes Sixt
Quoted 2 lines
+ 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 <[email protected]>
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.
--- compat/poll/poll.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
compat/poll/poll.c +8 -4
@@ -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)
Signature · 1 line
2.19.1-816-gcd69ec8cde
RE: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Quoted 8 lines · Junio C Hamano,
Quoted 6 lines · Johannes Sixt
Am 03.11.18 um 09:14 schrieb Carlo Arenas:
Quoted 5 lines · Johannes Sixt
Quoted 3 lines
+ timeout = elapsed >=
orig_timeout? 0 : (int)(orig_timeout -
+ elapsed);
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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
Quoted 1 line
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
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
In reply to Junio C HamanoQuoted 9 lines · 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?
-- >8 --
From: Steve Hoelzer <[email protected]>Signed-off-by Johannes Sixt <[email protected]>Acked-by Steve Hoelzer <[email protected]>
It's not clear from this who the author is.
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Quoted 5 lines · Eric Sunshine
Quoted 4 lines · 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?
Quoted 7 lines
Quoted 5 lines
-- >8 --
From: Steve Hoelzer <[email protected]>Signed-off-by Johannes Sixt <[email protected]>Acked-by Steve Hoelzer <[email protected]>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.
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
In reply to Junio C HamanoAm 05.11.18 um 00:26 schrieb Junio C Hamano:
Quoted 3 lines
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
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Am 05.11.18 um 08:01 schrieb Johannes Sixt:
Quoted 6 lines
Am 05.11.18 um 00:26 schrieb Junio C Hamano:
Quoted 3 lines
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
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
In reply to Johannes SixtHi Hannes,
Quoted 7 lines · Johannes Sixt
Am 05.11.18 um 00:26 schrieb Junio C Hamano:
Quoted 3 lines
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