Remove null support in SharedFileRegion
ParcelFileDescriptor does not support null (negative) file descriptors
so we remove it from SharedFileRegion and expect that a @nullable
SharedFileRegion is used for cases where a null file descriptor is
allowed.
Test: atest shmemTest
Change-Id: Icf226181cf6de9759ed3145ff6b5ad0d8fd650af
diff --git a/media/libshmem/Android.bp b/media/libshmem/Android.bp
index fae98ed..c8d2284 100644
--- a/media/libshmem/Android.bp
+++ b/media/libshmem/Android.bp
@@ -41,6 +41,7 @@
srcs: ["ShmemTest.cpp"],
shared_libs: [
"libbinder",
+ "libcutils",
"libshmemcompat",
"libshmemutil",
"libutils",
diff --git a/media/libshmem/ShmemCompat.cpp b/media/libshmem/ShmemCompat.cpp
index 5dd83f4..44fe39c 100644
--- a/media/libshmem/ShmemCompat.cpp
+++ b/media/libshmem/ShmemCompat.cpp
@@ -24,15 +24,12 @@
bool convertSharedFileRegionToIMemory(const SharedFileRegion& shmem,
sp<IMemory>* result) {
+ assert(result != nullptr);
+
if (!validateSharedFileRegion(shmem)) {
return false;
}
- if (shmem.fd.get() < 0) {
- *result = nullptr;
- return true;
- }
-
// Heap offset and size must be page aligned.
const size_t pageSize = getpagesize();
const size_t pageMask = ~(pageSize - 1);
@@ -62,16 +59,19 @@
bool convertIMemoryToSharedFileRegion(const sp<IMemory>& mem,
SharedFileRegion* result) {
+ assert(mem != nullptr);
+ assert(result != nullptr);
+
*result = SharedFileRegion();
- if (mem == nullptr) {
- return true;
- }
ssize_t offset;
size_t size;
sp<IMemoryHeap> heap = mem->getMemory(&offset, &size);
- if (heap != nullptr) {
+ if (size > 0) {
+ if (heap == nullptr) {
+ return false;
+ }
// Make sure the offset and size do not overflow from int64 boundaries.
if (size > std::numeric_limits<int64_t>::max() ||
offset > std::numeric_limits<int64_t>::max() ||
@@ -90,9 +90,33 @@
result->size = size;
result->offset = heap->getOffset() + offset;
}
-
return true;
}
+bool convertNullableSharedFileRegionToIMemory(const std::optional<SharedFileRegion>& shmem,
+ sp<IMemory>* result) {
+ assert(result != nullptr);
+
+ if (!shmem.has_value()) {
+ result->clear();
+ return true;
+ }
+
+ return convertSharedFileRegionToIMemory(shmem.value(), result);
+}
+
+bool convertNullableIMemoryToSharedFileRegion(const sp<IMemory>& mem,
+ std::optional<SharedFileRegion>* result) {
+ assert(result != nullptr);
+
+ if (mem == nullptr) {
+ result->reset();
+ return true;
+ }
+
+ result->emplace();
+ return convertIMemoryToSharedFileRegion(mem, &result->value());
+}
+
} // namespace media
} // namespace android
diff --git a/media/libshmem/ShmemTest.cpp b/media/libshmem/ShmemTest.cpp
index 4f11b51..d076ad0 100644
--- a/media/libshmem/ShmemTest.cpp
+++ b/media/libshmem/ShmemTest.cpp
@@ -17,6 +17,7 @@
#include "binder/MemoryBase.h"
#include "binder/MemoryHeapBase.h"
+#include "cutils/ashmem.h"
#include "media/ShmemCompat.h"
#include "media/ShmemUtil.h"
@@ -24,11 +25,22 @@
namespace media {
namespace {
-// Creates a SharedFileRegion instance with a null FD.
+// Creates a SharedFileRegion instance.
SharedFileRegion makeSharedFileRegion(int64_t offset, int64_t size) {
SharedFileRegion shmem;
shmem.offset = offset;
shmem.size = size;
+ int fd = ashmem_create_region("", size + offset);
+ assert(fd >= 0);
+ shmem.fd = os::ParcelFileDescriptor(base::unique_fd(fd));
+ return shmem;
+}
+
+// Creates a SharedFileRegion instance with an invalid FD.
+SharedFileRegion makeInvalidSharedFileRegion(int64_t offset, int64_t size) {
+ SharedFileRegion shmem;
+ shmem.offset = offset;
+ shmem.size = size;
return shmem;
}
@@ -46,9 +58,7 @@
EXPECT_TRUE(validateSharedFileRegion(makeSharedFileRegion(1, 2)));
EXPECT_FALSE(validateSharedFileRegion(makeSharedFileRegion(-1, 2)));
EXPECT_FALSE(validateSharedFileRegion(makeSharedFileRegion(2, -1)));
- EXPECT_TRUE(validateSharedFileRegion(makeSharedFileRegion(
- std::numeric_limits<int64_t>::max(),
- std::numeric_limits<int64_t>::max())));
+ EXPECT_FALSE(validateSharedFileRegion(makeInvalidSharedFileRegion(1, 2)));
}
TEST(ShmemTest, Conversion) {
@@ -72,12 +82,11 @@
TEST(ShmemTest, NullConversion) {
sp<IMemory> reconstructed;
{
- SharedFileRegion shmem;
+ std::optional<SharedFileRegion> shmem;
sp<IMemory> imem;
- ASSERT_TRUE(convertIMemoryToSharedFileRegion(imem, &shmem));
- ASSERT_EQ(0, shmem.size);
- ASSERT_LT(shmem.fd.get(), 0);
- ASSERT_TRUE(convertSharedFileRegionToIMemory(shmem, &reconstructed));
+ ASSERT_TRUE(convertNullableIMemoryToSharedFileRegion(imem, &shmem));
+ ASSERT_FALSE(shmem.has_value());
+ ASSERT_TRUE(convertNullableSharedFileRegionToIMemory(shmem, &reconstructed));
}
ASSERT_EQ(nullptr, reconstructed);
}
diff --git a/media/libshmem/ShmemUtil.cpp b/media/libshmem/ShmemUtil.cpp
index a6d047f..e075346 100644
--- a/media/libshmem/ShmemUtil.cpp
+++ b/media/libshmem/ShmemUtil.cpp
@@ -19,6 +19,11 @@
namespace media {
bool validateSharedFileRegion(const SharedFileRegion& shmem) {
+ // FD must be valid.
+ if (shmem.fd.get() < 0) {
+ return false;
+ }
+
// Size and offset must be non-negative.
if (shmem.size < 0 || shmem.offset < 0) {
return false;
diff --git a/media/libshmem/aidl/android/media/SharedFileRegion.aidl b/media/libshmem/aidl/android/media/SharedFileRegion.aidl
index c99ad95..a910e69 100644
--- a/media/libshmem/aidl/android/media/SharedFileRegion.aidl
+++ b/media/libshmem/aidl/android/media/SharedFileRegion.aidl
@@ -20,13 +20,15 @@
* A shared file region.
*
* This type contains the required information to share a region of a file between processes over
- * AIDL. An invalid (null) region may be represented using a negative file descriptor.
+ * AIDL.
+ * An instance of this type represents a valid FD. For representing a null SharedFileRegion, use a
+ * @nullable SharedFileRegion.
* Primarily, this is intended for shared memory blocks.
*
* @hide
*/
parcelable SharedFileRegion {
- /** File descriptor of the region. */
+ /** File descriptor of the region. Must be valid. */
ParcelFileDescriptor fd;
/** Offset, in bytes within the file of the start of the region. Must be non-negative. */
long offset;
diff --git a/media/libshmem/include/media/ShmemCompat.h b/media/libshmem/include/media/ShmemCompat.h
index 3bf7f67..ba59f25 100644
--- a/media/libshmem/include/media/ShmemCompat.h
+++ b/media/libshmem/include/media/ShmemCompat.h
@@ -19,6 +19,8 @@
// This module contains utilities for interfacing between legacy code that is using IMemory and new
// code that is using android.os.SharedFileRegion.
+#include <optional>
+
#include "android/media/SharedFileRegion.h"
#include "binder/IMemory.h"
#include "utils/StrongPointer.h"
@@ -29,8 +31,7 @@
/**
* Converts a SharedFileRegion parcelable to an IMemory instance.
* @param shmem The SharedFileRegion instance.
- * @param result The resulting IMemory instance, or null of the SharedFileRegion is null (has a
- * negative FD).
+ * @param result The resulting IMemory instance. May not be null.
* @return true if the conversion is successful (should always succeed under normal circumstances,
* failure usually means corrupt data).
*/
@@ -38,8 +39,19 @@
sp<IMemory>* result);
/**
+ * Converts a nullable SharedFileRegion parcelable to an IMemory instance.
+ * @param shmem The SharedFileRegion instance.
+ * @param result The resulting IMemory instance. May not be null. Pointee assigned to null,
+ * if the input is null.
+ * @return true if the conversion is successful (should always succeed under normal circumstances,
+ * failure usually means corrupt data).
+ */
+bool convertNullableSharedFileRegionToIMemory(const std::optional<SharedFileRegion>& shmem,
+ sp<IMemory>* result);
+
+/**
* Converts an IMemory instance to SharedFileRegion.
- * @param mem The IMemory instance. May be null.
+ * @param mem The IMemory instance. May not be null.
* @param result The resulting SharedFileRegion instance.
* @return true if the conversion is successful (should always succeed under normal circumstances,
* failure usually means corrupt data).
@@ -47,5 +59,16 @@
bool convertIMemoryToSharedFileRegion(const sp<IMemory>& mem,
SharedFileRegion* result);
+/**
+ * Converts a nullable IMemory instance to a nullable SharedFileRegion.
+ * @param mem The IMemory instance. May be null.
+ * @param result The resulting SharedFileRegion instance. May not be null. Assigned to empty,
+ * if the input is null.
+ * @return true if the conversion is successful (should always succeed under normal circumstances,
+ * failure usually means corrupt data).
+ */
+bool convertNullableIMemoryToSharedFileRegion(const sp<IMemory>& mem,
+ std::optional<SharedFileRegion>* result);
+
} // namespace media
} // namespace android
diff --git a/media/libshmem/include/media/ShmemUtil.h b/media/libshmem/include/media/ShmemUtil.h
index 563cb71..3a7a5a5 100644
--- a/media/libshmem/include/media/ShmemUtil.h
+++ b/media/libshmem/include/media/ShmemUtil.h
@@ -25,7 +25,6 @@
/**
* Checks whether a SharedFileRegion instance is valid (all the fields have sane values).
- * A null SharedFileRegion (having a negative FD) is considered valid.
*/
bool validateSharedFileRegion(const SharedFileRegion& shmem);