threads / patch / 30392
Subject: [PATCH 0/2] Fix some constness errors in fetch-pack
mhagger@alum.mit.eduFrom: Michael Haggerty <mhagger@alum.mit.edu>
Fix some constness errors that I noticed while reading the code in builtin/fetch-pack.c.
Michael Haggerty (2): cmd_fetch_pack(): declare dest to be const cmd_fetch_pack(): fix constness problem and memory leak
builtin/fetch-pack.c | 152 +++++++++++++++++++++++++------------------------- 1 file changed, 77 insertions(+), 75 deletions(-)
-- 1.7.10
mhagger@alum.mit.edu[PATCH 1/2] cmd_fetch_pack(): declare dest to be const
From: Michael Haggerty <mhagger@alum.mit.edu>
There is no need for it to be non-const, and this avoids the need for casting away constness of argv elements.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> --- builtin/fetch-pack.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 10db15b..7e9d62f 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -901,7 +901,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) { int i, ret, nr_heads; struct ref *ref = NULL; - char *dest = NULL, **heads; + const char *dest = NULL; + char **heads; int fd[2]; char *pack_lockfile = NULL; char **pack_lockfile_ptr = NULL; @@ -971,7 +972,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) } usage(fetch_pack_usage); } - dest = (char *)arg; + dest = arg; heads = (char **)(argv + i + 1); nr_heads = argc - i - 1; break; @@ -1018,7 +1019,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) fd[0] = 0; fd[1] = 1; } else { - conn = git_connect(fd, (char *)dest, args.uploadpack, + conn = git_connect(fd, dest, args.uploadpack, args.verbose ? CONNECT_VERBOSE : 0); }
-- 1.7.10
mhagger@alum.mit.edu[PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
From: Michael Haggerty <mhagger@alum.mit.edu>
The old code cast away the constness of the strings passed to the function in argument argv[], which could result in their being modified by filter_refs(). Moreover, if refs were passed via stdin, then the memory allocated for them was never freed (though, of course, this function is only called once so it is not a real problem).
Fix both errors by copying *all* reference names into our own array and always freeing the array at the end of the function.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> ---
I understand that it is not crucial to free memory allocated in a cmd_*() function but it is unclear to me whether it is *preferred* to let the process clean up take care of things. If so, the last chunk of this patch can be omitted.
builtin/fetch-pack.c | 149 +++++++++++++++++++++++++------------------------- 1 file changed, 75 insertions(+), 74 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 7e9d62f..5f769e9 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -899,10 +899,11 @@ static void fetch_pack_setup(void) int cmd_fetch_pack(int argc, const char **argv, const char *prefix) { - int i, ret, nr_heads; + int i, ret; struct ref *ref = NULL; const char *dest = NULL; - char **heads; + int alloc_heads = 0, nr_heads = 0; + char **heads = NULL; int fd[2]; char *pack_lockfile = NULL; char **pack_lockfile_ptr = NULL; @@ -910,86 +911,82 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) packet_trace_identity("fetch-pack"); - nr_heads = 0; - heads = NULL; - for (i = 1; i < argc; i++) { + for (i = 1; i < argc && *argv[i] == '-'; i++) { const char *arg = argv[i]; - if (*arg == '-') { - if (!prefixcmp(arg, "--upload-pack=")) { - args.uploadpack = arg + 14; - continue; - } - if (!prefixcmp(arg, "--exec=")) { - args.uploadpack = arg + 7; - continue; - } - if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) { - args.quiet = 1; - continue; - } - if (!strcmp("--keep", arg) || !strcmp("-k", arg)) { - args.lock_pack = args.keep_pack; - args.keep_pack = 1; - continue; - } - if (!strcmp("--thin", arg)) { - args.use_thin_pack = 1; - continue; - } - if (!strcmp("--include-tag", arg)) { - args.include_tag = 1; - continue; - } - if (!strcmp("--all", arg)) { - args.fetch_all = 1; - continue; - } - if (!strcmp("--stdin", arg)) { - args.stdin_refs = 1; - continue; - } - if (!strcmp("-v", arg)) { - args.verbose = 1; - continue; - } - if (!prefixcmp(arg, "--depth=")) { - args.depth = strtol(arg + 8, NULL, 0); - continue; - } - if (!strcmp("--no-progress", arg)) { - args.no_progress = 1; - continue; - } - if (!strcmp("--stateless-rpc", arg)) { - args.stateless_rpc = 1; - continue; - } - if (!strcmp("--lock-pack", arg)) { - args.lock_pack = 1; - pack_lockfile_ptr = &pack_lockfile; - continue; - } - usage(fetch_pack_usage); + if (!prefixcmp(arg, "--upload-pack=")) { + args.uploadpack = arg + 14; + continue; + } + if (!prefixcmp(arg, "--exec=")) { + args.uploadpack = arg + 7; + continue; + } + if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) { + args.quiet = 1; + continue; + } + if (!strcmp("--keep", arg) || !strcmp("-k", arg)) { + args.lock_pack = args.keep_pack; + args.keep_pack = 1; + continue; + } + if (!strcmp("--thin", arg)) { + args.use_thin_pack = 1; + continue; + } + if (!strcmp("--include-tag", arg)) { + args.include_tag = 1; + continue; + } + if (!strcmp("--all", arg)) { + args.fetch_all = 1; + continue; + } + if (!strcmp("--stdin", arg)) { + args.stdin_refs = 1; + continue; + } + if (!strcmp("-v", arg)) { + args.verbose = 1; + continue; + } + if (!prefixcmp(arg, "--depth=")) { + args.depth = strtol(arg + 8, NULL, 0); + continue; } - dest = arg; - heads = (char **)(argv + i + 1); - nr_heads = argc - i - 1; - break; + if (!strcmp("--no-progress", arg)) { + args.no_progress = 1; + continue; + } + if (!strcmp("--stateless-rpc", arg)) { + args.stateless_rpc = 1; + continue; + } + if (!strcmp("--lock-pack", arg)) { + args.lock_pack = 1; + pack_lockfile_ptr = &pack_lockfile; + continue; + } + usage(fetch_pack_usage); } - if (!dest) + if (i < argc) + dest = argv[i++]; + else usage(fetch_pack_usage); + /* + * Copy refs from cmdline to new growable list, then append + * any refs from the standard input. + */ + ALLOC_GROW(heads, argc - i, alloc_heads); + for (; i < argc; i++) + heads[nr_heads++] = xstrdup(argv[i]); + if (args.stdin_refs) { - /* - * Copy refs from cmdline to new growable list, then - * append the refs from the standard input. - */ - int alloc_heads = nr_heads; - int size = nr_heads * sizeof(*heads); - heads = memcpy(xmalloc(size), heads, size); if (args.stateless_rpc) { - /* in stateless RPC mode we use pkt-line to read + /* + * in stateless RPC mode we use pkt-line to read * from stdin, until we get a flush packet */ static char line[1000]; @@ -1055,6 +1052,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) ref = ref->next; } + for (i = 0; i < nr_heads; ++i) + free(heads[i]); + free(heads); + return ret; }
-- 1.7.10
Nguyen Thai Ngoc DuyRe: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
On Wed, May 2, 2012 at 5:40 PM, <mhagger@alum.mit.edu> wrote:
> const char *arg = argv[i];
>
> - if (*arg == '-') {
> - if (!prefixcmp(arg, "--upload-pack=")) {
> - args.uploadpack = arg + 14;
> - continue;
> - }
> - if (!prefixcmp(arg, "--exec=")) {
> - args.uploadpack = arg + 7;
> - continue;
> - }
> - if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
> - args.quiet = 1;
> - continue;
> - }
> - if (!strcmp("--keep", arg) || !strcmp("-k", arg)) {
> - args.lock_pack = args.keep_pack;
> - args.keep_pack = 1;
> - continue;
> - }
> - if (!strcmp("--thin", arg)) {
> - args.use_thin_pack = 1;
> - continue;
> - }
> - if (!strcmp("--include-tag", arg)) {
> - args.include_tag = 1;
> - continue;
> - }
> - if (!strcmp("--all", arg)) {
> - args.fetch_all = 1;
> - continue;
> - }
> - if (!strcmp("--stdin", arg)) {
> - args.stdin_refs = 1;
> - continue;
> - }
> - if (!strcmp("-v", arg)) {
> - args.verbose = 1;
> - continue;
> - }
> - if (!prefixcmp(arg, "--depth=")) {
> - args.depth = strtol(arg + 8, NULL, 0);
> - continue;
> - }
> - if (!strcmp("--no-progress", arg)) {
> - args.no_progress = 1;
> - continue;
> - }
> - if (!strcmp("--stateless-rpc", arg)) {
> - args.stateless_rpc = 1;
> - continue;
> - }
> - if (!strcmp("--lock-pack", arg)) {
> - args.lock_pack = 1;
> - pack_lockfile_ptr = &pack_lockfile;
> - continue;
> - }
> - usage(fetch_pack_usage);
> + if (!prefixcmp(arg, "--upload-pack=")) {
> + args.uploadpack = arg + 14;
> + continue;
> + }
> + if (!prefixcmp(arg, "--exec=")) {
> + args.uploadpack = arg + 7;
> + continue;
> + }
> + if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
> + args.quiet = 1;
> + continue;
> + }
> + if (!strcmp("--keep", arg) || !strcmp("-k", arg)) {
> + args.lock_pack = args.keep_pack;
> + args.keep_pack = 1;
> + continue;
> + }
> + if (!strcmp("--thin", arg)) {
> + args.use_thin_pack = 1;
> + continue;
> + }
> + if (!strcmp("--include-tag", arg)) {
> + args.include_tag = 1;
> + continue;
> + }
> + if (!strcmp("--all", arg)) {
> + args.fetch_all = 1;
> + continue;
> + }
> + if (!strcmp("--stdin", arg)) {
> + args.stdin_refs = 1;
> + continue;
> + }
> + if (!strcmp("-v", arg)) {
> + args.verbose = 1;
> + continue;
> + }
> + if (!prefixcmp(arg, "--depth=")) {
> + args.depth = strtol(arg + 8, NULL, 0);
> + continue;
> }
> - dest = arg;
> - heads = (char **)(argv + i + 1);
> - nr_heads = argc - i - 1;
> - break;
> + if (!strcmp("--no-progress", arg)) {
> + args.no_progress = 1;
> + continue;
> + }
> + if (!strcmp("--stateless-rpc", arg)) {
> + args.stateless_rpc = 1;
> + continue;
> + }
> + if (!strcmp("--lock-pack", arg)) {
> + args.lock_pack = 1;
> + pack_lockfile_ptr = &pack_lockfile;
> + continue;
> + }
> + usage(fetch_pack_usage);Ugh, perhaps you can convert the above to parse_options() too while you're making changes in this part? You can say no, I'll do it (my itch anyway).
-- Duy
Michael HaggertyRe: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
On 05/02/2012 01:14 PM, Nguyen Thai Ngoc Duy wrote:
> On Wed, May 2, 2012 at 5:40 PM,<mhagger@alum.mit.edu> wrote:
>> const char *arg = argv[i];
>>
>> - if (*arg == '-') {
>> - if (!prefixcmp(arg, "--upload-pack=")) {
>> - args.uploadpack = arg + 14;
>> - continue;
>> - }
>> [...]
> Ugh, perhaps you can convert the above to parse_options() too while
> you're making changes in this part? You can say no, I'll do it (my
> itch anyway).Sorry, I have too many balls in the air already. But feel free to veto my patch if my changes make it harder to switch to parse_options().
Michael
-- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/
Nguyễn Thái Ngọc Duy[PATCH 0/3] Fix some constness errors in fetch-pack and parseopt conversion
On Wed, May 2, 2012 at 8:35 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/02/2012 01:14 PM, Nguyen Thai Ngoc Duy wrote:
>>
>> On Wed, May 2, 2012 at 5:40 PM,<mhagger@alum.mit.edu> wrote:
>>>
>>> const char *arg = argv[i];
>>>
>>> - if (*arg == '-') {
>>> - if (!prefixcmp(arg, "--upload-pack=")) {
>>> - args.uploadpack = arg + 14;
>>> - continue;
>>> - }
>>> [...]
>>
>> Ugh, perhaps you can convert the above to parse_options() too while
>> you're making changes in this part? You can say no, I'll do it (my
>> itch anyway).
>
>
> Sorry, I have too many balls in the air already. But feel free to veto my
> patch if my changes make it harder to switch to parse_options().No problem. My new patch may slow down your patches to master though.
Michael Haggerty (2): cmd_fetch_pack(): declare dest to be const cmd_fetch_pack(): fix constness problem and memory leak
Nguyễn Thái Ngọc Duy (1): fetch-pack: use parse_options()
builtin/fetch-pack.c | 138 ++++++++++++++++++++------------------------------ fetch-pack.h | 20 ++++---- 2 files changed, 65 insertions(+), 93 deletions(-)
-- 1.7.8.36.g69ee2
Nguyễn Thái Ngọc Duy[PATCH 1/3] cmd_fetch_pack(): declare dest to be const
From: Michael Haggerty <mhagger@alum.mit.edu>
There is no need for it to be non-const, and this avoids the need for casting away constness of argv elements.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/fetch-pack.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 10db15b..7e9d62f 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -901,7 +901,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) { int i, ret, nr_heads; struct ref *ref = NULL; - char *dest = NULL, **heads; + const char *dest = NULL; + char **heads; int fd[2]; char *pack_lockfile = NULL; char **pack_lockfile_ptr = NULL; @@ -971,7 +972,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) } usage(fetch_pack_usage); } - dest = (char *)arg; + dest = arg; heads = (char **)(argv + i + 1); nr_heads = argc - i - 1; break; @@ -1018,7 +1019,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) fd[0] = 0; fd[1] = 1; } else { - conn = git_connect(fd, (char *)dest, args.uploadpack, + conn = git_connect(fd, dest, args.uploadpack, args.verbose ? CONNECT_VERBOSE : 0); }
-- 1.7.8.36.g69ee2
Nguyễn Thái Ngọc Duy[PATCH 2/3] fetch-pack: use parse_options()
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- I _think_ this also fixes a case when --keep is passed twice, then lock_pack is set to 1, but pack_lockfile_ptr is not set while it is set when --lock-pack is given.
builtin/fetch-pack.c | 110 ++++++++++++++++++-------------------------------- fetch-pack.h | 20 +++++----- 2 files changed, 49 insertions(+), 81 deletions(-)
builtin/fetch-pack.c, fetch-pack.h
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 7e9d62f..65ac111 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -10,6 +10,7 @@ #include "remote.h" #include "run-command.h" #include "transport.h" +#include "parse-options.h" static int transfer_unpack_limit = -1; static int fetch_unpack_limit = -1; @@ -22,10 +23,12 @@ static struct fetch_pack_args args = { /* .uploadpack = */ "git-upload-pack", }; -static const char fetch_pack_usage[] = -"git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] " -"[--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] " -"[--no-progress] [-v] [<host>:]<directory> [<refs>...]"; +static const char* fetch_pack_usage[] = { + "git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] " + "[--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] " + "[--no-progress] [-v] [<host>:]<directory> [<refs>...]", + NULL +}; #define COMPLETE (1U << 0) #define COMMON (1U << 1) @@ -906,79 +909,44 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) int fd[2]; char *pack_lockfile = NULL; char **pack_lockfile_ptr = NULL; + int progress = 1; struct child_process *conn; + struct option opts[] = { + OPT_STRING(0, "upload-pack", &args.uploadpack, "path", + "path to upload pack on remote end"), + OPT_STRING( 0 , "exec", &args.uploadpack, "upload-pack", + "path to upload pack on remote end"), + OPT__QUIET(&args.quiet, "do not print results to stdout"), + OPT_COUNTUP('k', "keep", &args.keep_pack, "keep downloaded pack"), + OPT_BOOL(0, "thin", &args.use_thin_pack, "create thin packs"), + OPT_BOOL(0, "include-tag", &args.include_tag, + "include tag objects that refer to objects to be packed"), + OPT_BOOL(0, "all", &args.fetch_all, "fetch from all remotes" ), + OPT_BOOL(0, "stdin", &args.stdin_refs, "read refs from stdin" ), + OPT__VERBOSE(&args.verbose, "be more verbose"), + OPT_INTEGER(0, "depth", &args.depth, + "deepen history of shallow clone"), + OPT_BOOL(0, "progress", &progress, "show progress"), + OPT_BOOL(0, "stateless-rpc", &args.stateless_rpc, "use stateless RPC"), + OPT_BOOL(0, "lock-pack", &args.lock_pack, "lock the downloaded pack"), + OPT_END() + }; packet_trace_identity("fetch-pack"); nr_heads = 0; heads = NULL; - for (i = 1; i < argc; i++) { - const char *arg = argv[i]; - - if (*arg == '-') { - if (!prefixcmp(arg, "--upload-pack=")) { - args.uploadpack = arg + 14; - continue; - } - if (!prefixcmp(arg, "--exec=")) { - args.uploadpack = arg + 7; - continue; - } - if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) { - args.quiet = 1; - continue; - } - if (!strcmp("--keep", arg) || !strcmp("-k", arg)) { - args.lock_pack = args.keep_pack; - args.keep_pack = 1; - continue; - } - if (!strcmp("--thin", arg)) { - args.use_thin_pack = 1; - continue; - } - if (!strcmp("--include-tag", arg)) { - args.include_tag = 1; - continue; - } - if (!strcmp("--all", arg)) { - args.fetch_all = 1; - continue; - } - if (!strcmp("--stdin", arg)) { - args.stdin_refs = 1; - continue; - } - if (!strcmp("-v", arg)) { - args.verbose = 1; - continue; - } - if (!prefixcmp(arg, "--depth=")) { - args.depth = strtol(arg + 8, NULL, 0); - continue; - } - if (!strcmp("--no-progress", arg)) { - args.no_progress = 1; - continue; - } - if (!strcmp("--stateless-rpc", arg)) { - args.stateless_rpc = 1; - continue; - } - if (!strcmp("--lock-pack", arg)) { - args.lock_pack = 1; - pack_lockfile_ptr = &pack_lockfile; - continue; - } - usage(fetch_pack_usage); - } - dest = arg; - heads = (char **)(argv + i + 1); - nr_heads = argc - i - 1; - break; - } - if (!dest) - usage(fetch_pack_usage); + argc = parse_options(argc, argv, prefix, opts, fetch_pack_usage, 0); + args.no_progress = !progress; + if (args.keep_pack > 1) + args.lock_pack = 1; + if (args.lock_pack) + pack_lockfile_ptr = &pack_lockfile; + dest = argv[0]; + if (!argc || !dest) + usage_with_options(fetch_pack_usage, opts); + heads = (char **)(argv + 1); + nr_heads = argc - 1; if (args.stdin_refs) { /* diff --git a/fetch-pack.h b/fetch-pack.h index 7c2069c..d440162 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -5,16 +5,16 @@ struct fetch_pack_args { const char *uploadpack; int unpacklimit; int depth; - unsigned quiet:1, - keep_pack:1, - lock_pack:1, - use_thin_pack:1, - fetch_all:1, - stdin_refs:1, - verbose:1, - no_progress:1, - include_tag:1, - stateless_rpc:1; + int quiet; + int keep_pack; + int lock_pack; + int use_thin_pack; + int fetch_all; + int stdin_refs; + int verbose; + int no_progress; + int include_tag; + int stateless_rpc; }; struct ref *fetch_pack(struct fetch_pack_args *args,
-- 1.7.8.36.g69ee2
Nguyễn Thái Ngọc Duy[PATCH 3/3] cmd_fetch_pack(): fix constness problem and memory leak
From: Michael Haggerty <mhagger@alum.mit.edu>
The old code cast away the constness of the strings passed to the function in argument argv[], which could result in their being modified by filter_refs(). Moreover, if refs were passed via stdin, then the memory allocated for them was never freed (though, of course, this function is only called once so it is not a real problem).
Fix both errors by copying *all* reference names into our own array and always freeing the array at the end of the function.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/fetch-pack.c | 31 +++++++++++++++++-------------- 1 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 65ac111..beabdc2 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -902,10 +902,11 @@ static void fetch_pack_setup(void) int cmd_fetch_pack(int argc, const char **argv, const char *prefix) { - int i, ret, nr_heads; + int i, ret; struct ref *ref = NULL; const char *dest = NULL; - char **heads; + int alloc_heads = 0, nr_heads = 0; + char **heads = NULL; int fd[2]; char *pack_lockfile = NULL; char **pack_lockfile_ptr = NULL; @@ -934,8 +935,6 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) packet_trace_identity("fetch-pack"); - nr_heads = 0; - heads = NULL; argc = parse_options(argc, argv, prefix, opts, fetch_pack_usage, 0); args.no_progress = !progress; if (args.keep_pack > 1) @@ -945,19 +944,19 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) dest = argv[0]; if (!argc || !dest) usage_with_options(fetch_pack_usage, opts); - heads = (char **)(argv + 1); - nr_heads = argc - 1; + + /* + * Copy refs from cmdline to new growable list, then append + * any refs from the standard input. + */ + ALLOC_GROW(heads, argc - 1, alloc_heads); + for (i = 1; i < argc; i++) + heads[nr_heads++] = xstrdup(argv[i]); if (args.stdin_refs) { - /* - * Copy refs from cmdline to new growable list, then - * append the refs from the standard input. - */ - int alloc_heads = nr_heads; - int size = nr_heads * sizeof(*heads); - heads = memcpy(xmalloc(size), heads, size); if (args.stateless_rpc) { - /* in stateless RPC mode we use pkt-line to read + /* + * in stateless RPC mode we use pkt-line to read * from stdin, until we get a flush packet */ static char line[1000]; @@ -1023,6 +1022,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) ref = ref->next; } + for (i = 0; i < nr_heads; ++i) + free(heads[i]); + free(heads); + return ret; }
-- 1.7.8.36.g69ee2
Junio C HamanoRe: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> Ugh, perhaps you can convert the above to parse_options() too while > you're making changes in this part?
Please don't. A parse-opt'ification is a very low priority change for a command nobody runs from the command line, and is not worth the hassle of having to deal with unnecessary merge conflicts.
Do so when the codebase around the area is otherwise quiet.
Junio C HamanoRe: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
mhagger@alum.mit.edu writes:
> I understand that it is not crucial to free memory allocated in a > cmd_*() function but it is unclear to me whether it is *preferred* to > let the process clean up take care of things.
Traditionally, the cmd_foo() functions roughly correspond to main() in other programs, so from that point of view, "it is not crucial to free" is an understatement. It is not even worth wasting people's time to (1) decide which way is *preferred* and to (2) churn the code only to match whichever way.
These cmd_foo() functions, being roughly equivalent to main(), have logic to interpret the end-user's intentions (i.e. parse_options()), and carry out that intention. In the long run, some _other_ parts of the system may want to do "foo" (whatever that "foo" may be) from inside the process without forking, and the first step to do so may be to split cmd_foo() into two, one that does "parse options", and the other that does "foo". At that point, it starts to matter that we make the part that does "foo" leak free, especially if such a caller (or callers) can ask to do "foo" number of times.
If you have a plan to split cmd_fetch_pack() and make other parts of the system call it, probably restructuring the current separation between "figure out what refs will be fetched" done in cmd_fetch_pack() and "fetch these refs in heads[] array" in fetch_pack() into something else (like "the new caller also wants to read the list of refs from the standard input, instead of having parse them into head[] array itself"), then freeing the memory would matter a lot more, and the approach this patch takes is a first step in the right direction, I would think.
It also seems that some cruft has snuck into this patch, e.g. like this part,
> - int i, ret, nr_heads; > + int i, ret;
that do not have anything to do with "fix constness" nor "memory leak".
Michael HaggertyRe: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
On 05/21/2012 03:47 AM, Junio C Hamano wrote:
> mhagger@alum.mit.edu writes: > >> I understand that it is not crucial to free memory allocated in a >> cmd_*() function but it is unclear to me whether it is *preferred* to >> let the process clean up take care of things. > > Traditionally, the cmd_foo() functions roughly correspond to main() in > other programs, so from that point of view, "it is not crucial to free" is > an understatement. It is not even worth wasting people's time to (1) > decide which way is *preferred* and to (2) churn the code only to match > whichever way.
OK, thanks for the info. I will remove the "freeing-memory" part of the patch.
> If you have a plan to split cmd_fetch_pack() and make other parts of the > system call it, [...]
No, I have no plans for cmd_fetch_pack() besides cleaning up the constness errors that I found when randomly reading the code.
> It also seems that some cruft has snuck into this patch, e.g. like this > part, > >> - int i, ret, nr_heads; >> + int i, ret; > > that do not have anything to do with "fix constness" nor "memory leak".
This particular hunk is part of moving alloc_heads, nr_heads, and heads together to make it more obvious that they are part of an ALLOC_GROW triplet. Previously alloc_heads was a block-local variable used only in the case of the --stdin option.
But I admit that the patch is harder than necessary to read because of the indentation changes etc, so I will break it up into more digestible quanta.
Michael
-- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/
Michael HaggertyRe: [PATCH 0/2] Fix some constness errors in fetch-pack
On 05/02/2012 12:40 PM, mhagger@alum.mit.edu wrote:
> From: Michael Haggerty<mhagger@alum.mit.edu> > > Fix some constness errors that I noticed while reading the code in > builtin/fetch-pack.c. > > Michael Haggerty (2): > cmd_fetch_pack(): declare dest to be const > cmd_fetch_pack(): fix constness problem and memory leak > > builtin/fetch-pack.c | 152 +++++++++++++++++++++++++------------------------- > 1 file changed, 77 insertions(+), 75 deletions(-)
I think that these patches got dropped because of an inconclusive discussion [1]: Duy suggested converting the code for fetch-pack to use parse_options(), which idea Junio didn't like. But I think that any objections were raised against these two patches, which are just simple cleanups.
Michael
[1] http://comments.gmane.org/gmane.comp.version-control.git/196797
-- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/