Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit e817f397 authored by Ytai Ben-Tsvi's avatar Ytai Ben-Tsvi
Browse files

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
parent 9bcf4989

media/libshmem/Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -41,6 +41,7 @@ cc_test { 
    srcs: ["ShmemTest.cpp"],

    shared_libs: [

        "libbinder",

        "libcutils",

        "libshmemcompat",

        "libshmemutil",

        "libutils",

media/libshmem/ShmemCompat.cpp

+34 −10
Original line number Diff line number Diff line
@@ -24,15 +24,12 @@ namespace media { 


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 convertSharedFileRegionToIMemory(const SharedFileRegion& shmem, 


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 @@ bool convertIMemoryToSharedFileRegion(const sp<IMemory>& mem, 
        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

media/libshmem/ShmemTest.cpp

+18 −9
Original line number Diff line number Diff line
@@ -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,8 +25,19 @@ namespace android { 
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;
@@ -46,9 +58,7 @@ TEST(ShmemTest, Validate) { 
    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, Conversion) { 
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);

}

media/libshmem/ShmemUtil.cpp

+5 −0
Original line number Diff line number Diff line
@@ -19,6 +19,11 @@ namespace android { 
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;

media/libshmem/aidl/android/media/SharedFileRegion.aidl

+4 −2
Original line number Diff line number Diff line
@@ -20,13 +20,15 @@ package android.media; 
 * 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;