From aacfa95f36a374d01226dbd7edf935955b4f6908 Mon Sep 17 00:00:00 2001 From: Patrick Donnelly Date: Thu, 14 Feb 2019 10:49:32 -0800 Subject: [PATCH] mds: pass LogEvent as unique_ptr To avoid leaks. Fixes: http://tracker.ceph.com/issues/38324 Signed-off-by: Patrick Donnelly --- src/mds/LogEvent.cc | 96 +++++++++++++++++++----------- src/mds/LogEvent.h | 6 +- src/mds/MDLog.cc | 18 +++--- src/tools/cephfs/EventOutput.cc | 44 ++++++-------- src/tools/cephfs/JournalScanner.cc | 22 ++----- src/tools/cephfs/JournalScanner.h | 14 ++--- src/tools/cephfs/JournalTool.cc | 2 +- 7 files changed, 104 insertions(+), 98 deletions(-) diff --git a/src/mds/LogEvent.cc b/src/mds/LogEvent.cc index 9a6e26beee673..bafe4b4fbd5ea 100644 --- a/src/mds/LogEvent.cc +++ b/src/mds/LogEvent.cc @@ -41,12 +41,11 @@ #define dout_context g_ceph_context -LogEvent *LogEvent::decode(bufferlist& bl) +std::unique_ptr LogEvent::decode_event(bufferlist::const_iterator p) { // parse type, length - auto p = bl.cbegin(); EventType type; - LogEvent *event = NULL; + std::unique_ptr event; using ceph::decode; decode(type, p); @@ -54,7 +53,7 @@ LogEvent *LogEvent::decode(bufferlist& bl) try { DECODE_START(1, p); decode(type, p); - event = decode_event(bl, p, type); + event = decode_event(p, type); DECODE_FINISH(p); } catch (const buffer::error &e) { @@ -62,7 +61,7 @@ LogEvent *LogEvent::decode(bufferlist& bl) return NULL; } } else { // we are using classic encoding - event = decode_event(bl, p, type); + event = decode_event(p, type); } return event; } @@ -126,43 +125,73 @@ LogEvent::EventType LogEvent::str_to_type(std::string_view str) } -LogEvent *LogEvent::decode_event(bufferlist& bl, bufferlist::const_iterator& p, LogEvent::EventType type) +std::unique_ptr LogEvent::decode_event(bufferlist::const_iterator& p, LogEvent::EventType type) { - int length = bl.length() - p.get_off(); + const auto length = p.get_remaining(); generic_dout(15) << "decode_log_event type " << type << ", size " << length << dendl; // create event - LogEvent *le; + std::unique_ptr le; switch (type) { - case EVENT_SUBTREEMAP: le = new ESubtreeMap; break; + case EVENT_SUBTREEMAP: + le = std::make_unique(); + break; case EVENT_SUBTREEMAP_TEST: - le = new ESubtreeMap; + le = std::make_unique(); le->set_type(type); break; - case EVENT_EXPORT: le = new EExport; break; - case EVENT_IMPORTSTART: le = new EImportStart; break; - case EVENT_IMPORTFINISH: le = new EImportFinish; break; - case EVENT_FRAGMENT: le = new EFragment; break; - - case EVENT_RESETJOURNAL: le = new EResetJournal; break; - - case EVENT_SESSION: le = new ESession; break; - case EVENT_SESSIONS_OLD: le = new ESessions; (static_cast(le))->mark_old_encoding(); break; - case EVENT_SESSIONS: le = new ESessions; break; - - case EVENT_UPDATE: le = new EUpdate; break; - case EVENT_SLAVEUPDATE: le = new ESlaveUpdate; break; - case EVENT_OPEN: le = new EOpen; break; - case EVENT_COMMITTED: le = new ECommitted; break; - - case EVENT_TABLECLIENT: le = new ETableClient; break; - case EVENT_TABLESERVER: le = new ETableServer; break; - - case EVENT_NOOP: le = new ENoOp; break; - + case EVENT_EXPORT: + le = std::make_unique(); + break; + case EVENT_IMPORTSTART: + le = std::make_unique(); + break; + case EVENT_IMPORTFINISH: + le = std::make_unique(); + break; + case EVENT_FRAGMENT: + le = std::make_unique(); + break; + case EVENT_RESETJOURNAL: + le = std::make_unique(); + break; + case EVENT_SESSION: + le = std::make_unique(); + break; + case EVENT_SESSIONS_OLD: + { + auto e = std::make_unique(); + e->mark_old_encoding(); + le = std::move(e); + } + break; + case EVENT_SESSIONS: + le = std::make_unique(); + break; + case EVENT_UPDATE: + le = std::make_unique(); + break; + case EVENT_SLAVEUPDATE: + le = std::make_unique(); + break; + case EVENT_OPEN: + le = std::make_unique(); + break; + case EVENT_COMMITTED: + le = std::make_unique(); + break; + case EVENT_TABLECLIENT: + le = std::make_unique(); + break; + case EVENT_TABLESERVER: + le = std::make_unique(); + break; + case EVENT_NOOP: + le = std::make_unique(); + break; default: generic_dout(0) << "uh oh, unknown log event type " << type << " length " << length << dendl; - return NULL; + return nullptr; } // decode @@ -171,8 +200,7 @@ LogEvent *LogEvent::decode_event(bufferlist& bl, bufferlist::const_iterator& p, } catch (const buffer::error &e) { generic_dout(0) << "failed to decode LogEvent type " << type << dendl; - delete le; - return NULL; + return nullptr; } ceph_assert(p.end()); diff --git a/src/mds/LogEvent.h b/src/mds/LogEvent.h index 11aa4a82eb763..f9133b9aac47a 100644 --- a/src/mds/LogEvent.h +++ b/src/mds/LogEvent.h @@ -58,7 +58,7 @@ public: private: EventType _type; uint64_t _start_off; - static LogEvent *decode_event(bufferlist& bl, bufferlist::const_iterator& p, EventType type); + static std::unique_ptr decode_event(bufferlist::const_iterator&, EventType); protected: utime_t stamp; @@ -87,8 +87,8 @@ public: // encoding virtual void encode(bufferlist& bl, uint64_t features) const = 0; - virtual void decode(bufferlist::const_iterator &bl) = 0; - static LogEvent *decode(bufferlist &bl); + virtual void decode(bufferlist::const_iterator &) = 0; + static std::unique_ptr decode_event(bufferlist::const_iterator); virtual void dump(Formatter *f) const = 0; void encode_with_header(bufferlist& bl, uint64_t features) { diff --git a/src/mds/MDLog.cc b/src/mds/MDLog.cc index 9c08232f540e3..75e5934b31f50 100644 --- a/src/mds/MDLog.cc +++ b/src/mds/MDLog.cc @@ -1177,13 +1177,13 @@ void MDLog::_reformat_journal(JournalPointer const &jp_in, Journaler *old_journa ceph_assert(r); // Update segment_pos_rewrite - LogEvent *le = LogEvent::decode(bl); + auto le = LogEvent::decode_event(bl.cbegin()); if (le) { bool modified = false; if (le->get_type() == EVENT_SUBTREEMAP || le->get_type() == EVENT_RESETJOURNAL) { - ESubtreeMap *sle = dynamic_cast(le); + auto sle = dynamic_cast(le.get()); if (sle == NULL || sle->event_seq == 0) { // A non-explicit event seq: the effective sequence number // of this segment is it's position in the old journal and @@ -1207,11 +1207,10 @@ void MDLog::_reformat_journal(JournalPointer const &jp_in, Journaler *old_journa // (expire_pos is just an optimization so it's safe to eliminate it) if (le->get_type() == EVENT_SUBTREEMAP || le->get_type() == EVENT_SUBTREEMAP_TEST) { - ESubtreeMap *sle = dynamic_cast(le); - ceph_assert(sle != NULL); + auto& sle = dynamic_cast(*le); dout(20) << __func__ << " zeroing expire_pos in subtreemap event at " - << le_pos << " seq=" << sle->event_seq << dendl; - sle->expire_pos = 0; + << le_pos << " seq=" << sle.event_seq << dendl; + sle.expire_pos = 0; modified = true; } @@ -1219,8 +1218,6 @@ void MDLog::_reformat_journal(JournalPointer const &jp_in, Journaler *old_journa bl.clear(); le->encode_with_header(bl, mds->mdsmap->get_up_features()); } - - delete le; } else { // Failure from LogEvent::decode, our job is to change the journal wrapper, // not validate the contents, so pass it through. @@ -1385,7 +1382,7 @@ void MDLog::_replay_thread() ceph_assert(r); // unpack event - LogEvent *le = LogEvent::decode(bl); + auto le = LogEvent::decode_event(bl.cbegin()); if (!le) { dout(0) << "_replay " << pos << "~" << bl.length() << " / " << journaler->get_write_pos() << " -- unable to decode event" << dendl; @@ -1410,7 +1407,7 @@ void MDLog::_replay_thread() // new segment? if (le->get_type() == EVENT_SUBTREEMAP || le->get_type() == EVENT_RESETJOURNAL) { - ESubtreeMap *sle = dynamic_cast(le); + auto sle = dynamic_cast(le.get()); if (sle && sle->event_seq > 0) event_seq = sle->event_seq; else @@ -1442,7 +1439,6 @@ void MDLog::_replay_thread() le->replay(mds); } } - delete le; logger->set(l_mdl_rdpos, pos); } diff --git a/src/tools/cephfs/EventOutput.cc b/src/tools/cephfs/EventOutput.cc index ed8403f32ab14..8cb235a82f987 100644 --- a/src/tools/cephfs/EventOutput.cc +++ b/src/tools/cephfs/EventOutput.cc @@ -39,12 +39,10 @@ int EventOutput::binary() const for (JournalScanner::EventMap::const_iterator i = scan.events.begin(); i != scan.events.end(); ++i) { bufferlist bin; std::stringstream filename; - if (i->second.log_event) { - LogEvent *le = i->second.log_event; + if (auto& le = i->second.log_event; le) { le->encode(bin, CEPH_FEATURES_SUPPORTED_DEFAULT); filename << "0x" << std::hex << i->first << std::dec << "_" << le->get_type_str() << ".bin"; - } else if (i->second.pi) { - PurgeItem* pi = i->second.pi; + } else if (auto& pi = i->second.pi; pi) { pi->encode(bin); filename << "0x" << std::hex << i->first << std::dec << "_" << pi->get_type_str() << ".bin"; } @@ -69,15 +67,11 @@ int EventOutput::json() const jf.open_array_section("journal"); { for (JournalScanner::EventMap::const_iterator i = scan.events.begin(); i != scan.events.end(); ++i) { - if (i->second.log_event) { - LogEvent *le = i->second.log_event; + if (auto& le = i->second.log_event; le) { jf.open_object_section("log_event"); - { - le->dump(&jf); - } + le->dump(&jf); jf.close_section(); // log_event - } else if (i->second.pi) { - PurgeItem* pi = i->second.pi; + } else if (auto& pi = i->second.pi; pi) { jf.open_object_section("purge_action"); pi->dump(&jf); jf.close_section(); @@ -99,30 +93,30 @@ int EventOutput::json() const void EventOutput::list() const { for (JournalScanner::EventMap::const_iterator i = scan.events.begin(); i != scan.events.end(); ++i) { - if (i->second.log_event) { + if (auto& le = i->second.log_event; le) { std::vector ev_paths; - EMetaBlob const *emb = i->second.log_event->get_metablob(); + EMetaBlob const *emb = le->get_metablob(); if (emb) { emb->get_paths(ev_paths); } std::string detail; - if (i->second.log_event->get_type() == EVENT_UPDATE) { - EUpdate *eu = reinterpret_cast(i->second.log_event); - detail = eu->type; + if (le->get_type() == EVENT_UPDATE) { + auto& eu = reinterpret_cast(*le); + detail = eu.type; } - std::cout <second.log_event->get_stamp() << " 0x" + std::cout << le->get_stamp() << " 0x" << std::hex << i->first << std::dec << " " - << i->second.log_event->get_type_str() << ": " + << le->get_type_str() << ": " << " (" << detail << ")" << std::endl; for (std::vector::iterator i = ev_paths.begin(); i != ev_paths.end(); ++i) { std::cout << " " << *i << std::endl; } - } else if (i->second.pi) { - std::cout << i->second.pi->stamp << " 0x" + } else if (auto& pi = i->second.pi; pi) { + std::cout << pi->stamp << " 0x" << std::hex << i->first << std::dec << " " - << i->second.pi->get_type_str() << std::endl; + << pi->get_type_str() << std::endl; } } } @@ -132,10 +126,10 @@ void EventOutput::summary() const std::map type_count; for (JournalScanner::EventMap::const_iterator i = scan.events.begin(); i != scan.events.end(); ++i) { std::string type; - if (i->second.log_event) - type = i->second.log_event->get_type_str(); - else if (i->second.pi) - type = i->second.pi->get_type_str(); + if (auto& le = i->second.log_event; le) + type = le->get_type_str(); + else if (auto& pi = i->second.pi; pi) + type = pi->get_type_str(); if (type_count.count(type) == 0) { type_count[type] = 0; } diff --git a/src/tools/cephfs/JournalScanner.cc b/src/tools/cephfs/JournalScanner.cc index 36c018a4bf5bb..f81c33d0489db 100644 --- a/src/tools/cephfs/JournalScanner.cc +++ b/src/tools/cephfs/JournalScanner.cc @@ -276,15 +276,15 @@ int JournalScanner::scan_events() } bool valid_entry = true; if (is_mdlog) { - LogEvent *le = LogEvent::decode(le_bl); + auto le = LogEvent::decode_event(le_bl.cbegin()); if (le) { dout(10) << "Valid entry at 0x" << std::hex << read_offset << std::dec << dendl; if (le->get_type() == EVENT_SUBTREEMAP || le->get_type() == EVENT_SUBTREEMAP_TEST) { - ESubtreeMap *sle = dynamic_cast(le); - if (sle->expire_pos > read_offset) { + auto&& sle = dynamic_cast(*le); + if (sle.expire_pos > read_offset) { errors.insert(std::make_pair( read_offset, EventError( -ERANGE, @@ -293,22 +293,18 @@ int JournalScanner::scan_events() } if (filter.apply(read_offset, *le)) { - events[read_offset] = EventRecord(le, consumed); - } else { - delete le; + events.insert_or_assign(read_offset, EventRecord(std::move(le), consumed)); } } else { valid_entry = false; } } else if (type == "purge_queue"){ - PurgeItem* pi = new PurgeItem(); + auto pi = std::make_unique(); try { auto q = le_bl.cbegin(); pi->decode(q); if (filter.apply(read_offset, *pi)) { - events[read_offset] = EventRecord(pi, consumed); - } else { - delete pi; + events.insert_or_assign(read_offset, EventRecord(std::move(pi), consumed)); } } catch (const buffer::error &err) { valid_entry = false; @@ -351,12 +347,6 @@ JournalScanner::~JournalScanner() header = NULL; } dout(4) << events.size() << " events" << dendl; - for (EventMap::iterator i = events.begin(); i != events.end(); ++i) { - if (i->second.log_event) - delete i->second.log_event; - else if (i->second.pi) - delete i->second.pi; - } events.clear(); } diff --git a/src/tools/cephfs/JournalScanner.h b/src/tools/cephfs/JournalScanner.h index 89ee2fd2803f4..018a3b30fd298 100644 --- a/src/tools/cephfs/JournalScanner.h +++ b/src/tools/cephfs/JournalScanner.h @@ -87,14 +87,12 @@ class JournalScanner // The results of the scan inodeno_t ino; // Corresponds to journal ino according their type - class EventRecord { - public: - EventRecord() : log_event(NULL), raw_size(0) {} - EventRecord(LogEvent *le, uint32_t rs) : log_event(le), pi(NULL), raw_size(rs) {} - EventRecord(PurgeItem* p, uint32_t rs) : log_event(NULL), pi(p), raw_size(rs) {} - LogEvent *log_event; - PurgeItem *pi; - uint32_t raw_size; //< Size from start offset including all encoding overhead + struct EventRecord { + EventRecord(std::unique_ptr le, uint32_t rs) : log_event(std::move(le)), raw_size(rs) {} + EventRecord(std::unique_ptr p, uint32_t rs) : pi(std::move(p)), raw_size(rs) {} + std::unique_ptr log_event; + std::unique_ptr pi; + uint32_t raw_size = 0; //< Size from start offset including all encoding overhead }; class EventError { diff --git a/src/tools/cephfs/JournalTool.cc b/src/tools/cephfs/JournalTool.cc index 37f3b5e165a5c..29dca56df00c2 100644 --- a/src/tools/cephfs/JournalTool.cc +++ b/src/tools/cephfs/JournalTool.cc @@ -452,7 +452,7 @@ int JournalTool::main_event(std::vector &argv) std::set consumed_inos; for (JournalScanner::EventMap::iterator i = js.events.begin(); i != js.events.end(); ++i) { - LogEvent *le = i->second.log_event; + auto& le = i->second.log_event; EMetaBlob const *mb = le->get_metablob(); if (mb) { int scav_r = recover_dentries(*mb, dry_run, &consumed_inos); -- 2.39.5