[Bro-Dev] (no subject)

[Johanna Amann]

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.

On Fri, Feb 02, 2018 at 04:33:55PM +0000, Martina Balintova wrote:
> 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


>  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

> _______________________________________________
> bro-dev mailing list
> bro-dev at bro.org
> http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev