Re: [PATCH] timing.cc integer overflow

[David Fifield <david () bamsoftware com>]

On Mon, Feb 09, 2009 at 03:03:06PM -0700, David Fifield wrote:
> On Mon, Feb 09, 2009 at 08:19:06PM +0100, Henri Doreau wrote:
> > > Thanks, I have committed this patch. However I think there's still some
> > > work to do. The member function ScanProgressMeter::printStatsIfNecessary
> > > does some of the same calculations as ScanProgressMeter::printStats, and
> > > it still does them with longs. I don't know what the effect would be,
> > > but it could cause the stats not to be printed when they should be, or
> > > vice versa. Can you look at that function and do the calculations in the
> > > same way, or (even better) remove the duplicated code into a separate
> > > function?
> > I have added a (public) method: ScanProgressMeter::estimateTimeLeft that
> > returns a double. It "factorizes" the duplicated code for both
> > ScanProgressMeter::printStatsIfNecessary and ScanProgressMeter::printStats.
> > So the patch contains a modified version of
> > ScanProgressMeter::printStatsIfNecessary, and the same way,
> > ScanProgressMeter::mayBePrinted was changed to compute things in seconds
> > only.
>
> The patch looks reasonable but I'm still checking it out.
>
> Can you elaborate on this comment about the ABS macro?
>
> -    prev_est_time_left_ms = TIMEVAL_MSEC_SUBTRACT(last_est, *now);
> -    change_abs_ms = ABS(prev_est_time_left_ms - time_left_ms);
> -    if (prev_est_time_left_ms <= 0)
> +    prev_est_time_left_s = difftime(last_est.tv_sec, now->tv_sec);
> +
> +    /* the ABS() macro produces strange results with doubles */
> +    if ((change_abs_s = prev_est_time_left_s - time_left_s) < 0)
> +        change_abs_s *= -1;
> +
> +    if (prev_est_time_left_s <= 0)
>
> The reason I ask is that ABS seems to be broken with any data type. It's
> defined in nbase/nbase.h as
>
> #define ABS(x) (((x) >= 0)?(x):(-x))
>
> but it should be
>
> #define ABS(x) (((x) >= 0)?(x):-(x))
>
> Running the ABS line in timing.cc through the preprocessor gives
>
>     change_abs_ms = (((prev_est_time_left_ms - time_left_ms) >= 0)?(prev_est_time_left_ms - 
> time_left_ms):(-prev_est_time_left_ms - time_left_ms));
>
> which will have a negative value whenever prev_est_time_left_ms <
> time_left_ms and both are positive.

I corrected that in r12085. I added some debugging code and it clearly
was giving wrong results sometimes.

David Fifield

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