[OPEN-ILS-DEV] PATCH: oils_requestor.c memory leaks

[Dan Scott]

BTW, Scott, if you install VMWare then there are a couple of VMWare
images that you could use to pretty easily test your suggested
changes. The Gentoo image retains the complete set of build tools +
the CVS tree, for example.

But hey, I don't think anyone is going to complain if you keep sending
out untested patches that are easy to apply and make sense :)

On 04/05/07, Scott McKellar <mck9 at swbell.net> wrote:
> The attached patch plugs some memory leaks and tidies up a few other
> things in ILS/Open-ILS/src/extras/oils_requestor.c.
>
> 1. I made the functions do_request() and format_response() static
> instead of global, since there appears to be no reference to them
> elsewhere in the code base.
>
> 2. In main() I changed the type of c from char to int.  That is the
> type returned by getopt(), and the type to which c is compared in
> the switch/case structure.  (Contrary to one's intuition, the type
> of a character literal in C is int, not char.) (Yes, I know I'm
> being really picky.)
>
> 3. In the main loop toward the end of main(), I contrived to free
> the request allocated by readline().  Otherwise we leak a chunk of
> memory with every iteration.
>
> 4. At the end of main() I freed several buffers that were potentially
> allocated as copies of command line parameters.  We were already
> freeing authtoken and idl_filename, so we might as well be consistent.
> Alternatively we could free none of them, since we're about to exit
> anyway.
>
> Note that we still have memory leaks in the case of early exits due
> to errors.  Whether these leaks are worth plugging depends on how
> compulsive you like to be about such things.
>
> Since I'm too lazy to do a full build and installation, I tested
> with a rump version that had most of the functionality commented
> out.  Then I copied my changes to an intact version of the code
> and made the patch.  Since I haven't actually compiled the version
> from which I made the patch, it is possible that I introduced some
> error along the way.
>
> Here are some further suggestions.  I can create a patch for any or
> all of them but first I want to get some feedback.
>
> 1. The authtoken variable could almost certainly be declared as local
> within main().  I see no reference to it elsewhere in the code base,
> nor any reason for it to be global.
>
> 2. Likewise for the script variable, but I haven't looked at it
> closely.
>
> 3. We accept a -s option and load its argument into the script
> variable, but then we don't do anything with it.  If we're not going
> to do anything with this option, we should get rid of it.  (There may
> however be issues of backward compatibility, if this option is merely
> obsolete.)
>
> 4. I doubt that we need to make copies of the command line arguments.
> For example, instead of saying:
>
>     username    = strdup(optarg);
>
> ...we could just say:
>
>    username     = optarg;
>
> Then we wouldn't have to worry about freeing those buffers.
>
> Scott McKellar
> http://home.swbell.net/mck9/aargh/
>
>


-- 
Dan Scott
Laurentian University