Web lists-archives.com

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




Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
From: Steve Hoelzer <shoelzer@xxxxxxxxx>

 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@xxxxxxxxx>
Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
---
  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