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@bro.org > http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev _______________________________________________ bro-dev mailing list bro-dev@bro.org http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev