gralloc4: Only free allocated handles in error path

If we encounter an error while allocating handles, some error paths
could end up trying to freeing more handles that we allocated so far.

Also add some extra checks and logging in mali_grallic_ion_allocate() to
catch incorrect calls to this function, and ensure the array of
filedescriptors in handles is initialized.

Bug: 241512108
Test: v2/android-platinum/health/unit/camera
Change-Id: Idd2b09ed11424b110d485e10656771aafe0b20a9

2 files changed, 39 insertions, 39 deletions

diff --git a/gralloc4/src/allocator/mali_gralloc_ion.cpp b/gralloc4/src/allocator/mali_gralloc_ion.cpp
index da9aba5..2d63a70 100644
--- a/gralloc4/src/allocator/mali_gralloc_ion.cpp
+++ b/gralloc4/src/allocator/mali_gralloc_ion.cpp
@@ -48,6 +48,7 @@
 #include "mali_gralloc_ion.h"
 
 #include <array>
+#include <cassert>
 #include <string>
 
 static const char kDmabufSensorDirectHeapName[] = "sensor_direct_heap";
@@ -369,61 +370,57 @@ int mali_gralloc_ion_allocate(const gralloc_buffer_descriptor_t *descriptors,
 	unsigned int priv_heap_flag = 0;
 	uint64_t usage;
 	uint32_t i;
-	int fds[MAX_FDS];
-	std::fill(fds, fds + MAX_FDS, -1);
 
 	for (i = 0; i < numDescriptors; i++)
 	{
-		buffer_descriptor_t *bufDescriptor = (buffer_descriptor_t *)(descriptors[i]);
-		usage = bufDescriptor->consumer_usage | bufDescriptor->producer_usage;
-
-		for (int fidx = 0; fidx < bufDescriptor->fd_count; fidx++)
-		{
-			if (ion_fd >= 0 && fidx == 0) {
-				fds[fidx] = ion_fd;
-			} else {
-				fds[fidx] = alloc_from_dmabuf_heap(usage, bufDescriptor->alloc_sizes[fidx], bufDescriptor->name);
-			}
-			if (fds[fidx] < 0)
-			{
-				MALI_GRALLOC_LOGE("ion_alloc failed");
-
-				for (int cidx = 0; cidx < fidx; cidx++)
-				{
-					close(fds[cidx]);
-				}
-
-				/* need to free already allocated memory. not just this one */
-				mali_gralloc_ion_free_internal(pHandle, numDescriptors);
-
-				return -1;
-			}
-		}
+		buffer_descriptor_t *bufDescriptor = reinterpret_cast<buffer_descriptor_t *>(descriptors[i]);
+		assert(bufDescriptor);
+		assert(bufDescriptor->fd_count >= 0);
+		assert(bufDescriptor->fd_count <= MAX_FDS);
 
-		private_handle_t *hnd = new private_handle_t(
+		auto hnd = new private_handle_t(
 		    priv_heap_flag,
 		    bufDescriptor->alloc_sizes,
 		    bufDescriptor->consumer_usage, bufDescriptor->producer_usage,
-		    fds, bufDescriptor->fd_count,
+		    nullptr, bufDescriptor->fd_count,
 		    bufDescriptor->hal_format, bufDescriptor->alloc_format,
 		    bufDescriptor->width, bufDescriptor->height, bufDescriptor->pixel_stride,
 		    bufDescriptor->layer_count, bufDescriptor->plane_info);
 
-		if (NULL == hnd)
+		/* Reset the number of valid filedescriptors, we will increment
+		 * it each time a valid fd is added, so we can rely on the
+		 * cleanup functions to close open fds. */
+		hnd->set_numfds(0);
+
+		if (nullptr == hnd)
 		{
 			MALI_GRALLOC_LOGE("Private handle could not be created for descriptor:%d in non-shared usecase", i);
+			mali_gralloc_ion_free_internal(pHandle, i);
+			return -1;
+		}
+
+		pHandle[i] = hnd;
+		usage = bufDescriptor->consumer_usage | bufDescriptor->producer_usage;
+
+		for (uint32_t fidx = 0; fidx < bufDescriptor->fd_count; fidx++)
+		{
+			int& fd = hnd->fds[fidx];
 
-			/* Close the obtained shared file descriptor for the current handle */
-			for (int j = 0; j < bufDescriptor->fd_count; j++)
+			if (ion_fd >= 0 && fidx == 0) {
+				fd = ion_fd;
+			} else {
+				fd = alloc_from_dmabuf_heap(usage, bufDescriptor->alloc_sizes[fidx], bufDescriptor->name);
+			}
+
+			if (fd < 0)
 			{
-				close(fds[j]);
+				MALI_GRALLOC_LOGE("ion_alloc failed for fds[%u] = %d", fidx, fd);
+				mali_gralloc_ion_free_internal(pHandle, i + 1);
+				return -1;
 			}
 
-			mali_gralloc_ion_free_internal(pHandle, numDescriptors);
-			return -1;
+			hnd->incr_numfds(1);
 		}
-
-		pHandle[i] = hnd;
 	}
 
 #if defined(GRALLOC_INIT_AFBC) && (GRALLOC_INIT_AFBC == 1)
@@ -431,8 +428,8 @@ int mali_gralloc_ion_allocate(const gralloc_buffer_descriptor_t *descriptors,
 	unsigned char *cpu_ptr = NULL;
 	for (i = 0; i < numDescriptors; i++)
 	{
-		buffer_descriptor_t *bufDescriptor = (buffer_descriptor_t *)(descriptors[i]);
-		private_handle_t *hnd = (private_handle_t *)(pHandle[i]);
+		buffer_descriptor_t *bufDescriptor = reinterpret_cast<buffer_descriptor_t *>(descriptors[i]);
+		const private_handle_t *hnd = static_cast<const private_handle_t *>(pHandle[i]);
 
 		usage = bufDescriptor->consumer_usage | bufDescriptor->producer_usage;
 
@@ -459,6 +456,7 @@ int mali_gralloc_ion_allocate(const gralloc_buffer_descriptor_t *descriptors,
 				ATRACE_NAME("data init");
 				/* For separated plane YUV, there is a header to initialise per plane. */
 				const plane_info_t *plane_info = bufDescriptor->plane_info;
+				assert(plane_info);
 				const bool is_multi_plane = hnd->is_multi_plane();
 				for (int i = 0; i < MAX_PLANES && (i == 0 || plane_info[i].byte_stride != 0); i++)
 				{
diff --git a/gralloc4/src/mali_gralloc_buffer.h b/gralloc4/src/mali_gralloc_buffer.h
index 9cc7920..c4db9fc 100644
--- a/gralloc4/src/mali_gralloc_buffer.h
+++ b/gralloc4/src/mali_gralloc_buffer.h
@@ -286,6 +286,8 @@ struct private_handle_t
 
 		if (_fds)
 			memcpy(fds, _fds, sizeof(fds));
+		else
+			memset(fds, -1, sizeof(fds));
 
 		if (_alloc_sizes)
 			memcpy(alloc_sizes, _alloc_sizes, sizeof(alloc_sizes));