pw_ring_buffer: Crash on corrupt buffer access
Crash when accessing a corrupt ring buffer since there is no general way
to recover and corrupt data is indicative of other major memory issues.
Remove user's expectancy of DataLoss errors and documentation in
comments and update comments where necessary.
Remove unused iterator class and status.
Change-Id: I75e64ceb17dd7013461f4034d1cfa13090017e64
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/59040
Reviewed-by: Ewout van Bekkum <ewout@google.com>
Pigweed-Auto-Submit: Carlos Chinchilla <cachinchilla@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
diff --git a/pw_log_rpc/rpc_log_drain.cc b/pw_log_rpc/rpc_log_drain.cc
index 385b9e0..6c21286 100644
--- a/pw_log_rpc/rpc_log_drain.cc
+++ b/pw_log_rpc/rpc_log_drain.cc
@@ -87,8 +87,7 @@
uint32_t drop_count = 0;
Result<multisink::MultiSink::Drain::PeekedEntry> possible_entry =
PeekEntry(log_entry_buffer_, drop_count);
- if (possible_entry.status().IsResourceExhausted() ||
- possible_entry.status().IsDataLoss()) {
+ if (possible_entry.status().IsResourceExhausted()) {
continue;
}
diff --git a/pw_multisink/public/pw_multisink/multisink.h b/pw_multisink/public/pw_multisink/multisink.h
index 258d85b..7bfd3f0 100644
--- a/pw_multisink/public/pw_multisink/multisink.h
+++ b/pw_multisink/public/pw_multisink/multisink.h
@@ -118,6 +118,8 @@
// } while (!result.IsOutOfRange());
// }
// }
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
//
// Return values:
// OK - An entry was successfully read from the multisink.
@@ -125,8 +127,6 @@
// FAILED_PRECONDITION - The drain must be attached to a sink.
// RESOURCE_EXHAUSTED - The provided buffer was not large enough to store
// the next available entry, which was discarded.
- // DATA_LOSS - An entry that did not match the expected format was read and
- // discarded.
Result<ConstByteSpan> PopEntry(ByteSpan buffer, uint32_t& drop_count_out)
PW_LOCKS_EXCLUDED(multisink_->lock_);
@@ -147,6 +147,9 @@
// }
// PW_CHECK_OK(PopEntry(peek_result.value());
//
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
+ //
// Return values:
// OK - the entry or entries were removed from the multisink succesfully.
// FAILED_PRECONDITION - The drain must be attached to a sink.
@@ -155,19 +158,20 @@
// Returns a copy of the next available entry if it exists and acquires the
// latest drop count, without moving the drain forward, except if there is a
- // DATA_LOSS or RESOURCE_EXHAUSTED error when peeking, in which case the
- // drain is automatically advanced.
+ // RESOURCE_EXHAUSTED error when peeking, in which case the drain is
+ // automatically advanced.
// The `drop_count_out` follows the same logic as `PopEntry`. The user must
// call `PopEntry` once the data in peek was used successfully.
//
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
+ //
// Return values:
// OK - An entry was successfully read from the multisink.
// OUT_OF_RANGE - No entries were available.
// FAILED_PRECONDITION - The drain must be attached to a sink.
// RESOURCE_EXHAUSTED - The provided buffer was not large enough to store
// the next available entry, which was discarded.
- // DATA_LOSS - An entry that did not match the expected format was read and
- // discarded.
Result<PeekedEntry> PeekEntry(ByteSpan buffer, uint32_t& drop_count_out)
PW_LOCKS_EXCLUDED(multisink_->lock_);
@@ -211,8 +215,6 @@
virtual void OnNewEntryAvailable() = 0;
};
- class Iterator;
-
class iterator {
public:
iterator& operator++() {
@@ -239,6 +241,13 @@
return it_ != rhs.it_;
}
+ // Returns the status of the last iteration operation. If the iterator
+ // fails to read an entry, it will move to iterator::end() and indicate
+ // the failure reason here.
+ //
+ // Return values:
+ // OK - iteration is successful and iterator points to the next entry.
+ // DATA_LOSS - Failed to read the metadata at this location.
Status status() const { return it_.status(); }
private:
@@ -250,7 +259,6 @@
ring_buffer::PrefixedEntryRingBufferMulti::iterator it_;
ConstByteSpan entry_;
- Status iteration_status_;
};
class UnsafeIterationWrapper {
@@ -346,6 +354,9 @@
// to `Request::kPop`. Drains use this API to strip away sequence ID
// information for drop calculation.
//
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
+ //
// Returns:
// OK - An entry was successfully read from the multisink. The
// `drop_count_out` is set to the difference between the current sequence ID
@@ -354,8 +365,6 @@
// a multisink.
// RESOURCE_EXHAUSTED - The provided buffer was not large enough to store
// the next available entry, which was discarded.
- // DATA_LOSS - An entry that did not match the expected format was read and
- // discarded.
Result<ConstByteSpan> PeekOrPopEntry(Drain& drain,
ByteSpan buffer,
Request request,
diff --git a/pw_ring_buffer/docs.rst b/pw_ring_buffer/docs.rst
index a98a44c..986a82c 100644
--- a/pw_ring_buffer/docs.rst
+++ b/pw_ring_buffer/docs.rst
@@ -68,6 +68,15 @@
PW_LOG_WARN("Iterator failed to read some entries!");
}
+Data corruption
+===============
+``PrefixedEntryRingBufferMulti`` offers a circular ring buffer for arbitrary
+length data entries. Some metadata bytes are added at the beginning of each
+entry to delimit the size of the entry. Unlike the iterator, the methods in
+``PrefixedEntryRingBufferMulti`` require that data in the buffer is not corrupt.
+When these methods encounter data corruption, there is no generic way to
+recover, and thus, the application crashes. Data corruption is indicative of
+other issues.
Dependencies
============
diff --git a/pw_ring_buffer/prefixed_entry_ring_buffer.cc b/pw_ring_buffer/prefixed_entry_ring_buffer.cc
index 0dcff63..1e28214 100644
--- a/pw_ring_buffer/prefixed_entry_ring_buffer.cc
+++ b/pw_ring_buffer/prefixed_entry_ring_buffer.cc
@@ -327,7 +327,7 @@
PrefixedEntryRingBufferMulti::FrontEntryInfo(const Reader& reader) const {
Result<PrefixedEntryRingBufferMulti::EntryInfo> entry_info =
RawFrontEntryInfo(reader.read_idx_);
- PW_DCHECK_OK(entry_info.status());
+ PW_CHECK_OK(entry_info.status());
return entry_info.value();
}
diff --git a/pw_ring_buffer/public/pw_ring_buffer/prefixed_entry_ring_buffer.h b/pw_ring_buffer/public/pw_ring_buffer/prefixed_entry_ring_buffer.h
index c958729..a4e3490 100644
--- a/pw_ring_buffer/public/pw_ring_buffer/prefixed_entry_ring_buffer.h
+++ b/pw_ring_buffer/public/pw_ring_buffer/prefixed_entry_ring_buffer.h
@@ -72,6 +72,9 @@
// the provided destination std::span. The number of bytes read is written
// to bytes_read
//
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
+ //
// Return values:
// OK - Data successfully read from the ring buffer.
// FAILED_PRECONDITION - Buffer not initialized.
@@ -89,6 +92,9 @@
}
// Peek the front entry's preamble only to avoid copying data unnecessarily.
+ //
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
Status PeekFrontPreamble(uint32_t& user_preamble_out) const {
return buffer_->InternalPeekFrontPreamble(*this, user_preamble_out);
}
@@ -114,6 +120,9 @@
// Pop and discard the oldest stored data chunk of data from the ring
// buffer.
//
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
+ //
// Return values:
// OK - Data successfully read from the ring buffer.
// FAILED_PRECONDITION - Buffer not initialized.
@@ -122,12 +131,18 @@
// Get the size in bytes of the next chunk, not including preamble, to be
// read.
+ //
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
size_t FrontEntryDataSizeBytes() const {
return buffer_->InternalFrontEntryDataSizeBytes(*this);
}
// Get the size in bytes of the next chunk, including preamble and data
// chunk, to be read.
+ //
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
size_t FrontEntryTotalSizeBytes() const {
return buffer_->InternalFrontEntryTotalSizeBytes(*this);
}
@@ -311,6 +326,9 @@
// entry. It is only used if user_preamble was set at class construction
// time. It is varint-encoded before insertion into the buffer.
//
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
+ //
// Return values:
// OK - Data successfully written to the ring buffer.
// INVALID_ARGUMENT - Size of data to write is zero bytes
@@ -349,6 +367,9 @@
// the provided destination std::span. The number of bytes read is written to
// `bytes_read_out`.
//
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
+ //
// Return values:
// OK - Data successfully read from the ring buffer.
// FAILED_PRECONDITION - Buffer not initialized.
@@ -373,6 +394,9 @@
// Pop and discard the oldest stored data chunk of data from the ring buffer.
//
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
+ //
// Return values:
// OK - Data successfully read from the ring buffer.
// FAILED_PRECONDITION - Buffer not initialized.
@@ -421,7 +445,8 @@
// multiple readers if multiple are slow.
//
// Precondition: This function requires that at least one reader is attached
- // and has at least one entry to pop.
+ // and has at least one entry to pop. There will be a crash if data is
+ // corrupted.
void InternalPopFrontAll();
// Returns a the slowest reader in the list.
@@ -434,14 +459,17 @@
// Get info struct with the size of the preamble and data chunk for the next
// entry to be read. Calls RawFrontEntryInfo and asserts on failure.
+ //
+ // Precondition: the buffer data must not be corrupt, otherwise there will
+ // be a crash.
EntryInfo FrontEntryInfo(const Reader& reader) const;
// Get info struct with the size of the preamble and data chunk for the next
// entry to be read.
//
// Returns:
- // Ok - EntryInfo containing the next entry metadata.
- // DataLoss - Failed to read the metadata at this location.
+ // OK - EntryInfo containing the next entry metadata.
+ // DATA_LOSS - Failed to read the metadata at this location.
Result<EntryInfo> RawFrontEntryInfo(size_t source_idx) const;
// Get the raw number of available bytes free in the ring buffer. This is
