From: Alex Kuleshov Date: Mon, 24 Nov 2014 08:12:47 GMT Subject: Re: [PATCH] exec_cmd: system_path memory leak fix Message-ID: <87lhn1au67.fsf@gmail.com> In-Reply-To: Junio C Hamano @ 2014-11-24 13:37 ALMT: > [jc: added those who were mentioned but were missing back to Cc] > > On Sun, Nov 23, 2014 at 11:02 PM, Alex Kuleshov wrote: >> >> Junio C Hamano: >> >>>Fixing these callers are done as separate patches, that can be >>>applied either before or after this patch. >> >> How to do it better? Update this patch, fix all callers which broken and >> concat this patches to one or make separate patches? > > As I said, I do not think the approach your v2 takes is better than the original > approach to pass the ownership of the returned value to the caller. I'd say that > a cleaned up v1 that makes sure it adds a necessary strdup() in the codepath > where it returns an absolute pathname given as-is, with necessary changes to > callers that do not currently free the received result to free it when they are > done, and to callers that currently do strdup() of the received result not to do > strdup(), in a single patch, would be the right thing to do. > > I think I already wrote the bulk of proposed commit message for you for such > a change earlier ;-) The one that talks about changing the contract between the > system_path() and its callers. > > Thanks. OK, but i'm little confused now, long thread :) So what's our next strategy? I'll make system_path will return volatile value, so callers must free it by itself, but we have two types of callers: Callers which doesn't need to store system_path return value (like git --man-path and etc.. in git.c), and which need to store it (as in attr.c). We must to free system_path return value in first case, and does not need to create copy of returned string with xstrdup. Am i correct or i'm wrong with this? Thank you. -- Best regards. 0xAX