(no subject)

Development
1

Hi,
Here are 2 questions, one about usage of memory after free, another seems to me like memory leak. Could you please either confirm that these are bugs and suggest fixes or help me work out why no?

Function src/loggin/Manager.cc: bool Manager::Write(EnumVal* id, RecordVal* columns) seems to be using memory that might have been freed already. CreateWriter function can delete info_and_fields and still return non null writer. Any suggestions how to fix it? Maybe do not delete info in CreateWriter if old writer still exists? Will info’s memory be freed somewhere later?

I do not have a test case for this issue though.

// CreateWriter() will set the other fields in info.

writer = CreateWriter(stream->id, filter->writer,
info, filter->num_fields, arg_fields, filter->local,
filter->remote, false, filter->name);

if ( ! writer )
{
Unref(columns);
return false;
}
}

// Alright, can do the write now.

threading::Value** vals = RecordToFilterVals(stream, filter, columns);

if ( ! PLUGIN_HOOK_WITH_RESULT(HOOK_LOG_WRITE,
HookLogWrite(filter->writer->Type()->AsEnumType()->Lookup(filter->writer->InternalInt()),
filter->name, *info,
filter->num_fields,
filter->fields, vals), true) )

Another question is - inside src/input/Manager.cc → a lot of places where ev is allocated from EnumVal, it is not being freed or Unrefed. Eg in function Manager::Delete(…reader, vals) , it seems like predidx and ev are allocated, but are not freed if there was not convert_error. This looks like memory is leaked in all those cases.

if ( stream->pred )
{
int startpos = 0;
Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos, convert_error);

if ( convert_error )
Unref(predidx);
else
{
Ref(val);
EnumVal *ev = new EnumVal(BifEnum::Input::EVENT_REMOVED, BifType::Enum::Input::Event);

streamresult = CallPred(stream->pred, 3, ev, predidx, val);

if ( streamresult == false )
{
// keep it.
Unref(idxval);
success = true;
}
}

}

Thank you for suggestions,

Martina

2

Hi Martina,

you have picked out one of the more confusing parts of Bro to look at. The
logging code is sadly quite complex - mostly because it has to handle a
lot of different cases.

Hi,
Here are 2 questions, one about usage of memory after free, another seems
to me like memory leak. Could you please either confirm that these are bugs
and suggest fixes or help me work out why no?

Function src/loggin/Manager.cc: bool Manager::Write(EnumVal* id, RecordVal*
columns) seems to be using memory that might have been freed already.
CreateWriter function can delete info_and_fields and still return non null
writer. Any suggestions how to fix it? Maybe do not delete info in
CreateWriter if old writer still exists? Will info's memory be freed
somewhere later?

This is not a problem. If you check the 2 cases in which
delete_info_and_fields are called, you see that one of them is when
FindStream returns a nullptr, and the other one is if the Steram already
exists in WriterMap.

If you look at the Manager::Write function, both of these cases are
checked before calling CreateStream - in both of these cases, CreateStream
will not be called. So - this write-after-use cannto happen.

I think I tried to think about a way to make this nicer looking in the
past, but was not able to find one.

Another question is - inside src/input/Manager.cc -> a lot of places where
ev is allocated from EnumVal, it is not being freed or Unrefed. Eg in
function Manager::Delete(...reader, vals) , it seems like predidx and ev
are allocated, but are not freed if there was not convert_error. This looks
like memory is leaked in all those cases.

The values here are only Unref'd in the error case, because they are
consumed, and will be deleted, by CallPred in the case where there is no
error. In this case they are basically "passed to scriptland" and their
freeing will be managed by the script interpreter.

Sadly it is a bit difficult to follow what happens in cases like these.
Basically, one needs to knpw if each function call that goes into the Bro
core will take ownership and free the data structure afterwards, or if
that is up to the calling function. Often this means looking the function
that is called up to see what happens there - and even then one has to run
tests afterwards to make sure one got it correct in all cases.

I hope this made a few things slightly more clear,
Johanna