simdeez icon indicating copy to clipboard operation
simdeez copied to clipboard

Load and store methods use &T but read/write out of bounds

Open nico-abram opened this issue 3 years ago • 0 comments

The following methods:

  • load_ps
  • load_pd
  • load_epi16
  • load_epi32
  • load_epi64
  • loadu_ps
  • loadu_pd
  • loadu_epi32
  • loadu_epi64

Take a &T (Ex. a &f32) and read out of bounds to load a vector.

The following methods:

  • store_ps
  • store_pd
  • store_epi32
  • store_epi64
  • storeu_ps
  • storeu_pd
  • storeu_epi32
  • storeu_epi64

Are the same but stores which write out of bounds of a &mut T.

It is not entirely clear, but if you see the discussion in https://github.com/rust-lang/unsafe-code-guidelines/issues/134 and https://github.com/rust-lang/unsafe-code-guidelines/issues/134 , it looks like reading/writing past the end of a &T might be UB.

Also, a reference has an alignment requirement, which may be stricter than wanted. The alignment requirement is that the &T ptr is aligned to align_of::<T>(), which is not enough to use the aligned load/write variants, but can certainly be more strict than needed for the unaligned loads.

I propose switching to raw pointers. This avoids the possibility of Undefined Behaviour from reading past the end of the reference and the unnecessary alignment requirements on the unaligned variants. Code like this from the example in the readme:

            let xv1 = S::loadu_ps(&x1[0]);
            let yv1 = S::loadu_ps(&y1[0]);
            let xv2 = S::loadu_ps(&x2[0]);
            let yv2 = S::loadu_ps(&y2[0]);

Would still compile after that change, since a &T coerces to *const T, but it would remain UB because that pointer has the same provenance/tag, but it could now be fixed by using the correct pointer:

            let xv1 = S::loadu_ps(x1.as_ptr());
            let yv1 = S::loadu_ps(y1.as_ptr());
            let xv2 = S::loadu_ps(x2.as_ptr());
            let yv2 = S::loadu_ps(y2.as_ptr());

nico-abram avatar Dec 23 '21 17:12 nico-abram