zig icon indicating copy to clipboard operation
zig copied to clipboard

std: Avoid overflowing in the midpoint calculation in upperBound

Open g2mt opened this issue 1 year ago • 4 comments

The line that calculates the midpoint in the upperBound function didn't take into account overflows from large arrays like in other binary search functions.

g2mt avatar May 22 '24 01:05 g2mt

Is it possible to add a regression test that would ensure this doesn't break again?

marler8997 avatar May 22 '24 01:05 marler8997

How should I write the unit test? Triggering an overflow requires the left and right indices add up to greater than max of usize. Allocating a 2 GB array to test it seems feasible for a 32-bit system, but I'm not sure if you can allocate 2^63 bytes on most 64-bit systems.

g2mt avatar May 22 '24 14:05 g2mt

There's upperBound unit tests a little further down in the code, you should be able to add your test there. No need to actually allocate that much memory.

marler8997 avatar May 22 '24 15:05 marler8997

No need to actually allocate that much memory.

Unless I'm missing something, there is; the relevant thing is items.len.

I suppose one way to get around that would be to split upperBound into two functions:

pub fn upperBound(
    comptime T: type,
    key: anytype,
    items: []const T,
    context: anytype,
    comptime lessThan: fn (context: @TypeOf(context), lhs: @TypeOf(key), rhs: T) bool,
) usize {
    return upperBound(usize, T, key, items, context, lessThan);
}

fn upperBoundImpl(
    comptime IndexType: type,
    comptime T: type,
    key: anytype,
    items: []const T,
    context: anytype,
    comptime lessThan: fn (context: @TypeOf(context), lhs: @TypeOf(key), rhs: T) bool,
) usize {
    var left: IndexType = 0;
    var right: IndexType = @intCast(items.len);

    while (left < right) {
        const mid = left + (right - left) / 2;
        if (!lessThan(context, key, items[mid])) {
            left = mid + 1;
        } else {
            right = mid;
        }
    }

    return left;
}

so then upperBoundImpl could be tested with a smaller IndexType. Not sure if there's a use-case for this outside of testing for the overflow, though.

squeek502 avatar May 23 '24 10:05 squeek502

Upon second look, I'm happy to merge this as-is. Thanks @g2mt

andrewrk avatar May 29 '24 00:05 andrewrk