Wireshark-dev: Re: [Wireshark-dev] leaking memory on shutdown

From: Evan Huus <eapache@xxxxxxxxx>
Date: Fri, 28 Jun 2013 09:21:27 -0400
On Fri, Jun 28, 2013 at 1:51 AM, Jakub Zawadzki
<darkjames-ws@xxxxxxxxxxxx> wrote:
> Hi,
>
> On Thu, Jun 27, 2013 at 08:22:27PM -0400, Evan Huus wrote:
>> On Thu, Jun 27, 2013 at 5:25 PM, Jeff Morriss <jeff.morriss.ws@xxxxxxxxx> wrote:
>> > [Taking discussion out of the bug since it's not specifically related to
>> > that bug.]
>> >
>> > But, more seriously, Guy pointed out a (long?) while ago that a certain OS
>> > from a company he knows well has a way for applications to tell the OS
>> > something like "I have nothing to save before exiting" and when that flag is
>> > set and the user closes the app, the OS kills it with SIGKILL: it's faster
>> > and simpler that way.  It's an interesting concept...  It makes me think
>> > that we shouldn't really be spending time trying to fix leaks on shutdown.
>
> https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=4140#c16 ;-)
>
>> The one benefit of fixing leaks on shutdown is that it makes it a
>> million times easier to detect other (real) leaks because valgrind's
>> output becomes useful. Right now any valgrind run you do with
>> --leak-check=full is painfully difficult to analyze because of all the
>> garbage (and dynamic hf arrays) that count as 'leaks'.
>
> Have you tried creating suppression file?
> http://wiki.wxwidgets.org/Valgrind_Suppression_File_Howto

Suppression files are for hiding errors you can't control in
third-party libraries. I suppose you could abuse them for this
purpose, but it leaves a bad taste in my mouth, and I'm not entirely
sure if it would play well with indirect leaks. It would also be very
hard to suppress the right errors without hiding real errors as well.

I have no problem with applications being lazy in order to shutdown
fast. However, libraries are a different matter. They have no
guarantees that they're being unloaded at the time of application
shutdown, so they should provide a function to do full cleanup, even
if a particular application doesn't need to use it. Epan is terrible
in this respect (thus why the new echild stuff has to keep spawning
new instances of it in separate processes, or whatever exactly it's
doing).

The correct approach, I think, is to fix epan_cleanup() so that it
actually cleans up *everything* owned by epan, but then simply not
call it if we're about to exit. If there is some cleanup we do need to
do, then epan should provide an epan_cleanup_fast() or
epan_cleanup_minimal() or similar to call in those cases.

Evan