Skip to content

Commit

Permalink
HHVM Debugger: Fix segfault from conditional breakpoint
Browse files Browse the repository at this point in the history 
Summary: BreakpointManager was implicitly invoking the copy constructor on Breakpoint and allocating temporaries that when destructed were calling clearCachedConditionUnit and delete'ing the HPHP::Unit cached for the breakpoint condition.  This resulted in a double-free of the unit.

Reviewed By: velocityboy

Differential Revision: D10351891

fbshipit-source-id: 447d4047c37ffc98e9a659711d45217683985668
  • Loading branch information
@ebluestein @hhvm-bot
ebluestein authored and hhvm-bot committed Nov 4, 2018
1 parent 2bcbe26 commit f371503
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 28 deletions.
71 changes: 44 additions & 27 deletions hphp/runtime/ext/vsdebug/breakpoint.cpp
View file
Expand Up @@ -70,8 +70,11 @@ Breakpoint::Breakpoint(
} }


Breakpoint::~Breakpoint() { Breakpoint::~Breakpoint() {
for (const auto& pair : m_unitCache) { for (auto it = m_unitCache.begin(); it != m_unitCache.end();) {
clearCachedConditionUnit(pair.first); if (it->second != nullptr) {
delete it->second;
}
it = m_unitCache.erase(it);
} }
} }


Expand Down Expand Up @@ -105,6 +108,12 @@ BreakpointManager::BreakpointManager(Debugger* debugger) :
} }


BreakpointManager::~BreakpointManager() { BreakpointManager::~BreakpointManager() {
for (auto it = m_breakpoints.begin(); it != m_breakpoints.end();) {
delete it->second;
it = m_breakpoints.erase(it);
}

assertx(m_breakpoints.empty());
} }


