Possible unsound safe function

[charlesxsh]

Hi there,

at crate, tract-core, version 0.21.7 at file /src/ops/cnn/patches.rs

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct ZoneScanner<'p> {
    pub patch: &'p Patch,
    pub zone: &'p Zone,
    pub output_offset: isize,
    pub output_coords: Box<[usize]>,
    pub input_center_offset: isize,
    pub inner_loop_axis: usize,
    pub inner_loop_len: usize,
    pub inner_loop_output_range: Range<usize>,
    pub inner_loop_output_stride: isize,
    pub inner_loop_input_full_stride: isize,
    pub done: bool,
}

impl<'p> ZoneScanner<'p> {
    ....
    #[inline]
    pub fn next(&mut self) {
        let inner_loop_axis = self.inner_loop_axis;
        unsafe {
            *self.output_coords.get_unchecked_mut(inner_loop_axis) += 1;
    ...

The inner_loop_axis is depends on self.inner_loop_axis, which is a public field of ZoneScanner. We can easily have memory issue by having invalid inner_loop_axis when dereferencing result of get_unchecked_mut. In Rust, we should not have memory issue by merely using safe function.

Suggestion:

1. make the field private
2. add appropriate check before dereferencing.

[kali]

Hey, I'm happy to consider PRs addressing this issue. One absolute constraint being, it should come with no measurable runtime overhead on some models. I can't tell exactly which ones, but look for conv nets in the CI and the .travis directory, it will give you an idea. A tiny overhead during preparation (loading/decluttering/optimizing time) is acceptable.

I don't remember at which exact time the ZoneScanner is built, but it is bound to a lifetime, so it is build during model runtime, not preparation. Maybe you would need to split it in two, one struct holding the validated configuration that we could build ahead of time during model prep, and then instantiating the ZoneScanner at the very last instant with the actual data.

But be aware, tract is full of issues like this. The core-linalg interface, for instance, is a mine field. My approach to safety was basically that tract high level API should be safe, but I allowed myself to cut a lot of corners under the hood, and you stumbled on one. To make matters worse, the "high level tract API" I just mentioned is something that was in my brain for a long time, but it took ages to define it and put it in place. It is the bits in API/* that are used for C and python bindings.

So here you go, absolutely thrilled to consider improvements, but 1/ there are absolutely critical imperatives on performance, and 2/ ZoneScanner is a drop in a ocean.