Re: [PATCH] timing.cc integer overflow

[David Fifield <david () bamsoftware com>]

On Sun, Jan 25, 2009 at 11:54:25PM +0100, Henri Doreau wrote:
> Please find attached an attempt to fix the integer overflow in the
> printStats method of ScanProgressMeter (timing.cc). Now the method
> doesn't print out negative values anymore and uses unsigned long
> integers.

Can you find a set of values that cause an overflow, to demonstrate that
the overflow is fixed in at least one case? I mean values for begin,
now, and perc_done.

Changing the type of variables from long to unsigned long makes sure the
result will always be positive but on its own it's not likely to fix any
overflow, so it might be unnecessary. But two other changes are
promising:

+  if (perc_done) { /* catch "divide by zero" attempts */
   if (!now) {
     gettimeofday(&tvtmp, NULL);
     now = (const struct timeval *) &tvtmp;
@@ -568,7 +569,7 @@

   /* OK, now lets estimate the time to finish */
   time_used_ms = TIMEVAL_MSEC_SUBTRACT(*now, begin);
-  time_needed_ms = (int) ((double) time_used_ms / perc_done);
+  time_needed_ms = (unsigned long) ((double) time_used_ms / perc_done);

If perc_done == 0.0, then the assignment of time_needed_ms would divide
by zero, which when cast to an integer, I think, results in the largest
possible integer, which would easily cause erroneous results later on.
Also the (int) cast was clearly wrong because time_needed_ms was a long.

I like to make sure that a bug is reproducible before it's fixed, so
that the fix can be shown to work in at least one case. If you can, find
values that cause an overflow and send them in.

David Fifield

_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://SecLists.Org