Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsafe bounds checking is off-by-one in sgrproj_box_ab_internal #3161

Open
CodesInChaos opened this issue Mar 18, 2023 · 0 comments
Open

Unsafe bounds checking is off-by-one in sgrproj_box_ab_internal #3161

CodesInChaos opened this issue Mar 18, 2023 · 0 comments
Labels

Comments

@CodesInChaos
Copy link
Contributor

CodesInChaos commented Mar 18, 2023

The function sgrproj_box_ab_internal looks like this:

  pub(crate) fn sgrproj_box_ab_internal<const BD: usize>(
    r: usize, af: &mut [u32], bf: &mut [u32], iimg: &[u32], iimg_sq: &[u32],
    iimg_stride: usize, start_x: usize, y: usize, stripe_w: usize, s: u32,
  ) {
    let d: usize = r * 2 + 1;
    let n: usize = d * d;
    let one_over_n = if r == 1 { 455 } else { 164 };

    assert!(iimg.len() > (y + d) * iimg_stride + stripe_w + d);
    assert!(iimg_sq.len() > (y + d) * iimg_stride + stripe_w + d);
    assert!(af.len() > stripe_w);
    assert!(bf.len() > stripe_w);

    for x in start_x..stripe_w + 2 {
      // SAFETY: We perform the bounds checks above, once for the whole loop
      unsafe {
        let sum = get_integral_square(iimg, iimg_stride, x, y, d);
        let ssq = get_integral_square(iimg_sq, iimg_stride, x, y, d);
        let (reta, retb) =
          sgrproj_sum_finish::<BD>(ssq, sum, n as u32, one_over_n, s);
        *af.get_unchecked_mut(x) = reta;
        *bf.get_unchecked_mut(x) = retb;
      }
    }
  }

The safety comment claims that the bounds are checked above. The safety condition required by af.get_unchecked_mut(x) is x < af.len(). The maximum value x can have is stripe_w + 2 (exclusive) or stripe_w + 1 (inclusive).

However the assertion only checked that stripe_w < bf.len(), not stripe_w + 1 < bf.len(), so this function doesn't actually ensure its safety preconditions, and is thus unsound.

The same issue applies to all 4 of these assertions. Luckily the stricter assertions do not fail in the unit test suite, so it looks like a theoretical soundness issue, not a practical problem.


An additional complication are integer overflows. If the calculation inside the assertion overflows, the assertion can pass, while the index can still be out of bounds for a smaller x. This currently affects only the first two assertions, but would also affect a "fixed" assertion like assert!(af.len() > stripe_w + 1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant