All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: 0xAX <kuleshovmail@gmail.com>
Cc: "git\@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: GIT: [PATCH] exec_cmd: system_path memory leak fix
Date: Sun, 23 Nov 2014 10:28:17 -0800	[thread overview]
Message-ID: <xmqqmw7hydoe.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1416750981-24446-1-git-send-email-kuleshovmail@gmail.com> (kuleshovmail@gmail.com's message of "Sun, 23 Nov 2014 19:56:20 +0600")

0xAX <kuleshovmail@gmail.com> writes:

> Hello All,
> I found memory leak at exec_cmd.c in system_path function with valgrind.
> After this patch valgrind doesn't show any memory leaks, but I'm not sure
> that it is the best way to solve this problem. There are a couple of places
> that uses system_path, if this way will be good, i'll make another patches
> to fix this leak.
>
> Waiting for feedback.

It is a bit sad that the callers of the function are prepared for
the returned value to be volatile (that is why the ones that want to
do something else between the time they call it and the time they
use the value returned do make copies), while other callers that
immediately use the returned value do not have to be worried about
freeing it, but with the change you have to force everybody to
remember freeing the returned value.

If you instead limited to your change only to these two points:

 - make "struct strbuf d" static;
 - return d.buf instead of the result of strbuf_detach(&d)

the updated system_path() will keep the promise all the caller
expects from it, i.e. the value out of the function is volatile but
the callers do not have to free it, in all cases, no?

> Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com>

Please have that S-o-b: line (and the same name in GIT_AUTHOR_NAME)
on the patches that we'd use "git am" on, not on the cover letter
;-).  Nom de guerre nobody else recognizes outside your close
friends may be fun, but I feel that it is not quite appropriate to
appear in "git shortlog -s" output on a public project like ours.

Thanks.

      parent reply	other threads:[~2014-11-23 18:28 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-23 13:56 GIT: [PATCH] exec_cmd: system_path memory leak fix 0xAX
2014-11-23 13:56 ` 0xAX
2014-11-23 14:01   ` 0xAX
2014-11-23 18:51   ` Junio C Hamano
2014-11-23 19:06     ` Alex Kuleshov
2014-11-23 19:19     ` Alex Kuleshov
2014-11-23 19:42       ` Jeff King
2014-11-23 20:07       ` Eric Sunshine
2014-11-23 21:58       ` Junio C Hamano
2014-11-24  7:02         ` Alex Kuleshov
2014-11-24  7:37           ` Junio C Hamano
2014-11-24  8:12             ` Alex Kuleshov
2014-11-24 13:11             ` Alexander Kuleshov
2014-11-24 14:00             ` Alex Kuleshov
2014-11-24 14:07               ` [PATCH] change contract between system_path and it's callers 0xAX
2014-11-24 19:33                 ` Re*: " Junio C Hamano
2014-11-24 19:53                   ` Alex Kuleshov
2014-11-24 20:20                     ` Junio C Hamano
2014-11-24 20:50                     ` Junio C Hamano
2014-11-25  6:45                       ` Alexander Kuleshov
2014-11-25  7:04                         ` Alexander Kuleshov
2014-11-25 17:55                           ` Junio C Hamano
2014-11-25 18:03                             ` Alexander Kuleshov
2014-11-25 18:24                               ` [PATCH 1/1] " Alexander Kuleshov
2014-11-25 21:13                                 ` Junio C Hamano
2014-11-26  3:53                                   ` Alexander Kuleshov
2014-11-26  9:42                                     ` Alexander Kuleshov
2014-11-26 14:00                                       ` Alexander Kuleshov
2014-11-26 17:53                                     ` Junio C Hamano
2014-11-28 13:09                                     ` Philip Oakley
2014-11-25 20:20                               ` Re*: [PATCH] " Junio C Hamano
2014-11-25 17:59                           ` Alexander Kuleshov
2014-11-23 18:28 ` Junio C Hamano [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqmw7hydoe.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=kuleshovmail@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.