Wireshark-bugs: [Wireshark-bugs] [Bug 9033] wmem assertion after changing preferences before loa
Comment # 5
on bug 9033
from Evan Huus
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > > There are probably two issues here:
> > > > - rescan_packets should bail because there is no file open
> > > > - if it doesn't bail, it should clean up the epan it creates so that wmem
> > > > leaves file scope
> > >
> > > - wmem shouldn't do stupid assertions.
> >
> > These are the kind of weird bugs that those assertions are meant to catch.
> > If it hadn't asserted there is the possibility of all sorts of bizarre
> > subtle errors.
>
> What about removing wmem_epan_scope(), wmem_file_scope(),
> wmem_packet_scope()?
>
> Replace with helper functions:
> proto_alloc() <-- alloc memory for libwireshark
> epan_alloc(epan) <-- alloc memory for given session
> packet_alloc(pinfo) <-- alloc memory for given packet
>
> (These two I think are mostly used, so it'd be great if we have some
> helpers, but we don't strictly need to)
>
> In other cases pass pool to some function:
> wmem_foo(epan_pool(epan), ...)
> wmem_foo(packet_pool(pinfo), ...)
>
> If you have valid epan or pinfo pointer it's much more likely that it'll
> work ;]
This was the original intention (see the comment at the top of wmem_scopes.c).
If you want to do the work of passing the epan and pinfo pointers *everywhere*
then I'm happy to use them :P
> I'm aware of trees created in proto_register_ routines,
> but if we support multi-files, we'd anyway need some functions for
> dissectors 'call me when new session is created, destroyed' (best auto
> generated - like proto_reg_*, proto_register_*).
>
> > The broader problem is that we unconditionally call epan_free (previously
> > cleanup_dissection) in places like cf_open (tshark.c:3806) where we don't
> > necessarily have a file open.
>
> > epan_free should be called when the old file
> > is closed, not when the new file is opened.
>
> Why? free(0) is NOP. It'd be best if epan_free(0) would be also NOP.
Good point. But we should free on file close anyways, since leaving the
resources around until they open a new file is wasteful (and potentially a leak
when we have more than one file open).
> > Rescan_packets has an 'excuse'
> > in that it can reasonably expect to only be called if there is a file open.
>
> If we really need to.
> btw. what about testing for cf->epan? It's shorter than cf->state !=
> FILE_CLOSED.
Also a good point.
You are receiving this mail because:
- You are watching all bug changes.
