On Wed, 23 Feb 2011, Bruce Cran wrote:
On Thu, 2011-02-24 at 08:23 +1100, Bruce Evans wrote:
The bug seems to have been overflow in this calculation.
[swap_bcount * SWAP_META_PAGES * n / <non-overflowing division>]
I've attached a patch which changes 'n' to be of type vm_ooffset_t. I
think this should fix the overflow bug?
I don't like using vm_ooffset_t either. There are no offsets here, and
it's bad technique to depend on having a large type to avoid overflows
in expressions when the result type is different.
I would cast operand(s) in the expression as necessary to prevent overflow
of subexpressions. vm_pindex_t would work, but I prefer to use a type
related to the subexpressions. Not sure what that is. Maybe just
uintmax_t for safety (even that is not safe if the subexpressions have
large values). So:
(uintmax_t)swap_bcount * SWAP_META_PAGES * n / mumble.
I like to cast only the leftmost term if possible, and depend on the
larger type propagating to all subexpressions via left-to-right
evaluation. This saves a lot of casts. Here this may be sub-optimal
and we could probably delay the cast to the final multiplication, which
reduces to the same safeness as using uintmax_t for n.
Next, there is the return type to consider. I don't see why it needs
to be changed from int. The patch in the PR actually changed it to
long, while changing n to vm_offset_t. But on 32-bit machines, long
is essentially the same as int, and vm_offset_t is not much larger.
Even 32-bit machine might actually need a type larger than 32 bits to
prevent overflow in expressions like the above.
Bruce
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
