Mirror/zeek
1
0
Fork 0
mirror of https://github.com/zeek/zeek.git synced 2025-10-08 09:38:19 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Fix for disable_analyzer() problem.

Browse source 
When calling an Analyzer's method to remove a child analyzer, we now
postpone the actual removal to later, as otherwise the call to Done()
might trigger further analyzer activity that can interfere with code
running after that that triggered the removal.

This should fix the SSL assertion crashes that we have seen.

This change is a bit tricky internally, but the trace-based tests
produce the same output as before so things should be fine ...
This commit is contained in:
Robin Sommer 2011-07-22 16:19:24 -07:00
parent d01b8c9d3d
commit 8dc1a52d9d
2 changed files with 42 additions and 13 deletions
Download patch file Download diff file Expand all files Collapse all files

54
src/Analyzer.cc
View file

@ -243,6 +243,7 @@ Analyzer::Analyzer(AnalyzerTag::Tag arg_tag, Connection* arg_conn)
protocol_confirmed = false; protocol_confirmed = false;
skip = false; skip = false;
finished = false; finished = false;
removing = false;
parent = 0; parent = 0;
orig_supporters = 0; orig_supporters = 0;
resp_supporters = 0; resp_supporters = 0;
@ -424,10 +425,16 @@ void Analyzer::ForwardPacket(int len, const u_char* data, bool is_orig,
Analyzer* current = *i; Analyzer* current = *i;
next = ++i; next = ++i;
if ( ! current->finished ) if ( ! (current->finished || current->removing ) )
current->NextPacket(len, data, is_orig, seq, ip, caplen); current->NextPacket(len, data, is_orig, seq, ip, caplen);
else else
{ {
if ( removing )
{
current->Done();
removing = false;
}
// Analyzer has already been disabled so delete it. // Analyzer has already been disabled so delete it.
DBG_LOG(DBG_DPD, "%s deleted child %s", DBG_LOG(DBG_DPD, "%s deleted child %s",
fmt_analyzer(this).c_str(), fmt_analyzer(current).c_str()); fmt_analyzer(this).c_str(), fmt_analyzer(current).c_str());
@ -453,11 +460,17 @@ void Analyzer::ForwardStream(int len, const u_char* data, bool is_orig)
Analyzer* current = *i; Analyzer* current = *i;
next = ++i; next = ++i;
if ( ! current->finished ) if ( ! (current->finished || current->removing ) )
current->NextStream(len, data, is_orig); current->NextStream(len, data, is_orig);
else else
{ {
// Analyzer has already been disabled so delete it. // Analyzer has already been disabled so delete it.
if ( current->removing )
{
current->Done();
removing = false;
}
DBG_LOG(DBG_DPD, "%s deleted child %s", DBG_LOG(DBG_DPD, "%s deleted child %s",
fmt_analyzer(this).c_str(), fmt_analyzer(current).c_str()); fmt_analyzer(this).c_str(), fmt_analyzer(current).c_str());
children.erase(--i); children.erase(--i);
@ -482,10 +495,16 @@ void Analyzer::ForwardUndelivered(int seq, int len, bool is_orig)
Analyzer* current = *i; Analyzer* current = *i;
next = ++i; next = ++i;
if ( ! current->finished ) if ( ! (current->finished || current->removing ) )
current->NextUndelivered(seq, len, is_orig); current->NextUndelivered(seq, len, is_orig);
else else
{ {
if ( current->removing )
{
current->Done();
removing = false;
}
// Analyzer has already been disabled so delete it. // Analyzer has already been disabled so delete it.
DBG_LOG(DBG_DPD, "%s deleted child %s", DBG_LOG(DBG_DPD, "%s deleted child %s",
fmt_analyzer(this).c_str(), fmt_analyzer(current).c_str()); fmt_analyzer(this).c_str(), fmt_analyzer(current).c_str());
@ -508,10 +527,16 @@ void Analyzer::ForwardEndOfData(bool orig)
Analyzer* current = *i; Analyzer* current = *i;
next = ++i; next = ++i;
if ( ! current->finished ) if ( ! (current->finished || current->removing ) )
current->NextEndOfData(orig); current->NextEndOfData(orig);
else else
{ {
if ( current->removing )
{
current->Done();
removing = false;
}
// Analyzer has already been disabled so delete it. // Analyzer has already been disabled so delete it.
DBG_LOG(DBG_DPD, "%s deleted child %s", DBG_LOG(DBG_DPD, "%s deleted child %s",
fmt_analyzer(this).c_str(), fmt_analyzer(current).c_str()); fmt_analyzer(this).c_str(), fmt_analyzer(current).c_str());
@ -563,15 +588,17 @@ Analyzer* Analyzer::AddChildAnalyzer(AnalyzerTag::Tag analyzer)
void Analyzer::RemoveChildAnalyzer(Analyzer* analyzer) void Analyzer::RemoveChildAnalyzer(Analyzer* analyzer)
{ {
LOOP_OVER_CHILDREN(i) LOOP_OVER_CHILDREN(i)
if ( *i == analyzer && ! analyzer->finished ) if ( *i == analyzer && ! (analyzer->finished || analyzer->removing) )
{ {
DBG_LOG(DBG_DPD, "%s disabled child %s", DBG_LOG(DBG_DPD, "%s disabling child %s",
fmt_analyzer(this).c_str(), fmt_analyzer(*i).c_str()); fmt_analyzer(this).c_str(), fmt_analyzer(*i).c_str());
(*i)->Done(); // We just flag it as being removed here but postpone
// We don't delete it here as we may in fact // actually doing that to later. Otherwise, we'd need
// iterate over the list right now. Done() sets // to call Done() here, which then in turn might
// "finished" to true and this is checked // cause further code to be executed that may assume
// later to delete it. // something not true because of a violation that
// triggered the removal in the first place.
(*i)->removing = true;
return; return;
} }
} }
@ -579,11 +606,12 @@ void Analyzer::RemoveChildAnalyzer(Analyzer* analyzer)
void Analyzer::RemoveChildAnalyzer(AnalyzerID id) void Analyzer::RemoveChildAnalyzer(AnalyzerID id)
{ {
LOOP_OVER_CHILDREN(i) LOOP_OVER_CHILDREN(i)
if ( (*i)->id == id && ! (*i)->finished ) if ( (*i)->id == id && ! ((*i)->finished || (*i)->removing) )
{ {
DBG_LOG(DBG_DPD, "%s disabled child %s", GetTagName(), id, DBG_LOG(DBG_DPD, "%s disabled child %s", GetTagName(), id,
fmt_analyzer(this).c_str(), fmt_analyzer(*i).c_str()); fmt_analyzer(this).c_str(), fmt_analyzer(*i).c_str());
(*i)->Done(); // See comment above.
(*i)->removing = true;
return; return;
} }
} }

1
src/Analyzer.h
View file

@ -299,6 +299,7 @@ private:
bool timers_canceled; bool timers_canceled;
bool skip; bool skip;
bool finished; bool finished;
bool removing;
static AnalyzerID id_counter; static AnalyzerID id_counter;