Fix error handling in CursorWindow

If a CursorWindow is created from a Parcel but the file descriptor is
invalid or the mmap() call fails, the CursorWindow destructor will be
invoked with an inconsistent object.  The destructor will attempt to
call munmap() with an unconfigured mData attribute.

This makes two changes.  First, the destructor only attempts to unmap
memory if the file descriptor is non-negative.  The old code unmapped
memory if the file descriptor was not -1.  However, the construction
code treated any negative file descriptor as an error.

Second, when a CursorWindow is created from a parcel, the file
descriptor field is not changed from -1 (the constructor default)
until it is known that the memory mapping has succeeded.  If the
function exits early, it is no longer in a half-constructed state.  As
a bonus, if mmap() fails in the method, the duplicated file descriptor
is closed rather than leaked.

Flag: EXEMPT bug-fix
Bug: 330108840
Test: atest
 * libandroidfw_tests
Change-Id: I94a9240dbd06b255bff9491ea390b485bac3cb88

1 files changed, 10 insertions, 6 deletions

diff --git a/libs/androidfw/CursorWindow.cpp b/libs/androidfw/CursorWindow.cpp
index 5e645cceea2d..a592749c5398 100644
--- a/libs/androidfw/CursorWindow.cpp
+++ b/libs/androidfw/CursorWindow.cpp
@@ -38,7 +38,7 @@ CursorWindow::CursorWindow() {
 }
 
 CursorWindow::~CursorWindow() {
-    if (mAshmemFd != -1) {
+    if (mAshmemFd >= 0) {
         ::munmap(mData, mSize);
         ::close(mAshmemFd);
     } else {
@@ -155,23 +155,27 @@ status_t CursorWindow::createFromParcel(Parcel* parcel, CursorWindow** outWindow
     bool isAshmem;
     if (parcel->readBool(&isAshmem)) goto fail;
     if (isAshmem) {
-        window->mAshmemFd = parcel->readFileDescriptor();
-        if (window->mAshmemFd < 0) {
+        int tempFd = parcel->readFileDescriptor();
+        if (tempFd < 0) {
             LOG(ERROR) << "Failed readFileDescriptor";
             goto fail_silent;
         }
 
-        window->mAshmemFd = ::fcntl(window->mAshmemFd, F_DUPFD_CLOEXEC, 0);
-        if (window->mAshmemFd < 0) {
+        tempFd = ::fcntl(tempFd, F_DUPFD_CLOEXEC, 0);
+        if (tempFd < 0) {
             PLOG(ERROR) << "Failed F_DUPFD_CLOEXEC";
             goto fail_silent;
         }
 
-        window->mData = ::mmap(nullptr, window->mSize, PROT_READ, MAP_SHARED, window->mAshmemFd, 0);
+        window->mData = ::mmap(nullptr, window->mSize, PROT_READ, MAP_SHARED, tempFd, 0);
         if (window->mData == MAP_FAILED) {
+            ::close(tempFd);
             PLOG(ERROR) << "Failed mmap";
             goto fail_silent;
         }
+
+        window->mAshmemFd = tempFd;
+
     } else {
         window->mAshmemFd = -1;