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

Commit 6355d2f3 authored by Yi Jin's avatar Yi Jin
Browse files

Wrap fd with unique_fd so it won't leak. 

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

cmds/incidentd/src/FdBuffer.cpp

+32 −32
Original line number Diff line number Diff line
@@ -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);

cmds/incidentd/src/FdBuffer.h

+6 −4
Original line number Diff line number Diff line
@@ -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.

cmds/incidentd/src/IncidentService.cpp

+2 −1
Original line number Diff line number Diff line
@@ -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;

cmds/incidentd/src/Section.cpp

+30 −51
Original line number Diff line number Diff line
@@ -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);

cmds/incidentd/src/incidentd_util.cpp

+7 −6
Original line number Diff line number Diff line
@@ -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;

}