tp: remove over-cautious detection of use-after-free in iterators
Back in the very old days of trace processor, we introduced a mechanism
to detect using an iterator after the trace processor instance was
freed. Unfortunately, turns out there are two problems:
1. The implementation is buggy. This is because iterators don't inform
the trace processor when they are moved so addresses the trace
processor holds onto are invalid.
2. Trace processor instances are very long lived in practice so the
likelihood that someone holds iterators past the lifetime is
unlikely.
Most other C++ classes don't try and protect against use-after-frees and
instead just crash so follow their behaviour given the unlikeliness of
ever hitting this scenario.
Change-Id: Ie37e0c746b7541d2fbec9adacd4ad9bc8194f406
diff --git a/src/trace_processor/iterator_impl.h b/src/trace_processor/iterator_impl.h
index 3613f29..f286f2c 100644
--- a/src/trace_processor/iterator_impl.h
+++ b/src/trace_processor/iterator_impl.h
@@ -108,13 +108,22 @@
util::Status Status() { return status_; }
- // Methods called by TraceProcessorImpl.
- void Reset();
-
private:
+ // Dummy function to pass to ScopedResource.
+ static int DummyClose(TraceProcessorImpl*) { return 0; }
+
+ // Iterators hold onto an instance of TraceProcessor to track when the query
+ // ends in the sql stats table. As iterators are movable, we need to null out
+ // the TraceProcessor in the moved out iterator to avoid double recording
+ // query ends. We could manually define a move constructor instead, but given
+ // the error prone nature of keeping functions up to date, this seems like a
+ // nicer approach.
+ using ScopedTraceProcessor =
+ base::ScopedResource<TraceProcessorImpl*, &DummyClose, nullptr>;
+
void RecordFirstNextInSqlStats();
- TraceProcessorImpl* trace_processor_;
+ ScopedTraceProcessor trace_processor_;
sqlite3* db_ = nullptr;
ScopedStmt stmt_;
uint32_t column_count_ = 0;
