RPM Community Forums

Mailing List Message of <popt-devel>

Re: POPT's API has designed in memory leaks. What to do?

From: Jeff Johnson <n3npq@mac.com>
Date: Mon 07 Jun 2010 - 22:49:15 CEST
Message-id: <9D764FC3-07BD-49ED-845A-74B489970095@mac.com>

On Jun 7, 2010, at 4:16 PM, Danny Sung wrote:

> 
> 
> On 6/7/2010 11:38 AM, Jeff Johnson wrote:
>> 
>> On Jun 7, 2010, at 2:00 PM, Danny Sung wrote:
>> 
>>> My personal choice in things I write is to expect the caller to strdup().  But I understand the reentrancy issue here.  (though why you'd be using popt in a thread is beyond me at this time.. and it does have a poptContext handle anyway).
>>> 
>> 
>> popt does not use threads, applications do. so its applications, not popt,
>> that force -lpopt into multi-threaded environments. All that POPT can
>> do is to try and work transparently in both environments.
> 
> Yeah, I meant as an app developer... I can't see why you'd want to use popt outside of a single thread.  That's the only context where reentrancy would be an issue while using the popt library, no?
> 

Ah ...

There's lots of usage cases for popt arg processing. E.g. RPM
has multiple embeddings and is just starting to explore the mysteries
of multi-threaded development (its taken most of the last year to
get posix mutexes stabilized and attached transparently to ~40 RPM objects)

POPT is used for all option processing and I want "thread safe" engineering.

> 
>>> If this is going to continue to be the behavior, I'd suggest a poptFreeOptArg() call just to highlight the necessity (and deal with type and NULL pointer checks).
>>> 
>> 
>> Why add a new method for what is already a well known operation called free(3)?
>> 
>> I mean I can add poptFreeArg in 1 #define to popt.h, but why bother?
> 
> Because it's not just "free()"   It's:
>   if(optarg) free((void *)optarg);
> 

(aside)
Hehe. FWIW, I deal with "free" issues (and the pesky GCC const warnings)
using a static inline _free() routine. I can will expose my _free() (its already
used pretty heavily internally to POPT) as poptFree() if you wish. So far I've just
tried not to inflict my C coding fetishism on anyone else. It is *in fact*
just laziness on the part of the developer to test for NULL and add the
silly cast. Developer laziness is not a POPT problem per se.

> Not a huge deal, but a bit of an inconvenience.  Plus like I said the added function just higlights the need to free the return from the Get call.  I actually have a str_delete() call in my standard library that encapsulates the if.  But I still have to call it like: str_delete((char *)optarg) just to break the const to satisfy the compiler warnings.
> 
> 
>> AFAIK, all POPT args are either returned by value (like POPT_ARG_INT) or
>> through malloc'd memory _ALWAYS_. The rules on table callbacks are different
>> than the rules for the no-brainer, in-line loop based getopt(3) like processing.
> 
> Yeah, it's fine.  As I said it was just a little unexpected when I first started using popt.  I didn't realize it until I ran a memory checker. And from the 'const' type in the Get call I had thought it was a bug in popt.
> 
> Looking over the man page right now (popt 1.16), I don't really see where it talks about freeing this memory either.  It's compounded by the fact that there is a poptFreeContext and poptDupArg() calls.  Those both imply to me that popt is doing memory management for me.
> 
> There's also a poptPeekArg() call.  Is the app developer expected to free that one as well?  If so, then the example seems to lead to a memory leak as well:
> 
>          portname = poptGetArg(optCon);
>          if((portname == NULL) || !(poptPeekArg(optCon) == NULL))
>             usage(optCon, 1, "Specify a single port", ".e.g., /dev/cua0");
> 

My guess (not looked, just from memory) is that poptPeekArg _SHOULD_ return
malloc'd arguments.

The underlying issue is that POPT has a ar filtering construct based on the
gawd awful bash syntax that looks like (here's an example from /usr/lib/rpm/rpmpopt-X.Y)

	#       [--dbpath DIRECTORY"    "use database in DIRECTORY"
	rpm     alias --dbpath          --define '_dbpath !#:+'

The --dbpath option is quite important for RPM and the ^%$^#^$^%#^%#$^% "!#:+"
syntax was quite useful in ripping out _LOTS_ of tedious code in RPM setting
what is essentially just a file path string argument to an option.

The relation to "memory leaks" is that POPT had to start actively parsing
argv[0] arrays in order to pull the next argument, and that turned out
to be easier to do if arguments were malloc'd before return to caller.

So poptPeekArg() _SHOULD_ return malloc'd memory (but may not do so atm
because I know that nothing but RPM has ever really used poptPeekArg() and
I've been fighting to simplify RPM arg handling for the last 10 years, mostly
successfully).


> Neither the return from poptGetArg() or poptPeekArg() are being freed.
> 

poptGetOptArg() is definitely malloc'd, that is the valgrind trace I posted.

I deal with the issue like this (again from RPM code in lib/poptALL.c with
spling pugliness left in cut-n-paste):

    /* Process all options, whine if unknown. */
    while ((rc = poptGetNextOpt(optCon)) > 0) {
        const char * optArg = poptGetOptArg(optCon);
/*@-dependenttrans -observertrans@*/    /* Avoid popt memory leaks. */
        optArg = _free(optArg);
/*@=dependenttrans =observertrans @*/
        switch (rc) {
        default:
/*@-nullpass@*/
            fprintf(stderr, _("%s: option table misconfigured (%d)\n"),
                __progname, rc);
/*@=nullpass@*/
            exit(EXIT_FAILURE);

            /*@notreached@*/ /*@switchbreak@*/ break;
        }
    }

The trace was from rpmio/poptIO.c (where I have never bothered
to "fix" the minor annoyance of a POPT memory leak).

> If this is the behavior you want, I'm fine with it.  But I think the man page needs updating.  (Apologies if it's already been updated, but I don't see it on my systems).
> 

Well technically, POPT CLI argument processing is no memory "leak" since
it happens only once, and the memory involved in argument strings is usually
in 10's of bytes, not otherwise.

But I can/will add anal retentive strict "make check" test cases
in POPT to fix these issues if necessary, and comply with some
"standard" API/ABI.

Note that popt-1.16 adds api-sanity-autotest.pl "shallow testing" from ISPRAS,
the same group that is doing the heavy duty implementation of LSB certification
testing.

Details are in popt/auto/* if interested. Nice Stuff (imho).

73 de Jeff

> If you'd like, I'll be happy to contribute these changes to the man page.  It just may take me a weeks before I have any free time.
> ______________________________________________________________________
> POPT Library                                           http://rpm5.org
> Developer Communication List                       popt-devel@rpm5.org
Received on Mon Jun 7 22:49:49 2010
Driven by Jeff Johnson and the RPM project team.
Hosted by OpenPKG and Ralf S. Engelschall.
Powered by FreeBSD and OpenPKG.