bool BreakpointManager::bpMatchesPath( bool BreakpointManager::bpMatchesPath(
Expand Down Expand Up @@ -153,7 +162,7 @@ const std::unordered_set<int> BreakpointManager::getBreakpointIdsForPath(
std::unordered_set<int> ids; std::unordered_set<int> ids;
const auto path = boost::filesystem::path(unitPath); const auto path = boost::filesystem::path(unitPath);
for (auto it = m_breakpoints.begin(); it != m_breakpoints.end(); it++) { for (auto it = m_breakpoints.begin(); it != m_breakpoints.end(); it++) {
if (bpMatchesPath(&it->second, path)) { if (bpMatchesPath(it->second, path)) {
ids.insert(it->first); ids.insert(it->first);
} }
} }
Expand Down Expand Up @@ -185,17 +194,24 @@ int BreakpointManager::addSourceLineBreakpoint(
const std::string& hitCondition const std::string& hitCondition
) { ) {
const int id = ++g_nextBreakpointId; const int id = ++g_nextBreakpointId;
Breakpoint bp = Breakpoint(id, line, column, path, condition, hitCondition); const auto bp = new Breakpoint(
id,
line,
column,
path,
condition,
hitCondition
);


// Add the new breakpoint to the breakpoint map. // Add the new breakpoint to the breakpoint map.
m_breakpoints.emplace(id, bp); m_breakpoints.emplace(id, bp);


// If this is the first time we've seen a bp for this file, create a new // If this is the first time we've seen a bp for this file, create a new
// set of breakpoint IDs for this file. // set of breakpoint IDs for this file.
auto it = m_sourceBreakpoints.find(bp.m_path); auto it = m_sourceBreakpoints.find(bp->m_path);
if (it == m_sourceBreakpoints.end()) { if (it == m_sourceBreakpoints.end()) {
m_sourceBreakpoints.emplace(bp.m_path, std::unordered_set<int>()); m_sourceBreakpoints.emplace(bp->m_path, std::unordered_set<int>());
it = m_sourceBreakpoints.find(bp.m_path); it = m_sourceBreakpoints.find(bp->m_path);
} }


// Add this ID to the set of breakpoints in this file. // Add this ID to the set of breakpoints in this file.
Expand All @@ -207,8 +223,8 @@ int BreakpointManager::addSourceLineBreakpoint(
VSDebugLogger::LogLevelInfo, VSDebugLogger::LogLevelInfo,
"Stored new breakpoint request (id = %d), (file = %s), (line = %d)", "Stored new breakpoint request (id = %d), (file = %s), (line = %d)",
id, id,
bp.m_path.c_str(), bp->m_path.c_str(),
bp.m_line bp->m_line
); );


// Tell the client the bp was installed. // Tell the client the bp was installed.
Expand Down Expand Up @@ -372,10 +388,10 @@ void BreakpointManager::onFuncIntercepted(


// Warn about any breakpoints in this request that resolved to // Warn about any breakpoints in this request that resolved to
// functions that match the intercepted function name. // functions that match the intercepted function name.
for (auto pair : m_breakpoints) { for (auto it = m_breakpoints.begin(); it != m_breakpoints.end(); it++) {
auto bpId = pair.first; auto bpId = it->first;
const auto& bp = pair.second; const auto bp = it->second;
if (bp.m_functionFullName == name) { if (bp->m_functionFullName == name) {
sendBpInterceptedWarning(requestId, bpId, name); sendBpInterceptedWarning(requestId, bpId, name);
} }
} }
Expand All @@ -387,9 +403,8 @@ int BreakpointManager::addFunctionBreakpoint(
const std::string& hitCondition const std::string& hitCondition
) { ) {
const int id = ++g_nextBreakpointId; const int id = ++g_nextBreakpointId;
Breakpoint bp = Breakpoint(id, function, condition, hitCondition); Breakpoint* bp = new Breakpoint(id, function, condition, hitCondition);

// Add the new breakpoint to the breakpoint map.
// Add the new breakpoint to the function breakpoint map.
m_breakpoints.emplace(id, bp); m_breakpoints.emplace(id, bp);
m_fnBreakpoints.emplace(function, id); m_fnBreakpoints.emplace(function, id);


Expand All @@ -409,8 +424,8 @@ int BreakpointManager::addFunctionBreakpoint(
const std::unordered_set<int> const std::unordered_set<int>
BreakpointManager::getFunctionBreakpoints() const { BreakpointManager::getFunctionBreakpoints() const {
std::unordered_set<int> ids; std::unordered_set<int> ids;
for (const auto& pair : m_fnBreakpoints) { for (auto it = m_fnBreakpoints.begin(); it != m_fnBreakpoints.end(); it++) {
ids.insert(pair.second); ids.insert(it->second);
} }
return ids; return ids;
} }
Expand All @@ -427,8 +442,10 @@ void BreakpointManager::removeBreakpoint(int id) {
} }


// Remove the breakpoint. // Remove the breakpoint.
const std::string filePath = it->second.m_path; const std::string filePath = it->second->m_path;
const std::string function = it->second.m_function; const std::string function = it->second->m_function;

delete it->second;
m_breakpoints.erase(it); m_breakpoints.erase(it);


// If it was resolved, remove it from the resolved set. // If it was resolved, remove it from the resolved set.
Expand Down Expand Up @@ -461,8 +478,8 @@ void BreakpointManager::removeBreakpoint(int id) {
VSDebugLogger::LogLevelInfo, VSDebugLogger::LogLevelInfo,
"Client removed breakpoint: (id = %d), (file = %s), (line = %d)", "Client removed breakpoint: (id = %d), (file = %s), (line = %d)",
id, id,
it->second.m_path.c_str(), it->second->m_path.c_str(),
it->second.m_line it->second->m_line
); );


// Tell the client the breakpoint is gone. // Tell the client the breakpoint is gone.
Expand All @@ -481,7 +498,7 @@ void BreakpointManager::onBreakpointResolved(
Breakpoint* bp = nullptr; Breakpoint* bp = nullptr;
const auto it = m_breakpoints.find(id); const auto it = m_breakpoints.find(id);
if (it != m_breakpoints.end()) { if (it != m_breakpoints.end()) {
bp = &it->second; bp = it->second;
} }


if (bp == nullptr) { if (bp == nullptr) {
Expand Down Expand Up @@ -689,7 +706,7 @@ void BreakpointManager::sendBreakpointEvent(
Breakpoint* BreakpointManager::getBreakpointById(int id) { Breakpoint* BreakpointManager::getBreakpointById(int id) {
auto it = m_breakpoints.find(id); auto it = m_breakpoints.find(id);
if (it != m_breakpoints.end()) { if (it != m_breakpoints.end()) {
return &it->second; return it->second;
} }


return nullptr; return nullptr;
Expand All @@ -702,7 +719,7 @@ bool BreakpointManager::isBreakpointResolved(int id) const {
void BreakpointManager::onBreakpointHit(int id) { void BreakpointManager::onBreakpointHit(int id) {
const auto it = m_breakpoints.find(id); const auto it = m_breakpoints.find(id);
if (it != m_breakpoints.end()) { if (it != m_breakpoints.end()) {
Breakpoint* bp = &it->second; Breakpoint* bp = it->second;
bp->m_hitCount++; bp->m_hitCount++;
sendBreakpointEvent(id, ReasonChanged); sendBreakpointEvent(id, ReasonChanged);
} }
Expand All @@ -727,8 +744,8 @@ void BreakpointManager::sendBpError(


void BreakpointManager::onRequestShutdown(request_id_t requestId) { void BreakpointManager::onRequestShutdown(request_id_t requestId) {
for (auto it = m_breakpoints.begin(); it != m_breakpoints.end(); it++) { for (auto it = m_breakpoints.begin(); it != m_breakpoints.end(); it++) {
Breakpoint& bp = it->second; Breakpoint* bp = it->second;
bp.clearCachedConditionUnit(requestId); bp->clearCachedConditionUnit(requestId);
} }


// When a request ends, it no longer has any intercepted functions. // When a request ends, it no longer has any intercepted functions.
Expand Down
3 changes: 2 additions & 1 deletion hphp/runtime/ext/vsdebug/breakpoint.h
View file
Expand Up @@ -65,6 +65,7 @@ struct Breakpoint {
const std::string& hitCondition const std::string& hitCondition
); );


Breakpoint(const Breakpoint&) = delete;
virtual ~Breakpoint(); virtual ~Breakpoint();


void updateConditions( void updateConditions(
Expand Down Expand Up @@ -235,7 +236,7 @@ struct BreakpointManager {
ExceptionBreakpointSettings m_exceptionSettings; ExceptionBreakpointSettings m_exceptionSettings;


// The authoratative list of the current breakpoints set by the client. // The authoratative list of the current breakpoints set by the client.
std::unordered_map<int, Breakpoint> m_breakpoints; std::unordered_map<int, Breakpoint*> m_breakpoints;


// Map of source file name to list of breakpoints in that file. // Map of source file name to list of breakpoints in that file.
std::unordered_map<std::string, std::unordered_set<int>> m_sourceBreakpoints; std::unordered_map<std::string, std::unordered_set<int>> m_sourceBreakpoints;
Expand Down

0 comments on commit f371503

Please sign in to comment.