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

Commit 6ea1d7e4 authored by Nicolas Roulet's avatar Nicolas Roulet
Browse files

Check corruption in NBLog Reader snapshots

Bug: 35642225
Test: Keeps working with uncorrupted buffers, not tested with corruption
Change-Id: I327badc97c9518b41a7085aebbd74f9452463a8e
parent dcdfaecc
Loading
Loading
Loading
Loading
+110 −42
Original line number Diff line number Diff line
@@ -91,8 +91,9 @@ pid_t NBLog::FormatEntry::author() const {
    return -1;
}

size_t NBLog::FormatEntry::copyTo(std::unique_ptr<audio_utils_fifo_writer> &dst, int author) const {
    auto it = this->begin();
NBLog::FormatEntry::iterator NBLog::FormatEntry::copyWithAuthor(
        std::unique_ptr<audio_utils_fifo_writer> &dst, int author) const {
    auto it = begin();
    // copy fmt start entry
    it.copyTo(dst);
    // insert author entry
@@ -110,7 +111,7 @@ size_t NBLog::FormatEntry::copyTo(std::unique_ptr<audio_utils_fifo_writer> &dst,
    }
    it.copyTo(dst);
    ++it;
    return it - this->begin();
    return it;
}

void NBLog::FormatEntry::iterator::copyTo(std::unique_ptr<audio_utils_fifo_writer> &dst) const {
@@ -126,6 +127,9 @@ NBLog::FormatEntry::iterator NBLog::FormatEntry::begin() const {
    return iterator(mEntry);
}

NBLog::FormatEntry::iterator::iterator()
    : ptr(nullptr) {}

NBLog::FormatEntry::iterator::iterator(const uint8_t *entry)
    : ptr(entry) {}

@@ -150,6 +154,16 @@ NBLog::FormatEntry::iterator& NBLog::FormatEntry::iterator::operator--() {
    return *this;
}

NBLog::FormatEntry::iterator NBLog::FormatEntry::iterator::next() const {
    iterator aux(*this);
    return ++aux;
}

NBLog::FormatEntry::iterator NBLog::FormatEntry::iterator::prev() const {
    iterator aux(*this);
    return --aux;
}

int NBLog::FormatEntry::iterator::operator-(const NBLog::FormatEntry::iterator &other) const {
    return ptr - other.ptr;
}
@@ -580,6 +594,23 @@ NBLog::Reader::~Reader()
    delete mFifo;
}

uint8_t *NBLog::Reader::findLastEntryOfType(uint8_t *front, uint8_t *back, uint8_t type) {
    while (back + Entry::kPreviousLengthOffset >= front) {
        uint8_t *prev = back - back[Entry::kPreviousLengthOffset] - Entry::kOverhead;
        if (prev < front || prev + prev[offsetof(FormatEntry::entry, length)] +
                            Entry::kOverhead != back) {

            // prev points to an out of limits or inconsistent entry
            return nullptr;
        }
        if (prev[offsetof(FormatEntry::entry, type)] == type) {
            return prev;
        }
        back = prev;
    }
    return nullptr; // no entry found
}

std::unique_ptr<NBLog::Reader::Snapshot> NBLog::Reader::getSnapshot()
{
    if (mFifoReader == NULL) {
@@ -588,22 +619,66 @@ std::unique_ptr<NBLog::Reader::Snapshot> NBLog::Reader::getSnapshot()
    // make a copy to avoid race condition with writer
    size_t capacity = mFifo->capacity();

    std::unique_ptr<Snapshot> snapshot(new Snapshot(capacity));
    // This emulates the behaviour of audio_utils_fifo_reader::read, but without incrementing the
    // reader index. The index is incremented after handling corruption, to after the last complete
    // entry of the buffer
    size_t lost;
    audio_utils_iovec iovec[2];
    ssize_t availToRead = mFifoReader->obtain(iovec, capacity, NULL /*timeout*/, &lost);
    if (availToRead <= 0) {
        return std::unique_ptr<NBLog::Reader::Snapshot>(new Snapshot());
    }

    std::unique_ptr<Snapshot> snapshot(new Snapshot(availToRead));
    memcpy(snapshot->mData, (const char *) mFifo->buffer() + iovec[0].mOffset, iovec[0].mLength);
    if (iovec[1].mLength > 0) {
        memcpy(snapshot->mData + (iovec[0].mLength),
            (const char *) mFifo->buffer() + iovec[1].mOffset, iovec[1].mLength);
    }

    // Handle corrupted buffer
    // Potentially, a buffer has corrupted data on both beginning (due to overflow) and end
    // (due to incomplete format entry). But even if the end format entry is incomplete,
    // it ends in a complete entry (which is not an END_FMT). So is safe to traverse backwards.
    // TODO: handle client corruption (in the middle of a buffer)

    uint8_t *back = snapshot->mData + availToRead;
    uint8_t *front = snapshot->mData;

    // Find last END_FMT. <back> is sitting on an entry which might be the middle of a FormatEntry.
    // We go backwards until we find an EVENT_END_FMT.
    uint8_t *lastEnd = findLastEntryOfType(front, back, EVENT_END_FMT);
    if (lastEnd == nullptr) {
        snapshot->mEnd = snapshot->mBegin = FormatEntry::iterator(front);
    } else {
        // end of snapshot points to after last END_FMT entry
        snapshot->mEnd = FormatEntry::iterator(lastEnd + Entry::kOverhead);
        // find first START_FMT
        uint8_t *firstStart = nullptr;
        uint8_t *firstStartTmp = lastEnd;
        while ((firstStartTmp = findLastEntryOfType(front, firstStartTmp, EVENT_START_FMT))
                != nullptr) {
            firstStart = firstStartTmp;
        }
        // firstStart is null if no START_FMT entry was found before lastEnd
        if (firstStart == nullptr) {
            snapshot->mBegin = snapshot->mEnd;
        } else {
            snapshot->mBegin = FormatEntry::iterator(firstStart);
        }
    }

    // advance fifo reader index to after last entry read.
    mFifoReader->release(snapshot->mEnd - front);

    ssize_t actual = mFifoReader->read((void*) snapshot->mData, capacity, NULL /*timeout*/,
                                       &(snapshot->mLost));
    ALOG_ASSERT(actual <= capacity);
    snapshot->mAvail = actual > 0 ? (size_t) actual : 0;
    snapshot->mLost = lost;
    return snapshot;

}

void NBLog::Reader::dump(int fd, size_t indent, NBLog::Reader::Snapshot &snapshot)
{
    NBLog::FormatEntry::iterator entry(snapshot.data() + snapshot.available());
    NBLog::FormatEntry::iterator prevEntry = entry;
    --prevEntry;
    NBLog::FormatEntry::iterator start(snapshot.data());

#if 0
    struct timespec ts;
    time_t maxSec = -1;
    while (entry - start >= (int) Entry::kOverhead) {
@@ -623,16 +698,18 @@ void NBLog::Reader::dump(int fd, size_t indent, NBLog::Reader::Snapshot &snapsho
        --entry;
        --prevEntry;
    }
#endif
    mFd = fd;
    mIndent = indent;
    String8 timestamp, body;
    size_t lost = snapshot.lost() + (entry - start);
    size_t lost = snapshot.lost() + (snapshot.begin() - FormatEntry::iterator(snapshot.data()));
    if (lost > 0) {
        body.appendFormat("warning: lost %zu bytes worth of events", lost);
        // TODO timestamp empty here, only other choice to wait for the first timestamp event in the
        //      log to push it out.  Consider keeping the timestamp/body between calls to readAt().
        dumpLine(timestamp, body);
    }
#if 0
    size_t width = 1;
    while (maxSec >= 10) {
        ++width;
@@ -642,9 +719,8 @@ void NBLog::Reader::dump(int fd, size_t indent, NBLog::Reader::Snapshot &snapsho
        timestamp.appendFormat("[%*s]", (int) width + 4, "");
    }
    bool deferredTimestamp = false;
    NBLog::FormatEntry::iterator end(snapshot.data() + snapshot.available());

    while (entry != end) {
#endif
    for (auto entry = snapshot.begin(); entry != snapshot.end();) {
        switch (entry->type) {
#if 0
        case EVENT_STRING:
@@ -717,21 +793,23 @@ void NBLog::Reader::dump(int fd, size_t indent, NBLog::Reader::Snapshot &snapsho
            break;
        case EVENT_END_FMT:
            body.appendFormat("warning: got to end format event");
            ++entry;
            break;
        case EVENT_RESERVED:
        default:
            body.appendFormat("warning: unknown event %d", entry->type);
            body.appendFormat("warning: unexpected event %d", entry->type);
            ++entry;
            break;
        }

        if (!body.isEmpty()) {
            dumpLine(timestamp, body);
            deferredTimestamp = false;
            // deferredTimestamp = false;
        }
    }
    if (deferredTimestamp) {
        dumpLine(timestamp, body);
    }
    // if (deferredTimestamp) {
    //     dumpLine(timestamp, body);
    // }
}

void NBLog::Reader::dump(int fd, size_t indent)
@@ -817,12 +895,6 @@ NBLog::FormatEntry::iterator NBLog::Reader::handleFormat(const FormatEntry &fmtE
        size_t length = arg->length;

        // TODO check length for event type is correct
        if (!arg.hasConsistentLength()) {
            // TODO: corrupt, resync buffer
            body->append("<invalid entry>");
            ++fmt_offset;
            continue;
        }

        if (event == EVENT_END_FMT) {
            break;
@@ -910,37 +982,33 @@ bool operator>(const struct MergeItem &i1, const struct MergeItem &i2) {
void NBLog::Merger::merge() {
    int nLogs = mNamedReaders.size();
    std::vector<std::unique_ptr<NBLog::Reader::Snapshot>> snapshots(nLogs);
    std::vector<NBLog::FormatEntry::iterator> offsets(nLogs);
    for (int i = 0; i < nLogs; ++i) {
        snapshots[i] = mNamedReaders[i].reader()->getSnapshot();
        offsets[i] = snapshots[i]->begin();
    }
    // initialize offsets
    std::vector<size_t> offsets(nLogs, 0);
    // TODO custom heap implementation could allow to update top, improving performance
    // for bursty buffers
    std::priority_queue<MergeItem, std::vector<MergeItem>, std::greater<MergeItem>> timestamps;
    for (int i = 0; i < nLogs; ++i)
    {
        if (snapshots[i]->available() > 0) {
            timespec ts = FormatEntry(snapshots[i]->data()).timestamp();
            MergeItem item(ts, i);
            timestamps.push(item);
        if (offsets[i] != snapshots[i]->end()) {
            timespec ts = FormatEntry(offsets[i]).timestamp();
            timestamps.emplace(ts, i);
        }
    }

    while (!timestamps.empty()) {
        // find minimum timestamp
        int index = timestamps.top().index;
        // copy it to the log
        size_t length = FormatEntry(snapshots[index]->data() + offsets[index]).copyTo(
            mFifoWriter, index);
        // copy it to the log, increasing offset
        offsets[index] = FormatEntry(offsets[index]).copyWithAuthor(mFifoWriter, index);
        // update data structures
        offsets[index] += length;
        ALOGW_IF(offsets[index] > snapshots[index]->available(), "Overflown snapshot capacity");
        timestamps.pop();
        if (offsets[index] != snapshots[index]->available()) {
            timespec ts = FormatEntry(snapshots[index]->data() + offsets[index]).timestamp();
            MergeItem item(ts, index);
            timestamps.emplace(item);
        if (offsets[index] != snapshots[index]->end()) {
            timespec ts = FormatEntry(offsets[index]).timestamp();
            timestamps.emplace(ts, index);
        }
    }
}
+22 −9
Original line number Diff line number Diff line
@@ -89,6 +89,7 @@ public:
    // entry iterator
    class iterator {
    public:
        iterator();
        iterator(const uint8_t *entry);
        iterator(const iterator &other);

@@ -99,6 +100,8 @@ public:
        iterator&       operator++(); // ++i
        // back to previous entry
        iterator&       operator--(); // --i
        iterator        next() const;
        iterator        prev() const;
        bool            operator!=(const iterator &other) const;
        int             operator-(const iterator &other) const;

@@ -134,7 +137,7 @@ public:
    int         author() const;

    // copy entry, adding author before timestamp, returns size of original entry
    size_t      copyTo(std::unique_ptr<audio_utils_fifo_writer> &dst, int author) const;
    iterator    copyWithAuthor(std::unique_ptr<audio_utils_fifo_writer> &dst, int author) const;

    iterator    begin() const;

@@ -317,26 +320,32 @@ public:
    // A snapshot of a readers buffer
    class Snapshot {
    public:
        Snapshot() : mData(NULL), mAvail(0), mLost(0) {}
        Snapshot() : mData(NULL), mLost(0) {}

        Snapshot(size_t bufferSize) : mData(new uint8_t[bufferSize]) {}

        ~Snapshot() { delete[] mData; }

        // copy of the buffer
        const uint8_t *data() const { return mData; }

        // amount of data available (given by audio_utils_fifo_reader)
        size_t   available() const { return mAvail; }
        uint8_t *data() const { return mData; }

        // amount of data lost (given by audio_utils_fifo_reader)
        size_t   lost() const { return mLost; }

        // iterator to beginning of readable segment of snapshot
        // data between begin and end has valid entries
        FormatEntry::iterator begin() { return mBegin; }

        // iterator to end of readable segment of snapshot
        FormatEntry::iterator end() { return mEnd; }


    private:
        friend class Reader;
        const uint8_t *mData;
        size_t         mAvail;
        uint8_t              *mData;
        size_t                mLost;
        FormatEntry::iterator mBegin;
        FormatEntry::iterator mEnd;
    };

    // Input parameter 'size' is the desired size of the timeline in byte units.
@@ -373,6 +382,10 @@ private:
    // dummy method for handling absent author entry
    virtual size_t handleAuthor(const FormatEntry &fmtEntry, String8 *body) { return 0; }

    // Searches for the last entry of type <type> in the range [front, back)
    // back has to be entry-aligned. Returns nullptr if none enconuntered.
    static uint8_t *findLastEntryOfType(uint8_t *front, uint8_t *back, uint8_t type);

    static const size_t kSquashTimestamp = 5; // squash this many or more adjacent timestamps
};