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

Commit cbafce94 authored by Yao Chen's avatar Yao Chen
Browse files

Add metadata and headers to incident reports. 

+ Remove the spawned thread inside the ReportFile for filter_and_write_report
  because it leads to accessing freed memory

  Instead, let the caller of ReportFile::startFileteringData create the thread.
  ReportFile class shouldn't care about whether it's writing to a pipe for IPC
  or regular file.

+ Add uri building in incidentd

+ Add metadata and headers to incident reports

Test: existing passed tests in incidentd_test still pass.
      Manually tested with statsd

Change-Id: I5fef900d31f5d181275814f1e1c8c98443f201a7
parent b51fda1b

cmds/incidentd/src/Broadcaster.cpp

+13 −5
Original line number Diff line number Diff line
@@ -22,6 +22,7 @@ 


#include <android/os/DropBoxManager.h>

#include <binder/IServiceManager.h>

#include <thread>



namespace android {

namespace os {
@@ -391,13 +392,20 @@ status_t Broadcaster::send_to_dropbox(const sp<ReportFile>& file, 
        return NO_ERROR;

    }



    // Start a thread to write the data to dropbox.

    int readFd = -1;

    err = file->startFilteringData(&readFd, args);

    if (err != NO_ERROR) {

        return err;

    int fds[2];

    if (pipe(fds) != 0) {

        ALOGW("Error opening pipe to filter incident report: %s", file->getDataFileName().c_str());

        return NO_ERROR;

    }



    int readFd = fds[0];

    int writeFd = fds[1];



    // spawn a thread to write the data. Release the writeFd ownership to the thread.

    thread th([file, writeFd, args]() { file->startFilteringData(writeFd, args); });



    th.detach();



    // Takes ownership of readFd.

    Status status = dropbox->addFile(String16("incident"), readFd, 0);

    if (!status.isOk()) {

cmds/incidentd/src/IncidentService.cpp

+14 −8
Original line number Diff line number Diff line
@@ -32,6 +32,7 @@ 
#include <log/log.h>

#include <private/android_filesystem_config.h>

#include <utils/Looper.h>

#include <thread>



#include <unistd.h>
@@ -117,7 +118,8 @@ static Status checkIncidentPermissions(const IncidentReportArgs& args) { 
}



static string build_uri(const string& pkg, const string& cls, const string& id) {

    return "build_uri not implemented " + pkg + "/" + cls + "/" + id;

    return "content://android.os.IncidentManager/pending?pkg="

        + pkg + "&receiver=" + cls + "&r=" + id;

}



// ================================================================================
@@ -358,17 +360,21 @@ Status IncidentService::getIncidentReport(const String16& pkg16, const String16& 
    IncidentReportArgs args;

    sp<ReportFile> file = mWorkDirectory->getReport(pkg, cls, id, &args);

    if (file != nullptr) {

        int fd;

        err = file->startFilteringData(&fd, args);

        if (err != 0) {

            ALOGW("Error reading data file that we think should exist: %s",

        // Create pipe

        int fds[2];

        if (pipe(fds) != 0) {

            ALOGW("Error opening pipe to filter incident report: %s",

                  file->getDataFileName().c_str());

            return Status::ok();

        }



        result->setTimestampNs(file->getTimestampNs());

        result->setPrivacyPolicy(file->getEnvelope().privacy_policy());

        result->takeFileDescriptor(fd);

        result->takeFileDescriptor(fds[0]);

        int writeFd = fds[1];

        // spawn a thread to write the data. Release the writeFd ownership to the thread.

        thread th([file, writeFd, args]() { file->startFilteringData(writeFd, args); });



        th.detach();

    }



    return Status::ok();

cmds/incidentd/src/Reporter.cpp

+1 −1
Original line number Diff line number Diff line
@@ -551,7 +551,7 @@ void Reporter::runReport(size_t* reportByteSize) { 
             buf++) {

            // If there was an error now, there will be an error later and we will remove

            // it from the list then.

            write_header_section(request->getFd(), *buf);

            write_header_section(request->getFd(), buf->data(), buf->size());

        }

    });

cmds/incidentd/src/WorkDirectory.cpp

+30 −38
Original line number Diff line number Diff line
@@ -18,6 +18,7 @@ 


#include "WorkDirectory.h"



#include "proto_util.h"

#include "PrivacyFilter.h"



#include <google/protobuf/io/zero_copy_stream_impl.h>
@@ -64,6 +65,9 @@ static const string EXTENSION_DATA(".data"); 
 */

const ComponentName DROPBOX_SENTINEL("android", "DROPBOX");



/** metadata field id in IncidentProto */

const int FIELD_ID_INCIDENT_METADATA = 2;



/**

 * Read a protobuf from disk into the message.

 */
@@ -386,65 +390,53 @@ void ReportFile::closeDataFile() { 
    }

}



status_t ReportFile::startFilteringData(int* fd, const IncidentReportArgs& args) {

status_t ReportFile::startFilteringData(int writeFd, const IncidentReportArgs& args) {

    // Open data file.

    int dataFd = open(mDataFileName.c_str(), O_RDONLY | O_CLOEXEC);

    if (dataFd < 0) {

        ALOGW("Error opening incident report '%s' %s", getDataFileName().c_str(), strerror(-errno));

        close(writeFd);

        return -errno;

    }



    // Check that the size on disk is what we thought we wrote.

    struct stat st;

    if (fstat(dataFd, &st) != 0) {

        ALOGW("Error running fstat incident report '%s' %s", getDataFileName().c_str(),

              strerror(-errno));

        close(writeFd);

        return -errno;

    }

    if (st.st_size != mEnvelope.data_file_size()) {

        ALOGW("File size mismatch. Envelope says %" PRIi64 " bytes but data file is %" PRIi64

                " bytes: %s", (int64_t)mEnvelope.data_file_size(), st.st_size,

                mDataFileName.c_str());

              " bytes: %s",

              (int64_t)mEnvelope.data_file_size(), st.st_size, mDataFileName.c_str());

        ALOGW("Removing incident report");

        mWorkDirectory->remove(this);

        close(writeFd);

        return BAD_VALUE;

    }



    // Create pipe

    int fds[2];

    if (pipe(fds) != 0) {

        ALOGW("Error opening pipe to filter incident report: %s", getDataFileName().c_str());

        return -errno;

    }

    status_t err;



    *fd = fds[0];

    int writeFd = fds[1];

    for (const auto& report : mEnvelope.report()) {

        for (const auto& header : report.header()) {

           write_header_section(writeFd,

               reinterpret_cast<const uint8_t*>(header.c_str()), header.size());

        }

    }



    // Spawn off a thread to do the filtering and writing

    thread th([this, dataFd, writeFd, args]() {

        ALOGD("worker thread started dataFd=%d writeFd=%d", dataFd, writeFd);

        status_t err;

    if (mEnvelope.has_metadata()) {

        write_section(writeFd, FIELD_ID_INCIDENT_METADATA, mEnvelope.metadata());

    }



    err = filter_and_write_report(writeFd, dataFd, mEnvelope.privacy_policy(), args);

        close(writeFd);



    if (err != NO_ERROR) {

        ALOGW("Error writing incident report '%s' to dropbox: %s", getDataFileName().c_str(),

                strerror(-err));

            // If there's an error here, there will also be an error returned from

            // addFile, so we'll use that error to reschedule the send_to_dropbox.

            // If the file is corrupted, we will put some logs in logcat, but won't

            // actually return an error.

            return;

    }

    });



    // Better would be to join this thread after write is back, but there is no

    // timeout parameter for that, which means we can't clean up if system server

    // is stuck. Better is to leak the thread, which will eventually clean itself

    // up after system server eventually dies, which it probably will.

    th.detach();



    // If the thread fails to start, we should return an error, but the thread

    // class doesn't give us a good way to determine that.  Just pretend everything

    // is ok.

    close(writeFd);

    return NO_ERROR;

}

cmds/incidentd/src/WorkDirectory.h

+6 −6
Original line number Diff line number Diff line
@@ -135,14 +135,14 @@ public: 
    void closeDataFile();



    /**

     * Spawn a thread to start writing and filtering data to a pipe, the read end of which

     * will be returned in fd.  This thread will be detached and run until somebody finishes

     * reading from the fd or closes it.  If there is an error, returns it and you will not

     * get an fd.

     * Use the privacy and section configuration from the args parameter to filter data, write

     * to [writeFd] and take the ownership of [writeFd].

     *

     * Use the privacy and section configuraiton from the args parameter.

     * Note: this call is blocking. When the writeFd is a pipe fd for IPC, caller should make sure

     * it's called on a separate thread so that reader can start to read without waiting for writer

     * to finish writing (which may not happen due to pipe buffer overflow).

     */

    status_t startFilteringData(int* fd, const IncidentReportArgs& args);

    status_t startFilteringData(int writeFd, const IncidentReportArgs& args);



    /**

     * Get the name of the data file on disk.