Resolve segfault in wsialloc_delete

A segfault can occur if wsialloc_delete is called but m_wsi_allocator's
ion field has not been allocated yet, as the function attempts to access
the ion properties.

This commit resolves the issue by changing m_wsi_allocator to an opaque
struct and removing the fd property, which was not being used.
wsialloc_new is then changed to return a pointer to the struct and
the swapchain then carries this pointer around, which will be
initialised to nullptr in the swapchain constructor.

Change-Id: I202e967cff4555babf0ddcd387275b74470b09d5
Signed-off-by: Dennis Tsiang <dennis.tsiang@arm.com>

util/wsialloc/wsialloc.h

@@ -61,16 +61,12 @@ extern "C" {
  */
 
 /**
- * @brief Union for allocator type.
+ * @brief Struct for allocator type.
  *
- * Allocators are usually file descriptors of the allocating device. However,
- * this interface provides the possibility to define private allocator structures.
+ * Represents a private allocator structures.
  */
-typedef union wsialloc_allocator
-{
-   void *ptr;
-   intptr_t fd;
-} wsialloc_allocator;
+struct wsialloc_allocator;
+typedef struct wsialloc_allocator wsialloc_allocator;
 
 /**
  * @brief Allocate and initialize a new WSI Allocator from an existing file descriptor.
@@ -83,11 +79,10 @@ typedef union wsialloc_allocator
  * @note The underlying implementation may choose to use @p external_fd or its own platform-specific allocation method.
  *
  * @param      external_fd file descriptor that the WSI Allocator could use for allocating new buffers
- * @param[out] allocator   a valid allocator for use in wsialloc functions
- * @retval     0           indicates success in creating the allocator
- * @retval     non-zero    indicates an error
+ *
+ * @return     Pointer to a wsialloc_allocator struct. A NULL return value indicates an error.
  */
-int wsialloc_new(int external_fd, wsialloc_allocator *allocator);
+wsialloc_allocator *wsialloc_new(int external_fd);
 
 /**
  * @brief Close down and free resources associated with a WSI Allocator

util/wsialloc/wsialloc_ion.c

@@ -42,7 +42,7 @@
 /** Default alignment */
 #define WSIALLOCP_MIN_ALIGN_SZ (64u)
 
-struct ion_allocator
+struct wsialloc_allocator
 {
    /* File descriptor of /dev/ion. */
    int fd;
@@ -103,46 +103,37 @@ static uint64_t round_size_up_to_align(uint64_t size)
    return (size + WSIALLOCP_MIN_ALIGN_SZ - 1) & ~(WSIALLOCP_MIN_ALIGN_SZ - 1);
 }
 
-int wsialloc_new(int external_fd, wsialloc_allocator *allocator)
+wsialloc_allocator *wsialloc_new(int external_fd)
 {
    UNUSED(external_fd);
-   assert(allocator != NULL);
 
-   struct ion_allocator *ion = NULL;
-   int ret = 0;
-
-   allocator->ptr = ion = malloc(sizeof(*ion));
+   wsialloc_allocator *ion = NULL;
+   ion = malloc(sizeof(*ion));
    if (NULL == ion)
    {
-      ret = -ENOMEM;
       goto fail;
    }
 
    ion->fd = open("/dev/ion", O_RDONLY);
    if (ion->fd < 0)
    {
-      ret = -errno;
       goto fail;
    }
 
    ion->alloc_heap_id = find_alloc_heap_id(ion->fd);
    if (ion->alloc_heap_id < 0)
    {
-      ret = ion->alloc_heap_id;
       goto fail;
    }
 
-   return 0;
+   return ion;
 fail:
-   wsialloc_delete(allocator);
-   return ret;
+   wsialloc_delete(ion);
+   return NULL;
 }
 
-int wsialloc_delete(wsialloc_allocator *allocator)
+int wsialloc_delete(wsialloc_allocator *ion)
 {
-   assert(allocator != NULL);
-
-   struct ion_allocator *ion = allocator->ptr;
    int ret = 0;
 
    if (NULL == ion)
@@ -159,8 +150,6 @@ int wsialloc_delete(wsialloc_allocator *allocator)
    }
 
    free(ion);
-   allocator->ptr = NULL;
-
    return ret;
 }
 
@@ -205,7 +194,7 @@ static int wsiallocp_get_fmt_info(uint32_t fourcc_fmt, uint32_t *nr_planes, uint
 }
 
 int wsialloc_alloc(
-    wsialloc_allocator *allocator,
+    wsialloc_allocator *ion,
     uint32_t fourcc,
     uint32_t width,
     uint32_t height,
@@ -214,7 +203,7 @@ int wsialloc_alloc(
     uint32_t *offset,
     const uint64_t *modifier)
 {
-   assert(allocator != NULL);
+   assert(ion != NULL);
    assert(fourcc != 0);
    assert(width > 0);
    assert(height > 0);
@@ -222,8 +211,6 @@ int wsialloc_alloc(
    assert(new_fd != NULL);
    assert(offset != NULL);
 
-   struct ion_allocator *ion = allocator->ptr;
-
    if (modifier != NULL && *modifier != 0)
    {
       return -ENOTSUP;

wsi/wayland/swapchain.cpp

@@ -91,7 +91,7 @@ swapchain::swapchain(layer::device_private_data &dev_data, const VkAllocationCal
    , m_dmabuf_interface(nullptr)
    , m_surface_queue(nullptr)
    , m_buffer_queue(nullptr)
-   , m_wsi_allocator()
+   , m_wsi_allocator(nullptr)
    , m_present_pending(false)
 {
 }
@@ -101,11 +101,12 @@ swapchain::~swapchain()
    int res;
    teardown();
 
-   res = wsialloc_delete(&m_wsi_allocator);
+   res = wsialloc_delete(m_wsi_allocator);
    if (res != 0)
    {
       WSI_LOG_ERROR("error deleting the allocator: %d", res);
    }
+   m_wsi_allocator = nullptr;
    if (m_surface_queue != nullptr)
    {
       wl_event_queue_destroy(m_surface_queue);
@@ -178,8 +179,8 @@ VkResult swapchain::init_platform(VkDevice device, const VkSwapchainCreateInfoKH
       return VK_ERROR_INITIALIZATION_FAILED;
    }
 
-   res = wsialloc_new(-1, &m_wsi_allocator);
-   if (res != 0)
+   m_wsi_allocator = wsialloc_new(-1);
+   if (nullptr == m_wsi_allocator)
    {
       WSI_LOG_ERROR("Failed to create wsi allocator.");
       return VK_ERROR_INITIALIZATION_FAILED;
@@ -433,7 +434,7 @@ VkResult swapchain::allocate_image(VkImageCreateInfo &image_create_info, wayland
       const auto fourcc = util::drm::vk_to_drm_format(image_create_info.format);
 
       const auto res =
-         wsialloc_alloc(&m_wsi_allocator, fourcc, image_create_info.extent.width, image_create_info.extent.height,
+         wsialloc_alloc(m_wsi_allocator, fourcc, image_create_info.extent.width, image_create_info.extent.height,
                         image_data->stride, image_data->buffer_fd, image_data->offset, nullptr);
       if (res != 0)
       {

wsi/wayland/swapchain.hpp

@@ -118,7 +118,7 @@ private:
    /**
     * @brief Handle to the WSI allocator.
     */
-   wsialloc_allocator m_wsi_allocator;
+   wsialloc_allocator *m_wsi_allocator;
 
    /**
     * @brief true when waiting for the server hint to present a buffer