Wrap fd with unique_fd so it won't leak.

Bug: 74021345
Test: manual and atest incidentd_test
Change-Id: Ib1000bfe6917c3d5cae7b9edce5b67d50897e10d

8 files changed, 134 insertions, 148 deletions

diff --git a/cmds/incidentd/src/FdBuffer.cpp b/cmds/incidentd/src/FdBuffer.cpp
index 35701446e9d9..2b85ec08f9a6 100644
--- a/cmds/incidentd/src/FdBuffer.cpp
+++ b/cmds/incidentd/src/FdBuffer.cpp
@@ -34,11 +34,11 @@ FdBuffer::FdBuffer()
 
 FdBuffer::~FdBuffer() {}
 
-status_t FdBuffer::read(int fd, int64_t timeout) {
-    struct pollfd pfds = {.fd = fd, .events = POLLIN};
+status_t FdBuffer::read(unique_fd* fd, int64_t timeout) {
+    struct pollfd pfds = {.fd = fd->get(), .events = POLLIN};
     mStartTime = uptimeMillis();
 
-    fcntl(fd, F_SETFL, fcntl(fd, F_GETFL, 0) | O_NONBLOCK);
+    fcntl(fd->get(), F_SETFL, fcntl(fd->get(), F_GETFL, 0) | O_NONBLOCK);
 
     while (true) {
         if (mBuffer.size() >= MAX_BUFFER_COUNT * BUFFER_SIZE) {
@@ -67,16 +67,16 @@ status_t FdBuffer::read(int fd, int64_t timeout) {
                 VLOG("return event has error %s", strerror(errno));
                 return errno != 0 ? -errno : UNKNOWN_ERROR;
             } else {
-                ssize_t amt = ::read(fd, mBuffer.writeBuffer(), mBuffer.currentToWrite());
+                ssize_t amt = ::read(fd->get(), mBuffer.writeBuffer(), mBuffer.currentToWrite());
                 if (amt < 0) {
                     if (errno == EAGAIN || errno == EWOULDBLOCK) {
                         continue;
                     } else {
-                        VLOG("Fail to read %d: %s", fd, strerror(errno));
+                        VLOG("Fail to read %d: %s", fd->get(), strerror(errno));
                         return -errno;
                     }
                 } else if (amt == 0) {
-                    VLOG("Reached EOF of fd=%d", fd);
+                    VLOG("Reached EOF of fd=%d", fd->get());
                     break;
                 }
                 mBuffer.wp()->move(amt);
@@ -87,7 +87,7 @@ status_t FdBuffer::read(int fd, int64_t timeout) {
     return NO_ERROR;
 }
 
-status_t FdBuffer::readFully(int fd) {
+status_t FdBuffer::readFully(unique_fd* fd) {
     mStartTime = uptimeMillis();
 
     while (true) {
@@ -99,10 +99,10 @@ status_t FdBuffer::readFully(int fd) {
         }
         if (mBuffer.writeBuffer() == NULL) return NO_MEMORY;
 
-        ssize_t amt =
-                TEMP_FAILURE_RETRY(::read(fd, mBuffer.writeBuffer(), mBuffer.currentToWrite()));
+        ssize_t amt = TEMP_FAILURE_RETRY(
+                ::read(fd->get(), mBuffer.writeBuffer(), mBuffer.currentToWrite()));
         if (amt < 0) {
-            VLOG("Fail to read %d: %s", fd, strerror(errno));
+            VLOG("Fail to read %d: %s", fd->get(), strerror(errno));
             return -errno;
         } else if (amt == 0) {
             VLOG("Done reading %zu bytes", mBuffer.size());
@@ -116,20 +116,20 @@ status_t FdBuffer::readFully(int fd) {
     return NO_ERROR;
 }
 
-status_t FdBuffer::readProcessedDataInStream(int fd, int toFd, int fromFd, int64_t timeoutMs,
-                                             const bool isSysfs) {
+status_t FdBuffer::readProcessedDataInStream(unique_fd* fd, unique_fd* toFd, unique_fd* fromFd,
+                                             int64_t timeoutMs, const bool isSysfs) {
     struct pollfd pfds[] = {
-            {.fd = fd, .events = POLLIN},
-            {.fd = toFd, .events = POLLOUT},
-            {.fd = fromFd, .events = POLLIN},
+            {.fd = fd->get(), .events = POLLIN},
+            {.fd = toFd->get(), .events = POLLOUT},
+            {.fd = fromFd->get(), .events = POLLIN},
     };
 
     mStartTime = uptimeMillis();
 
     // mark all fds non blocking
-    fcntl(fd, F_SETFL, fcntl(fd, F_GETFL, 0) | O_NONBLOCK);
-    fcntl(toFd, F_SETFL, fcntl(toFd, F_GETFL, 0) | O_NONBLOCK);
-    fcntl(fromFd, F_SETFL, fcntl(fromFd, F_GETFL, 0) | O_NONBLOCK);
+    fcntl(fd->get(), F_SETFL, fcntl(fd->get(), F_GETFL, 0) | O_NONBLOCK);
+    fcntl(toFd->get(), F_SETFL, fcntl(toFd->get(), F_GETFL, 0) | O_NONBLOCK);
+    fcntl(fromFd->get(), F_SETFL, fcntl(fromFd->get(), F_GETFL, 0) | O_NONBLOCK);
 
     // A circular buffer holds data read from fd and writes to parsing process
     uint8_t cirBuf[BUFFER_SIZE];
@@ -166,10 +166,10 @@ status_t FdBuffer::readProcessedDataInStream(int fd, int toFd, int fromFd, int64
         for (int i = 0; i < 3; ++i) {
             if ((pfds[i].revents & POLLERR) != 0) {
                 if (i == 0 && isSysfs) {
-                    VLOG("fd %d is sysfs, ignore its POLLERR return value", fd);
+                    VLOG("fd %d is sysfs, ignore its POLLERR return value", fd->get());
                     continue;
                 }
-                VLOG("fd[%d]=%d returns error events: %s", i, fd, strerror(errno));
+                VLOG("fd[%d]=%d returns error events: %s", i, fd->get(), strerror(errno));
                 return errno != 0 ? -errno : UNKNOWN_ERROR;
             }
         }
@@ -178,17 +178,17 @@ status_t FdBuffer::readProcessedDataInStream(int fd, int toFd, int fromFd, int64
         if (cirSize != BUFFER_SIZE && pfds[0].fd != -1) {
             ssize_t amt;
             if (rpos >= wpos) {
-                amt = ::read(fd, cirBuf + rpos, BUFFER_SIZE - rpos);
+                amt = ::read(fd->get(), cirBuf + rpos, BUFFER_SIZE - rpos);
             } else {
-                amt = ::read(fd, cirBuf + rpos, wpos - rpos);
+                amt = ::read(fd->get(), cirBuf + rpos, wpos - rpos);
             }
             if (amt < 0) {
                 if (!(errno == EAGAIN || errno == EWOULDBLOCK)) {
-                    VLOG("Fail to read fd %d: %s", fd, strerror(errno));
+                    VLOG("Fail to read fd %d: %s", fd->get(), strerror(errno));
                     return -errno;
                 }  // otherwise just continue
             } else if (amt == 0) {
-                VLOG("Reached EOF of input file %d", fd);
+                VLOG("Reached EOF of input file %d", fd->get());
                 pfds[0].fd = -1;  // reach EOF so don't have to poll pfds[0].
             } else {
                 rpos += amt;
@@ -200,13 +200,13 @@ status_t FdBuffer::readProcessedDataInStream(int fd, int toFd, int fromFd, int64
         if (cirSize > 0 && pfds[1].fd != -1) {
             ssize_t amt;
             if (rpos > wpos) {
-                amt = ::write(toFd, cirBuf + wpos, rpos - wpos);
+                amt = ::write(toFd->get(), cirBuf + wpos, rpos - wpos);
             } else {
-                amt = ::write(toFd, cirBuf + wpos, BUFFER_SIZE - wpos);
+                amt = ::write(toFd->get(), cirBuf + wpos, BUFFER_SIZE - wpos);
             }
             if (amt < 0) {
                 if (!(errno == EAGAIN || errno == EWOULDBLOCK)) {
-                    VLOG("Fail to write toFd %d: %s", toFd, strerror(errno));
+                    VLOG("Fail to write toFd %d: %s", toFd->get(), strerror(errno));
                     return -errno;
                 }  // otherwise just continue
             } else {
@@ -217,8 +217,8 @@ status_t FdBuffer::readProcessedDataInStream(int fd, int toFd, int fromFd, int64
 
         // if buffer is empty and fd is closed, close write fd.
         if (cirSize == 0 && pfds[0].fd == -1 && pfds[1].fd != -1) {
-            VLOG("Close write pipe %d", toFd);
-            ::close(pfds[1].fd);
+            VLOG("Close write pipe %d", toFd->get());
+            toFd->reset();
             pfds[1].fd = -1;
         }
 
@@ -231,14 +231,14 @@ status_t FdBuffer::readProcessedDataInStream(int fd, int toFd, int fromFd, int64
         }
 
         // read from parsing process
-        ssize_t amt = ::read(fromFd, mBuffer.writeBuffer(), mBuffer.currentToWrite());
+        ssize_t amt = ::read(fromFd->get(), mBuffer.writeBuffer(), mBuffer.currentToWrite());
         if (amt < 0) {
             if (!(errno == EAGAIN || errno == EWOULDBLOCK)) {
-                VLOG("Fail to read fromFd %d: %s", fromFd, strerror(errno));
+                VLOG("Fail to read fromFd %d: %s", fromFd->get(), strerror(errno));
                 return -errno;
             }  // otherwise just continue
         } else if (amt == 0) {
-            VLOG("Reached EOF of fromFd %d", fromFd);
+            VLOG("Reached EOF of fromFd %d", fromFd->get());
             break;
         } else {
             mBuffer.wp()->move(amt);
diff --git a/cmds/incidentd/src/FdBuffer.h b/cmds/incidentd/src/FdBuffer.h
index 34ebcf50905d..db3a74b78178 100644
--- a/cmds/incidentd/src/FdBuffer.h
+++ b/cmds/incidentd/src/FdBuffer.h
@@ -18,10 +18,12 @@
 #ifndef FD_BUFFER_H
 #define FD_BUFFER_H
 
+#include <android-base/unique_fd.h>
 #include <android/util/EncodedBuffer.h>
 #include <utils/Errors.h>
 
 using namespace android;
+using namespace android::base;
 using namespace android::util;
 using namespace std;
 
@@ -38,13 +40,13 @@ public:
      * Returns NO_ERROR if there were no errors or if we timed out.
      * Will mark the file O_NONBLOCK.
      */
-    status_t read(int fd, int64_t timeoutMs);
+    status_t read(unique_fd* fd, int64_t timeoutMs);
 
     /**
      * Read the data until we hit eof.
      * Returns NO_ERROR if there were no errors.
      */
-    status_t readFully(int fd);
+    status_t readFully(unique_fd* fd);
 
     /**
      * Read processed results by streaming data to a parsing process, e.g. incident helper.
@@ -56,8 +58,8 @@ public:
      *
      * Poll will return POLLERR if fd is from sysfs, handle this edge case.
      */
-    status_t readProcessedDataInStream(int fd, int toFd, int fromFd, int64_t timeoutMs,
-                                       const bool isSysfs = false);
+    status_t readProcessedDataInStream(unique_fd* fd, unique_fd* toFd, unique_fd* fromFd,
+                                       int64_t timeoutMs, const bool isSysfs = false);
 
     /**
      * Whether we timed out.
diff --git a/cmds/incidentd/src/IncidentService.cpp b/cmds/incidentd/src/IncidentService.cpp
index d02b4dd99067..aeccefdd15c0 100644
--- a/cmds/incidentd/src/IncidentService.cpp
+++ b/cmds/incidentd/src/IncidentService.cpp
@@ -352,7 +352,8 @@ status_t IncidentService::cmd_privacy(FILE* in, FILE* out, FILE* err, Vector<Str
             printPrivacy(p, out, String8(""));
         } else if (opt == "parse") {
             FdBuffer buf;
-            status_t error = buf.read(fileno(in), 60000);
+            unique_fd infd(fileno(in));
+            status_t error = buf.read(&infd, 60000);
             if (error != NO_ERROR) {
                 fprintf(err, "Error reading from stdin\n");
                 return error;
diff --git a/cmds/incidentd/src/Section.cpp b/cmds/incidentd/src/Section.cpp
index 5cde5a94e2dd..ab4e764d92a4 100644
--- a/cmds/incidentd/src/Section.cpp
+++ b/cmds/incidentd/src/Section.cpp
@@ -277,8 +277,8 @@ FileSection::~FileSection() {}
 status_t FileSection::Execute(ReportRequestSet* requests) const {
     // read from mFilename first, make sure the file is available
     // add O_CLOEXEC to make sure it is closed when exec incident helper
-    int fd = open(mFilename, O_RDONLY | O_CLOEXEC);
-    if (fd == -1) {
+    unique_fd fd(open(mFilename, O_RDONLY | O_CLOEXEC));
+    if (fd.get() == -1) {
         ALOGW("FileSection '%s' failed to open file", this->name.string());
         return -errno;
     }
@@ -299,9 +299,8 @@ status_t FileSection::Execute(ReportRequestSet* requests) const {
     }
 
     // parent process
-    status_t readStatus = buffer.readProcessedDataInStream(fd, p2cPipe.writeFd(), c2pPipe.readFd(),
-                                                           this->timeoutMs, mIsSysfs);
-    close(fd);  // close the fd anyway.
+    status_t readStatus = buffer.readProcessedDataInStream(
+            &fd, &p2cPipe.writeFd(), &c2pPipe.readFd(), this->timeoutMs, mIsSysfs);
 
     if (readStatus != NO_ERROR || buffer.timedOut()) {
         ALOGW("FileSection '%s' failed to read data from incident helper: %s, timedout: %s",
@@ -342,17 +341,17 @@ GZipSection::~GZipSection() {}
 status_t GZipSection::Execute(ReportRequestSet* requests) const {
     // Reads the files in order, use the first available one.
     int index = 0;
-    int fd = -1;
+    unique_fd fd;
     while (mFilenames[index] != NULL) {
-        fd = open(mFilenames[index], O_RDONLY | O_CLOEXEC);
-        if (fd != -1) {
+        fd.reset(open(mFilenames[index], O_RDONLY | O_CLOEXEC));
+        if (fd.get() != -1) {
             break;
         }
         ALOGW("GZipSection failed to open file %s", mFilenames[index]);
         index++;  // look at the next file.
     }
-    VLOG("GZipSection is using file %s, fd=%d", mFilenames[index], fd);
-    if (fd == -1) return -1;
+    VLOG("GZipSection is using file %s, fd=%d", mFilenames[index], fd.get());
+    if (fd.get() == -1) return -1;
 
     FdBuffer buffer;
     Fpipe p2cPipe;
@@ -388,9 +387,9 @@ status_t GZipSection::Execute(ReportRequestSet* requests) const {
     VLOG("GZipSection '%s' editPos=%zd, dataBeginAt=%zd", this->name.string(), editPos,
          dataBeginAt);
 
-    status_t readStatus = buffer.readProcessedDataInStream(
-            fd, p2cPipe.writeFd(), c2pPipe.readFd(), this->timeoutMs, isSysfs(mFilenames[index]));
-    close(fd);  // close the fd anyway.
+    status_t readStatus =
+            buffer.readProcessedDataInStream(&fd, &p2cPipe.writeFd(), &c2pPipe.readFd(),
+                                             this->timeoutMs, isSysfs(mFilenames[index]));
 
     if (readStatus != NO_ERROR || buffer.timedOut()) {
         ALOGW("GZipSection '%s' failed to read data from gzip: %s, timedout: %s",
@@ -424,7 +423,7 @@ status_t GZipSection::Execute(ReportRequestSet* requests) const {
 // ================================================================================
 struct WorkerThreadData : public virtual RefBase {
     const WorkerThreadSection* section;
-    int fds[2];
+    Fpipe pipe;
 
     // Lock protects these fields
     mutex lock;
@@ -433,16 +432,10 @@ struct WorkerThreadData : public virtual RefBase {
 
     WorkerThreadData(const WorkerThreadSection* section);
     virtual ~WorkerThreadData();
-
-    int readFd() { return fds[0]; }
-    int writeFd() { return fds[1]; }
 };
 
 WorkerThreadData::WorkerThreadData(const WorkerThreadSection* sec)
-    : section(sec), workerDone(false), workerError(NO_ERROR) {
-    fds[0] = -1;
-    fds[1] = -1;
-}
+    : section(sec), workerDone(false), workerError(NO_ERROR) {}
 
 WorkerThreadData::~WorkerThreadData() {}
 
@@ -454,7 +447,7 @@ WorkerThreadSection::~WorkerThreadSection() {}
 
 static void* worker_thread_func(void* cookie) {
     WorkerThreadData* data = (WorkerThreadData*)cookie;
-    status_t err = data->section->BlockingCall(data->writeFd());
+    status_t err = data->section->BlockingCall(data->pipe.writeFd().get());
 
     {
         unique_lock<mutex> lock(data->lock);
@@ -462,7 +455,7 @@ static void* worker_thread_func(void* cookie) {
         data->workerError = err;
     }
 
-    close(data->writeFd());
+    data->pipe.writeFd().reset();
     data->decStrong(data->section);
     // data might be gone now. don't use it after this point in this thread.
     return NULL;
@@ -479,8 +472,7 @@ status_t WorkerThreadSection::Execute(ReportRequestSet* requests) const {
     sp<WorkerThreadData> data = new WorkerThreadData(this);
 
     // Create the pipe
-    err = pipe(data->fds);
-    if (err != 0) {
+    if (!data->pipe.init()) {
         return -errno;
     }
 
@@ -507,7 +499,7 @@ status_t WorkerThreadSection::Execute(ReportRequestSet* requests) const {
     pthread_attr_destroy(&attr);
 
     // Loop reading until either the timeout or the worker side is done (i.e. eof).
-    err = buffer.read(data->readFd(), this->timeoutMs);
+    err = buffer.read(&data->pipe.readFd(), this->timeoutMs);
     if (err != NO_ERROR) {
         // TODO: Log this error into the incident report.
         ALOGW("WorkerThreadSection '%s' reader failed with error '%s'", this->name.string(),
@@ -516,7 +508,7 @@ status_t WorkerThreadSection::Execute(ReportRequestSet* requests) const {
 
     // Done with the read fd. The worker thread closes the write one so
     // we never race and get here first.
-    close(data->readFd());
+    data->pipe.readFd().reset();
 
     // If the worker side is finished, then return its error (which may overwrite
     // our possible error -- but it's more interesting anyway).  If not, then we timed out.
@@ -602,7 +594,8 @@ status_t CommandSection::Execute(ReportRequestSet* requests) const {
     // child process to execute the command as root
     if (cmdPid == 0) {
         // replace command's stdout with ihPipe's write Fd
-        if (dup2(cmdPipe.writeFd(), STDOUT_FILENO) != 1 || !ihPipe.close() || !cmdPipe.close()) {
+        if (dup2(cmdPipe.writeFd().get(), STDOUT_FILENO) != 1 || !ihPipe.close() ||
+            !cmdPipe.close()) {
             ALOGW("CommandSection '%s' failed to set up stdout: %s", this->name.string(),
                   strerror(errno));
             _exit(EXIT_FAILURE);
@@ -619,8 +612,8 @@ status_t CommandSection::Execute(ReportRequestSet* requests) const {
         return -errno;
     }
 
-    close(cmdPipe.writeFd());
-    status_t readStatus = buffer.read(ihPipe.readFd(), this->timeoutMs);
+    cmdPipe.writeFd().reset();
+    status_t readStatus = buffer.read(&ihPipe.readFd(), this->timeoutMs);
     if (readStatus != NO_ERROR || buffer.timedOut()) {
         ALOGW("CommandSection '%s' failed to read data from incident helper: %s, timedout: %s",
               this->name.string(), strerror(-readStatus), buffer.timedOut() ? "true" : "false");
@@ -921,10 +914,10 @@ status_t TombstoneSection::BlockingCall(int pipeWriteFd) const {
             break;
         } else if (child == 0) {
             // This is the child process.
-            close(dumpPipe.readFd());
+            dumpPipe.readFd().reset();
             const int ret = dump_backtrace_to_file_timeout(
                     pid, is_java_process ? kDebuggerdJavaBacktrace : kDebuggerdNativeBacktrace,
-                    is_java_process ? 5 : 20, dumpPipe.writeFd());
+                    is_java_process ? 5 : 20, dumpPipe.writeFd().get());
             if (ret == -1) {
                 if (errno == 0) {
                     ALOGW("Dumping failed for pid '%d', likely due to a timeout\n", pid);
@@ -932,25 +925,17 @@ status_t TombstoneSection::BlockingCall(int pipeWriteFd) const {
                     ALOGE("Dumping failed for pid '%d': %s\n", pid, strerror(errno));
                 }
             }
-            if (close(dumpPipe.writeFd()) != 0) {
-                ALOGW("TombstoneSection '%s' failed to close dump pipe writeFd: %d",
-                      this->name.string(), errno);
-                _exit(EXIT_FAILURE);
-            }
-
+            dumpPipe.writeFd().reset();
             _exit(EXIT_SUCCESS);
         }
-        close(dumpPipe.writeFd());
+        dumpPipe.writeFd().reset();
         // Parent process.
         // Read from the pipe concurrently to avoid blocking the child.
         FdBuffer buffer;
-        err = buffer.readFully(dumpPipe.readFd());
+        err = buffer.readFully(&dumpPipe.readFd());
         if (err != NO_ERROR) {
             ALOGW("TombstoneSection '%s' failed to read stack dump: %d", this->name.string(), err);
-            if (close(dumpPipe.readFd()) != 0) {
-                ALOGW("TombstoneSection '%s' failed to close dump pipe readFd: %s",
-                      this->name.string(), strerror(errno));
-            }
+            dumpPipe.readFd().reset();
             break;
         }
 
@@ -967,13 +952,7 @@ status_t TombstoneSection::BlockingCall(int pipeWriteFd) const {
         proto.write(android::os::BackTraceProto::Stack::DUMP_DURATION_NS,
                     static_cast<long long>(Nanotime() - start));
         proto.end(token);
-
-        if (close(dumpPipe.readFd()) != 0) {
-            ALOGW("TombstoneSection '%s' failed to close dump pipe readFd: %d", this->name.string(),
-                  errno);
-            err = -errno;
-            break;
-        }
+        dumpPipe.readFd().reset();
     }
 
     proto.flush(pipeWriteFd);
diff --git a/cmds/incidentd/src/incidentd_util.cpp b/cmds/incidentd/src/incidentd_util.cpp
index c869c7a8d1d4..d7995133c722 100644
--- a/cmds/incidentd/src/incidentd_util.cpp
+++ b/cmds/incidentd/src/incidentd_util.cpp
@@ -53,16 +53,17 @@ bool Fpipe::close() {
 
 bool Fpipe::init() { return Pipe(&mRead, &mWrite); }
 
-int Fpipe::readFd() const { return mRead.get(); }
+unique_fd& Fpipe::readFd() { return mRead; }
 
-int Fpipe::writeFd() const { return mWrite.get(); }
+unique_fd& Fpipe::writeFd() { return mWrite; }
 
 pid_t fork_execute_cmd(const char* cmd, char* const argv[], Fpipe* input, Fpipe* output) {
     // fork used in multithreaded environment, avoid adding unnecessary code in child process
     pid_t pid = fork();
     if (pid == 0) {
-        if (TEMP_FAILURE_RETRY(dup2(input->readFd(), STDIN_FILENO)) < 0 || !input->close() ||
-            TEMP_FAILURE_RETRY(dup2(output->writeFd(), STDOUT_FILENO)) < 0 || !output->close()) {
+        if (TEMP_FAILURE_RETRY(dup2(input->readFd().get(), STDIN_FILENO)) < 0 || !input->close() ||
+            TEMP_FAILURE_RETRY(dup2(output->writeFd().get(), STDOUT_FILENO)) < 0 ||
+            !output->close()) {
             ALOGW("Can't setup stdin and stdout for command %s", cmd);
             _exit(EXIT_FAILURE);
         }
@@ -76,8 +77,8 @@ pid_t fork_execute_cmd(const char* cmd, char* const argv[], Fpipe* input, Fpipe*
         _exit(EXIT_FAILURE);  // always exits with failure if any
     }
     // close the fds used in child process.
-    close(input->readFd());
-    close(output->writeFd());
+    input->readFd().reset();
+    output->writeFd().reset();
     return pid;
 }
 
diff --git a/cmds/incidentd/src/incidentd_util.h b/cmds/incidentd/src/incidentd_util.h
index 3f7df91e7e50..228d7762fc81 100644
--- a/cmds/incidentd/src/incidentd_util.h
+++ b/cmds/incidentd/src/incidentd_util.h
@@ -41,8 +41,8 @@ public:
 
     bool init();
     bool close();
-    int readFd() const;
-    int writeFd() const;
+    unique_fd& readFd();
+    unique_fd& writeFd();
 
 private:
     unique_fd mRead;
diff --git a/cmds/incidentd/tests/FdBuffer_test.cpp b/cmds/incidentd/tests/FdBuffer_test.cpp
index 0e5eec6c7023..bf770173793f 100644
--- a/cmds/incidentd/tests/FdBuffer_test.cpp
+++ b/cmds/incidentd/tests/FdBuffer_test.cpp
@@ -37,6 +37,7 @@ class FdBufferTest : public Test {
 public:
     virtual void SetUp() override {
         ASSERT_NE(tf.fd, -1);
+        tffd.reset(tf.fd);
         ASSERT_NE(p2cPipe.init(), -1);
         ASSERT_NE(c2pPipe.init(), -1);
     }
@@ -56,13 +57,13 @@ public:
         EXPECT_EQ(expected[i], '\0');
     }
 
-    bool DoDataStream(int rFd, int wFd) {
+    bool DoDataStream(unique_fd* rFd, unique_fd* wFd) {
         char buf[BUFFER_SIZE];
         ssize_t nRead;
-        while ((nRead = read(rFd, buf, BUFFER_SIZE)) > 0) {
+        while ((nRead = read(rFd->get(), buf, BUFFER_SIZE)) > 0) {
             ssize_t nWritten = 0;
             while (nWritten < nRead) {
-                ssize_t amt = write(wFd, buf + nWritten, nRead - nWritten);
+                ssize_t amt = write(wFd->get(), buf + nWritten, nRead - nWritten);
                 if (amt < 0) {
                     return false;
                 }
@@ -75,6 +76,7 @@ public:
 protected:
     FdBuffer buffer;
     TemporaryFile tf;
+    unique_fd tffd;
     Fpipe p2cPipe;
     Fpipe c2pPipe;
 
@@ -85,7 +87,7 @@ protected:
 TEST_F(FdBufferTest, ReadAndWrite) {
     std::string testdata = "FdBuffer test string";
     ASSERT_TRUE(WriteStringToFile(testdata, tf.path));
-    ASSERT_EQ(NO_ERROR, buffer.read(tf.fd, READ_TIMEOUT));
+    ASSERT_EQ(NO_ERROR, buffer.read(&tffd, READ_TIMEOUT));
     AssertBufferReadSuccessful(testdata.size());
     AssertBufferContent(testdata.c_str());
 }
@@ -98,7 +100,7 @@ TEST_F(FdBufferTest, IterateEmpty) {
 TEST_F(FdBufferTest, ReadAndIterate) {
     std::string testdata = "FdBuffer test string";
     ASSERT_TRUE(WriteStringToFile(testdata, tf.path));
-    ASSERT_EQ(NO_ERROR, buffer.read(tf.fd, READ_TIMEOUT));
+    ASSERT_EQ(NO_ERROR, buffer.read(&tffd, READ_TIMEOUT));
 
     int i = 0;
     EncodedBuffer::iterator it = buffer.data();
@@ -117,16 +119,16 @@ TEST_F(FdBufferTest, ReadTimeout) {
     ASSERT_TRUE(pid != -1);
 
     if (pid == 0) {
-        close(c2pPipe.readFd());
+        c2pPipe.readFd().reset();
         while (true) {
             write(c2pPipe.writeFd(), "poo", 3);
             sleep(1);
         }
         _exit(EXIT_FAILURE);
     } else {
-        close(c2pPipe.writeFd());
+        c2pPipe.writeFd().reset();
 
-        status_t status = buffer.read(c2pPipe.readFd(), QUICK_TIMEOUT_MS);
+        status_t status = buffer.read(&c2pPipe.readFd(), QUICK_TIMEOUT_MS);
         ASSERT_EQ(NO_ERROR, status);
         EXPECT_TRUE(buffer.timedOut());
 
@@ -143,20 +145,20 @@ TEST_F(FdBufferTest, ReadInStreamAndWrite) {
     ASSERT_TRUE(pid != -1);
 
     if (pid == 0) {
-        close(p2cPipe.writeFd());
-        close(c2pPipe.readFd());
+        p2cPipe.writeFd().reset();
+        c2pPipe.readFd().reset();
         ASSERT_TRUE(WriteStringToFd(HEAD, c2pPipe.writeFd()));
-        ASSERT_TRUE(DoDataStream(p2cPipe.readFd(), c2pPipe.writeFd()));
-        close(p2cPipe.readFd());
-        close(c2pPipe.writeFd());
+        ASSERT_TRUE(DoDataStream(&p2cPipe.readFd(), &c2pPipe.writeFd()));
+        p2cPipe.readFd().reset();
+        c2pPipe.writeFd().reset();
         // Must exit here otherwise the child process will continue executing the test binary.
         _exit(EXIT_SUCCESS);
     } else {
-        close(p2cPipe.readFd());
-        close(c2pPipe.writeFd());
+        p2cPipe.readFd().reset();
+        c2pPipe.writeFd().reset();
 
-        ASSERT_EQ(NO_ERROR, buffer.readProcessedDataInStream(tf.fd, p2cPipe.writeFd(),
-                                                             c2pPipe.readFd(), READ_TIMEOUT));
+        ASSERT_EQ(NO_ERROR, buffer.readProcessedDataInStream(&tffd, &p2cPipe.writeFd(),
+                                                             &c2pPipe.readFd(), READ_TIMEOUT));
         AssertBufferReadSuccessful(HEAD.size() + testdata.size());
         AssertBufferContent(expected.c_str());
         wait(&pid);
@@ -172,23 +174,23 @@ TEST_F(FdBufferTest, ReadInStreamAndWriteAllAtOnce) {
     ASSERT_TRUE(pid != -1);
 
     if (pid == 0) {
-        close(p2cPipe.writeFd());
-        close(c2pPipe.readFd());
+        p2cPipe.writeFd().reset();
+        c2pPipe.readFd().reset();
         std::string data;
         // wait for read finishes then write.
         ASSERT_TRUE(ReadFdToString(p2cPipe.readFd(), &data));
         data = HEAD + data;
         ASSERT_TRUE(WriteStringToFd(data, c2pPipe.writeFd()));
-        close(p2cPipe.readFd());
-        close(c2pPipe.writeFd());
+        p2cPipe.readFd().reset();
+        c2pPipe.writeFd().reset();
         // Must exit here otherwise the child process will continue executing the test binary.
         _exit(EXIT_SUCCESS);
     } else {
-        close(p2cPipe.readFd());
-        close(c2pPipe.writeFd());
+        p2cPipe.readFd().reset();
+        c2pPipe.writeFd().reset();
 
-        ASSERT_EQ(NO_ERROR, buffer.readProcessedDataInStream(tf.fd, p2cPipe.writeFd(),
-                                                             c2pPipe.readFd(), READ_TIMEOUT));
+        ASSERT_EQ(NO_ERROR, buffer.readProcessedDataInStream(&tffd, &p2cPipe.writeFd(),
+                                                             &c2pPipe.readFd(), READ_TIMEOUT));
         AssertBufferReadSuccessful(HEAD.size() + testdata.size());
         AssertBufferContent(expected.c_str());
         wait(&pid);
@@ -202,18 +204,18 @@ TEST_F(FdBufferTest, ReadInStreamEmpty) {
     ASSERT_TRUE(pid != -1);
 
     if (pid == 0) {
-        close(p2cPipe.writeFd());
-        close(c2pPipe.readFd());
-        ASSERT_TRUE(DoDataStream(p2cPipe.readFd(), c2pPipe.writeFd()));
-        close(p2cPipe.readFd());
-        close(c2pPipe.writeFd());
+        p2cPipe.writeFd().reset();
+        c2pPipe.readFd().reset();
+        ASSERT_TRUE(DoDataStream(&p2cPipe.readFd(), &c2pPipe.writeFd()));
+        p2cPipe.readFd().reset();
+        c2pPipe.writeFd().reset();
         _exit(EXIT_SUCCESS);
     } else {
-        close(p2cPipe.readFd());
-        close(c2pPipe.writeFd());
+        p2cPipe.readFd().reset();
+        c2pPipe.writeFd().reset();
 
-        ASSERT_EQ(NO_ERROR, buffer.readProcessedDataInStream(tf.fd, p2cPipe.writeFd(),
-                                                             c2pPipe.readFd(), READ_TIMEOUT));
+        ASSERT_EQ(NO_ERROR, buffer.readProcessedDataInStream(&tffd, &p2cPipe.writeFd(),
+                                                             &c2pPipe.readFd(), READ_TIMEOUT));
         AssertBufferReadSuccessful(0);
         AssertBufferContent("");
         wait(&pid);
@@ -223,24 +225,24 @@ TEST_F(FdBufferTest, ReadInStreamEmpty) {
 TEST_F(FdBufferTest, ReadInStreamMoreThan4MB) {
     const std::string testFile = kTestDataPath + "morethan4MB.txt";
     size_t fourMB = (size_t)4 * 1024 * 1024;
-    int fd = open(testFile.c_str(), O_RDONLY | O_CLOEXEC);
-    ASSERT_NE(fd, -1);
+    unique_fd fd(open(testFile.c_str(), O_RDONLY | O_CLOEXEC));
+    ASSERT_NE(fd.get(), -1);
     int pid = fork();
     ASSERT_TRUE(pid != -1);
 
     if (pid == 0) {
-        close(p2cPipe.writeFd());
-        close(c2pPipe.readFd());
-        ASSERT_TRUE(DoDataStream(p2cPipe.readFd(), c2pPipe.writeFd()));
-        close(p2cPipe.readFd());
-        close(c2pPipe.writeFd());
+        p2cPipe.writeFd().reset();
+        c2pPipe.readFd().reset();
+        ASSERT_TRUE(DoDataStream(&p2cPipe.readFd(), &c2pPipe.writeFd()));
+        p2cPipe.readFd().reset();
+        c2pPipe.writeFd().reset();
         _exit(EXIT_SUCCESS);
     } else {
-        close(p2cPipe.readFd());
-        close(c2pPipe.writeFd());
+        p2cPipe.readFd().reset();
+        c2pPipe.writeFd().reset();
 
-        ASSERT_EQ(NO_ERROR, buffer.readProcessedDataInStream(fd, p2cPipe.writeFd(),
-                                                             c2pPipe.readFd(), READ_TIMEOUT));
+        ASSERT_EQ(NO_ERROR, buffer.readProcessedDataInStream(&fd, &p2cPipe.writeFd(),
+                                                             &c2pPipe.readFd(), READ_TIMEOUT));
         EXPECT_EQ(buffer.size(), fourMB);
         EXPECT_FALSE(buffer.timedOut());
         EXPECT_TRUE(buffer.truncated());
@@ -266,18 +268,18 @@ TEST_F(FdBufferTest, ReadInStreamTimeOut) {
     ASSERT_TRUE(pid != -1);
 
     if (pid == 0) {
-        close(p2cPipe.writeFd());
-        close(c2pPipe.readFd());
+        p2cPipe.writeFd().reset();
+        c2pPipe.readFd().reset();
         while (true) {
             sleep(1);
         }
         _exit(EXIT_FAILURE);
     } else {
-        close(p2cPipe.readFd());
-        close(c2pPipe.writeFd());
+        p2cPipe.readFd().reset();
+        c2pPipe.writeFd().reset();
 
-        ASSERT_EQ(NO_ERROR, buffer.readProcessedDataInStream(tf.fd, p2cPipe.writeFd(),
-                                                             c2pPipe.readFd(), QUICK_TIMEOUT_MS));
+        ASSERT_EQ(NO_ERROR, buffer.readProcessedDataInStream(&tffd, &p2cPipe.writeFd(),
+                                                             &c2pPipe.readFd(), QUICK_TIMEOUT_MS));
         EXPECT_TRUE(buffer.timedOut());
         kill(pid, SIGKILL);  // reap the child process
     }
diff --git a/cmds/incidentd/tests/PrivacyBuffer_test.cpp b/cmds/incidentd/tests/PrivacyBuffer_test.cpp
index c7c69a746f0a..5edc0c79785b 100644
--- a/cmds/incidentd/tests/PrivacyBuffer_test.cpp
+++ b/cmds/incidentd/tests/PrivacyBuffer_test.cpp
@@ -58,7 +58,8 @@ public:
 
     void writeToFdBuffer(string str) {
         ASSERT_TRUE(WriteStringToFile(str, tf.path));
-        ASSERT_EQ(NO_ERROR, buffer.read(tf.fd, 10000));
+        unique_fd tffd(tf.fd);
+        ASSERT_EQ(NO_ERROR, buffer.read(&tffd, 10000));
         ASSERT_EQ(str.size(), buffer.size());
     }