masaori335 commented on code in PR #13328:
URL: https://github.com/apache/trafficserver/pull/13328#discussion_r3687437758
##########
src/iocore/cache/Stripe.cc:
##########
@@ -170,6 +178,52 @@ Stripe::_init_directory(std::size_t directory_size, int
header_size, int footer_
this->directory.footer = reinterpret_cast<StripeHeaderFooter
*>(this->directory.raw_dir + footer_offset);
}
+// Bounds-check the trusted header/freelist fields of an in-shm directory
before
+// the fast-restart attach (magic/version are already checked by the caller). A
+// stale-but-magic-valid segment could present out-of-range offsets that
become OOB
+// disk I/O. On failure the caller falls through to the disk read +
recover_data().
+//
+// Trust model: the shm segment is trusted to the same degree as the on-disk
+// directory (same-uid, mode 0600). Stripe geometry (segments/buckets) is
recomputed
+// locally each run and raw_dir_size is exact-matched before attach, so this
only
+// validates the header cursor fields and per-segment freelist heads; it does
not
+// re-validate individual Dir entries -- the read path already checks Doc
magic + key
+// before serving, so a stale entry resolves to a miss, never served
corruption.
+bool
+Stripe::_shm_directory_is_valid() const
+{
+ // sector_size must be sane geometry (mirrors the hw_sector_size range in
Cache.cc).
+ if (this->directory.header->sector_size == 0 ||
this->directory.header->sector_size > STORE_BLOCK_SIZE) {
+ return false;
+ }
+
+ // phase is a single bit of write-cursor state; only 0/1 are valid.
+ if (this->directory.header->phase > 1) {
+ return false;
+ }
+
+ // write_pos/last_write_pos/agg_pos must point into the data region.
+ const off_t data_lo = this->start;
+ const off_t data_hi = this->skip + this->len;
+
+ if (this->directory.header->write_pos < data_lo ||
this->directory.header->write_pos > data_hi ||
+ this->directory.header->last_write_pos < data_lo ||
this->directory.header->last_write_pos > data_hi ||
+ this->directory.header->agg_pos < data_lo ||
this->directory.header->agg_pos > data_hi) {
+ return false;
+ }
Review Comment:
Took the agg_pos half, left last_write_pos out.
agg_pos == write_pos is a real post-clean-shutdown invariant:
flush_aggregate_write_buffer() and aggWriteDone() both leave them equal, an
in-flight AIO invalidates the segment, and it is exactly what the debug-only
asserts at Stripe.cc:464 and StripeSM.cc:746 check. Added.
last_write_pos > write_pos is a legitimate state though — agg_wrap() sets
write_pos = start and agg_pos = write_pos but never touches last_write_pos, so
right after a wrap last_write_pos is the pre-wrap tail.
handle_recover_from_data() handles precisely that case:
recover_pos = directory.header->last_write_pos;
if (recover_pos >= skip + len) {
recover_wrapped = true;
recover_pos = start;
}
So that clause would send every restart in the post-wrap window back to the
disk read. On a low-traffic stripe that window can be long, and it fails
silently (just a slower start), which is the worst kind of regression to
notice. Left it out and noted why in the code and the design doc.
Addressed by 80cff77c80.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
