Re: [PATCH] exec_cmd: system_path memory leak fix

Alex Kuleshov <kuleshovmail@gmail.com> writes:

So this one does not change the contract between the caller and the callee. It does not change the fact that you need to explain your change, though. The story should read something like this:

    system_path(): always return a volatile result

    The function sometimes returns a newly allocated string and
    sometimes returns a borrowed string, the latter of which the
    callers must not free().

    The existing callers all assume that the return value belongs to
    the callee and most of them copy it with strdup() when they want
    to keep it around.  They end up leaking the returned copy when
    the callee returned a new string.

    Make sure all callers receive a volatile result, so that they do
    not have to be worried about freeing the returned string.

    There however are existing callers that are already broken, for
    example, ...

    Fixing these callers are done as separate patches, that can be
    applied either before or after this patch.

Personally, as I already said, I think the approach the previous version took (but not the implementation) to change the contract between the callers and this function is probably a good idea in the longer term. Leaking a bit by forgetting to convert a few callers to free the returned values will not affect the correctness, but making the returned value consistently more volatile will expose existing breakage more often and will break codepaths that happened to be working by accident.

Thanks.