Fix a nasty race condition.

This commit reflects Dev's discovery of the problem -- thanks Dev.

In these few instances of `Option<DevicePointer<GpuFloat>>`, the pointer within
a `DevicePointer` was being taken, but then the `DevicePointer` itself was
consumed, so if the relevant memory was altered, the code did not behave as
intended. Curiously, this never led to a segfault despite being a use after
free, and witnessing unintended behaviour seemed to require running many GPU
tests concurrently.

The code is now simpler and less prone to error; don't have an
`Option<DevicePointer>`, just make a `DevicePointer` and if it has no contents,
it will give out a null pointer anyway.

CHANGELOG.md

@@ -6,8 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic
 Versioning](https://semver.org/spec/v2.0.0.html).
 
-## [0.7.1] - unreleased
+## [0.7.1] - 2024-01-19
 ### Fixed
+- A (seemingly rare) race condition in FEE GPU code.
 - examples/analytic_cuda_device.cu wasn't complete.
 
 ## [0.7.0] - 2023-10-31

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "mwa_hyperbeam"
-version = "0.7.0"
+version = "0.7.1"
 authors = [
     "Christopher H. Jordan <christopherjordan87@gmail.com>",
     "Jack L. B. Line <jack.line@curtin.edu.au>",

src/fee/ffi/mod.rs

@@ -599,15 +599,17 @@ pub unsafe extern "C" fn fee_calc_jones_gpu_device(
     let za = slice::from_raw_parts(za_rad, num_azza as usize);
     let d_az = ffi_error!(DevicePointer::copy_to_device(az));
     let d_za = ffi_error!(DevicePointer::copy_to_device(za));
-    let d_latitude_rad = ffi_error!(latitude_rad
-        .as_ref()
-        .map(|f| DevicePointer::copy_to_device(&[*f as GpuFloat]))
-        .transpose());
+    let d_latitude_rad = match latitude_rad.as_ref() {
+        Some(f) => ffi_error!(DevicePointer::copy_to_device(&[*f as GpuFloat])),
+
+        // This won't allocate and accessing the pointer will give a null ptr.
+        None => DevicePointer::default(),
+    };
     ffi_error!(beam.calc_jones_device_pair_inner(
         d_az.get(),
         d_za.get(),
         num_azza,
-        d_latitude_rad.map(|p| p.get()).unwrap_or(std::ptr::null()),
+        d_latitude_rad.get(),
         iau_bool,
         d_jones.cast()
     ));

src/fee/gpu/mod.rs

@@ -394,15 +394,19 @@ impl FEEBeamGpu {
             let d_zas = DevicePointer::copy_to_device(&zas)?;
 
             // Allocate the latitude if we have to.
-            let d_latitude_rad = latitude_rad
-                .map(|f| DevicePointer::copy_to_device(&[f as GpuFloat]))
-                .transpose()?;
+            let d_latitude_rad = match latitude_rad {
+                Some(f) => DevicePointer::copy_to_device(&[f as GpuFloat])?,
+
+                // This won't allocate and accessing the pointer will give a
+                // null ptr.
+                None => DevicePointer::default(),
+            };
 
             self.calc_jones_device_pair_inner(
                 d_azs.get(),
                 d_zas.get(),
                 azels.len().try_into().expect("much fewer than i32::MAX"),
-                d_latitude_rad.map(|p| p.get()).unwrap_or(std::ptr::null()),
+                d_latitude_rad.get(),
                 iau_reorder,
                 d_results.get_mut() as *mut std::ffi::c_void,
             )?;
@@ -433,15 +437,19 @@ impl FEEBeamGpu {
             let d_zas = DevicePointer::copy_to_device(za_rad)?;
 
             // Allocate the latitude if we have to.
-            let d_latitude_rad = latitude_rad
-                .map(|f| DevicePointer::copy_to_device(&[f as GpuFloat]))
-                .transpose()?;
+            let d_latitude_rad = match latitude_rad {
+                Some(f) => DevicePointer::copy_to_device(&[f as GpuFloat])?,
+
+                // This won't allocate and accessing the pointer will give a
+                // null ptr.
+                None => DevicePointer::default(),
+            };
 
             self.calc_jones_device_pair_inner(
                 d_azs.get(),
                 d_zas.get(),
                 az_rad.len().try_into().expect("much fewer than i32::MAX"),
-                d_latitude_rad.map(|p| p.get()).unwrap_or(std::ptr::null()),
+                d_latitude_rad.get(),
                 iau_reorder,
                 d_results.get_mut() as *mut std::ffi::c_void,
             )?;

src/gpu.rs

@@ -61,13 +61,17 @@ impl<T> DevicePointer<T> {
     /// attempt to catch problems but there are no guarantees.
     #[track_caller]
     pub unsafe fn malloc(size: usize) -> Result<DevicePointer<T>, GpuError> {
-        let mut d_ptr = std::ptr::null_mut();
-        gpuMalloc(&mut d_ptr, size);
-        check_for_errors(GpuCall::Malloc)?;
-        Ok(Self {
-            ptr: d_ptr.cast(),
-            num_elements: size / std::mem::size_of::<T>(),
-        })
+        if size == 0 {
+            Ok(Self::default())
+        } else {
+            let mut d_ptr = std::ptr::null_mut();
+            gpuMalloc(&mut d_ptr, size);
+            check_for_errors(GpuCall::Malloc)?;
+            Ok(Self {
+                ptr: d_ptr.cast(),
+                num_elements: size / std::mem::size_of::<T>(),
+            })
+        }
     }
 
     /// Get the number of elements of `T` that have been allocated on the
@@ -160,6 +164,15 @@ impl<T> Drop for DevicePointer<T> {
     }
 }
 
+impl<T> Default for DevicePointer<T> {
+    fn default() -> Self {
+        Self {
+            ptr: std::ptr::null_mut(),
+            num_elements: 0,
+        }
+    }
+}
+
 #[derive(Error, Debug)]
 pub enum GpuError {
     #[error("When overwriting, the new amount of memory did not equal the old amount")]