* [PATCH v2 01/25] mingw: avoid memory leak when splitting PATH
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
@ 2017-04-28 13:49 ` Johannes Schindelin
2017-04-28 13:49 ` [PATCH v2 02/25] winansi: avoid use of uninitialized value Johannes Schindelin
` (24 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 13:49 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
In the (admittedly, concocted) case that PATH consists only of colons, we
would leak the duplicated string.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/mingw.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 3fbfda5978b..fe0e3ccd248 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -961,8 +961,10 @@ static char **get_path_split(void)
++n;
}
}
- if (!n)
+ if (!n) {
+ free(envpath);
return NULL;
+ }
ALLOC_ARRAY(path, n + 1);
p = envpath;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 02/25] winansi: avoid use of uninitialized value
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
2017-04-28 13:49 ` [PATCH v2 01/25] mingw: avoid memory leak when splitting PATH Johannes Schindelin
@ 2017-04-28 13:49 ` Johannes Schindelin
2017-04-28 13:49 ` [PATCH v2 03/25] winansi: avoid buffer overrun Johannes Schindelin
` (23 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 13:49 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
When stdout is not connected to a Win32 console, we incorrectly used an
uninitialized value for the "plain" character attributes.
Detected by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/winansi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/compat/winansi.c b/compat/winansi.c
index 793420f9d0d..fd6910746c8 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -105,6 +105,8 @@ static int is_console(int fd)
if (!fd) {
if (!GetConsoleMode(hcon, &mode))
return 0;
+ sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN |
+ FOREGROUND_RED;
} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
return 0;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 03/25] winansi: avoid buffer overrun
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
2017-04-28 13:49 ` [PATCH v2 01/25] mingw: avoid memory leak when splitting PATH Johannes Schindelin
2017-04-28 13:49 ` [PATCH v2 02/25] winansi: avoid use of uninitialized value Johannes Schindelin
@ 2017-04-28 13:49 ` Johannes Schindelin
2017-04-28 13:50 ` [PATCH v2 04/25] add_commit_patch_id(): avoid allocating memory unnecessarily Johannes Schindelin
` (22 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 13:49 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
When we could not convert the UTF-8 sequence into Unicode for writing to
the Console, we should not try to write an insanely-long sequence of
invalid wide characters (mistaking the negative return value for an
unsigned length).
Reported by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/winansi.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/compat/winansi.c b/compat/winansi.c
index fd6910746c8..861b79d8c31 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -135,6 +135,11 @@ static void write_console(unsigned char *str, size_t len)
/* convert utf-8 to utf-16 */
int wlen = xutftowcsn(wbuf, (char*) str, ARRAY_SIZE(wbuf), len);
+ if (wlen < 0) {
+ wchar_t *err = L"[invalid]";
+ WriteConsoleW(console, err, wcslen(err), &dummy, NULL);
+ return;
+ }
/* write directly to console */
WriteConsoleW(console, wbuf, wlen, &dummy, NULL);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 04/25] add_commit_patch_id(): avoid allocating memory unnecessarily
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (2 preceding siblings ...)
2017-04-28 13:49 ` [PATCH v2 03/25] winansi: avoid buffer overrun Johannes Schindelin
@ 2017-04-28 13:50 ` Johannes Schindelin
2017-04-28 13:50 ` [PATCH v2 05/25] git_config_rename_section_in_file(): avoid resource leak Johannes Schindelin
` (21 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 13:50 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
It would appear that we allocate (and forget to release) memory if the
patch ID is not even defined.
Reported by the Coverity tool.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
patch-ids.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/patch-ids.c b/patch-ids.c
index fa8f11de826..92eba7a059e 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -99,11 +99,12 @@ struct patch_id *has_commit_patch_id(struct commit *commit,
struct patch_id *add_commit_patch_id(struct commit *commit,
struct patch_ids *ids)
{
- struct patch_id *key = xcalloc(1, sizeof(*key));
+ struct patch_id *key;
if (!patch_id_defined(commit))
return NULL;
+ key = xcalloc(1, sizeof(*key));
if (init_patch_id_entry(key, commit, ids)) {
free(key);
return NULL;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 05/25] git_config_rename_section_in_file(): avoid resource leak
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (3 preceding siblings ...)
2017-04-28 13:50 ` [PATCH v2 04/25] add_commit_patch_id(): avoid allocating memory unnecessarily Johannes Schindelin
@ 2017-04-28 13:50 ` Johannes Schindelin
2017-04-28 13:50 ` [PATCH v2 06/25] get_mail_commit_oid(): " Johannes Schindelin
` (20 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 13:50 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
In case of errors, we really want the file descriptor to be closed.
Discovered by a Coverity scan.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index b4a3205da32..a30056ec7e9 100644
--- a/config.c
+++ b/config.c
@@ -2621,7 +2621,7 @@ int git_config_rename_section_in_file(const char *config_filename,
struct lock_file *lock;
int out_fd;
char buf[1024];
- FILE *config_file;
+ FILE *config_file = NULL;
struct stat st;
if (new_name && !section_name_is_ok(new_name)) {
@@ -2703,11 +2703,14 @@ int git_config_rename_section_in_file(const char *config_filename,
}
}
fclose(config_file);
+ config_file = NULL;
commit_and_out:
if (commit_lock_file(lock) < 0)
ret = error_errno("could not write config file %s",
config_filename);
out:
+ if (config_file)
+ fclose(config_file);
rollback_lock_file(lock);
out_no_rollback:
free(filename_buf);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 06/25] get_mail_commit_oid(): avoid resource leak
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (4 preceding siblings ...)
2017-04-28 13:50 ` [PATCH v2 05/25] git_config_rename_section_in_file(): avoid resource leak Johannes Schindelin
@ 2017-04-28 13:50 ` Johannes Schindelin
2017-04-28 13:50 ` [PATCH v2 07/25] difftool: address a couple of resource/memory leaks Johannes Schindelin
` (19 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 13:50 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
When we fail to read, or parse, the file, we still want to close the file
descriptor and release the strbuf.
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/am.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index a95dd8b4e6c..9c5c2c778e2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1351,19 +1351,16 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail)
struct strbuf sb = STRBUF_INIT;
FILE *fp = xfopen(mail, "r");
const char *x;
+ int ret = 0;
- if (strbuf_getline_lf(&sb, fp))
- return -1;
-
- if (!skip_prefix(sb.buf, "From ", &x))
- return -1;
-
- if (get_oid_hex(x, commit_id) < 0)
- return -1;
+ if (strbuf_getline_lf(&sb, fp) ||
+ !skip_prefix(sb.buf, "From ", &x) ||
+ get_oid_hex(x, commit_id) < 0)
+ ret = -1;
strbuf_release(&sb);
fclose(fp);
- return 0;
+ return ret;
}
/**
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 07/25] difftool: address a couple of resource/memory leaks
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (5 preceding siblings ...)
2017-04-28 13:50 ` [PATCH v2 06/25] get_mail_commit_oid(): " Johannes Schindelin
@ 2017-04-28 13:50 ` Johannes Schindelin
2017-04-28 13:50 ` [PATCH v2 08/25] status: close file descriptor after reading git-rebase-todo Johannes Schindelin
` (18 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 13:50 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
This change plugs a couple of memory leaks and makes sure that the file
descriptor is closed in run_dir_diff().
Spotted by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/difftool.c | 33 +++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 1354d0e4625..b9a892f2693 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -226,6 +226,7 @@ static void changed_files(struct hashmap *result, const char *index_path,
hashmap_entry_init(entry, strhash(buf.buf));
hashmap_add(result, entry);
}
+ fclose(fp);
if (finish_command(&diff_files))
die("diff-files did not exit properly");
strbuf_release(&index_env);
@@ -439,8 +440,10 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
}
if (lmode && status != 'C') {
- if (checkout_path(lmode, &loid, src_path, &lstate))
- return error("could not write '%s'", src_path);
+ if (checkout_path(lmode, &loid, src_path, &lstate)) {
+ ret = error("could not write '%s'", src_path);
+ goto finish;
+ }
}
if (rmode && !S_ISLNK(rmode)) {
@@ -456,9 +459,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
hashmap_add(&working_tree_dups, entry);
if (!use_wt_file(workdir, dst_path, &roid)) {
- if (checkout_path(rmode, &roid, dst_path, &rstate))
- return error("could not write '%s'",
- dst_path);
+ if (checkout_path(rmode, &roid, dst_path,
+ &rstate)) {
+ ret = error("could not write '%s'",
+ dst_path);
+ goto finish;
+ }
} else if (!is_null_oid(&roid)) {
/*
* Changes in the working tree need special
@@ -473,10 +479,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
ADD_CACHE_JUST_APPEND);
add_path(&rdir, rdir_len, dst_path);
- if (ensure_leading_directories(rdir.buf))
- return error("could not create "
- "directory for '%s'",
- dst_path);
+ if (ensure_leading_directories(rdir.buf)) {
+ ret = error("could not create "
+ "directory for '%s'",
+ dst_path);
+ goto finish;
+ }
add_path(&wtdir, wtdir_len, dst_path);
if (symlinks) {
if (symlink(wtdir.buf, rdir.buf)) {
@@ -497,13 +505,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
}
}
+ fclose(fp);
+ fp = NULL;
if (finish_command(&child)) {
ret = error("error occurred running diff --raw");
goto finish;
}
if (!i)
- return 0;
+ goto finish;
/*
* Changes to submodules require special treatment.This loop writes a
@@ -626,6 +636,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
exit_cleanup(tmpdir, rc);
finish:
+ if (fp)
+ fclose(fp);
+
free(lbase_dir);
free(rbase_dir);
strbuf_release(&ldir);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 08/25] status: close file descriptor after reading git-rebase-todo
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (6 preceding siblings ...)
2017-04-28 13:50 ` [PATCH v2 07/25] difftool: address a couple of resource/memory leaks Johannes Schindelin
@ 2017-04-28 13:50 ` Johannes Schindelin
2017-04-28 14:02 ` [PATCH v2 09/25] mailinfo & mailsplit: check for EOF while parsing Johannes Schindelin
` (17 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 13:50 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
wt-status.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/wt-status.c b/wt-status.c
index 03754849626..0a6e16dbe0f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1168,6 +1168,7 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines)
abbrev_sha1_in_line(&line);
string_list_append(lines, line.buf);
}
+ fclose(f);
return 0;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 09/25] mailinfo & mailsplit: check for EOF while parsing
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (7 preceding siblings ...)
2017-04-28 13:50 ` [PATCH v2 08/25] status: close file descriptor after reading git-rebase-todo Johannes Schindelin
@ 2017-04-28 14:02 ` Johannes Schindelin
2017-05-02 4:11 ` Junio C Hamano
2017-04-28 14:03 ` [PATCH v2 10/25] cat-file: fix memory leak Johannes Schindelin
` (16 subsequent siblings)
25 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
While POSIX states that it is okay to pass EOF to isspace() (and it seems
to be implied that EOF should *not* be treated as whitespace), and also to
pass EOF to ungetc() (which seems to be intended to fail without buffering
the character), it is much better to handle these cases explicitly. Not
only does it reduce head-scratching (and helps static analysis avoid
reporting false positives), it also lets us handle files containing
nothing but whitespace by erroring out.
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/mailsplit.c | 3 ++-
mailinfo.c | 15 +++++++++++----
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 30681681c13..9b3efc8e987 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -233,7 +233,8 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
do {
peek = fgetc(f);
} while (isspace(peek));
- ungetc(peek, f);
+ if (peek != EOF)
+ ungetc(peek, f);
if (strbuf_getwholeline(&buf, f, '\n')) {
/* empty stdin is OK */
diff --git a/mailinfo.c b/mailinfo.c
index 68037758f2f..a319911b510 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -882,7 +882,10 @@ static int read_one_header_line(struct strbuf *line, FILE *in)
for (;;) {
int peek;
- peek = fgetc(in); ungetc(peek, in);
+ peek = fgetc(in);
+ if (peek == EOF)
+ break;
+ ungetc(peek, in);
if (peek != ' ' && peek != '\t')
break;
if (strbuf_getline_lf(&continuation, in))
@@ -1094,14 +1097,18 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch)
return -1;
}
- mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
- mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
-
do {
peek = fgetc(mi->input);
+ if (peek == EOF) {
+ fclose(cmitmsg);
+ return error("empty patch: '%s'", patch);
+ }
} while (isspace(peek));
ungetc(peek, mi->input);
+ mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
+ mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
+
/* process the email header */
while (read_one_header_line(&line, mi->input))
check_header(mi, &line, mi->p_hdr_data, 1);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* Re: [PATCH v2 09/25] mailinfo & mailsplit: check for EOF while parsing
2017-04-28 14:02 ` [PATCH v2 09/25] mailinfo & mailsplit: check for EOF while parsing Johannes Schindelin
@ 2017-05-02 4:11 ` Junio C Hamano
2017-05-02 13:57 ` Johannes Schindelin
0 siblings, 1 reply; 178+ messages in thread
From: Junio C Hamano @ 2017-05-02 4:11 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Stefan Beller, Johannes Sixt, Jeff King
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> While POSIX states that it is okay to pass EOF to isspace() (and it seems
> to be implied that EOF should *not* be treated as whitespace), and also to
> pass EOF to ungetc() (which seems to be intended to fail without buffering
> the character), it is much better to handle these cases explicitly. Not
> only does it reduce head-scratching (and helps static analysis avoid
> reporting false positives), it also lets us handle files containing
> nothing but whitespace by erroring out.
>
> Reported via Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/mailsplit.c | 3 ++-
> mailinfo.c | 15 +++++++++++----
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> index 30681681c13..9b3efc8e987 100644
> --- a/builtin/mailsplit.c
> +++ b/builtin/mailsplit.c
> @@ -233,7 +233,8 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
> do {
> peek = fgetc(f);
> } while (isspace(peek));
> - ungetc(peek, f);
> + if (peek != EOF)
> + ungetc(peek, f);
I agree more with the first sentence in the proposed log message
than what this code actually does. I.e. breaking upon seeing an EOF
explicitly would be nice, just like the change to mailinfo.c in this
patch we see below.
> @@ -1094,14 +1097,18 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch)
> return -1;
> }
>
> - mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
> - mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
> -
> do {
> peek = fgetc(mi->input);
> + if (peek == EOF) {
> + fclose(cmitmsg);
> + return error("empty patch: '%s'", patch);
> + }
> } while (isspace(peek));
> ungetc(peek, mi->input);
The handling of EOF is improved, but is it correct to move the
allocation of p/s_hdr_data down?
Among the two callers, builtin/am.c just dies when it sees
mailinfo() returns an error, but builtin/mailinfo.c tries to be
nicer and calls clear_mailinfo(). Wouldn't this make that codepath
dereference a NULL pointer?
I think the moral of the story is that people tend to get sloppier
when doing "while we are at it" than the main task, and a reviewer
needs to be more careful while reviewing the "while we are at it"
part of the change than the primary thing a patch wants to do ;-)
> + mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
> + mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
> +
> /* process the email header */
> while (read_one_header_line(&line, mi->input))
> check_header(mi, &line, mi->p_hdr_data, 1);
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v2 09/25] mailinfo & mailsplit: check for EOF while parsing
2017-05-02 4:11 ` Junio C Hamano
@ 2017-05-02 13:57 ` Johannes Schindelin
0 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 13:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Stefan Beller, Johannes Sixt, Jeff King
Hi Junio,
On Mon, 1 May 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> > index 30681681c13..9b3efc8e987 100644
> > --- a/builtin/mailsplit.c
> > +++ b/builtin/mailsplit.c
> > @@ -233,7 +233,8 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
> > do {
> > peek = fgetc(f);
> > } while (isspace(peek));
> > - ungetc(peek, f);
> > + if (peek != EOF)
> > + ungetc(peek, f);
>
> I agree more with the first sentence in the proposed log message
> than what this code actually does. I.e. breaking upon seeing an EOF
> explicitly would be nice, just like the change to mailinfo.c in this
> patch we see below.
I changed it to error out with a (translated) "empty mbox: '%s'" as the
other hunks.
> > @@ -1094,14 +1097,18 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch)
> > return -1;
> > }
> >
> > - mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
> > - mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
> > -
> > do {
> > peek = fgetc(mi->input);
> > + if (peek == EOF) {
> > + fclose(cmitmsg);
> > + return error("empty patch: '%s'", patch);
> > + }
> > } while (isspace(peek));
> > ungetc(peek, mi->input);
>
> The handling of EOF is improved, but is it correct to move the
> allocation of p/s_hdr_data down?
Sorry, I *assumed* that the function was passed a zero-initialized
mailinfo struct. And as you pointed out, that assumption was wrong.
My thinking was that I did not want to introduce another leakage by my
patch, but as your careful analysis determined: the only caller that does
not die() afterwards releases the data (and would not even be able to
handle p_hdr_data == NULL...).
I reverted that move.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 178+ messages in thread
* [PATCH v2 10/25] cat-file: fix memory leak
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (8 preceding siblings ...)
2017-04-28 14:02 ` [PATCH v2 09/25] mailinfo & mailsplit: check for EOF while parsing Johannes Schindelin
@ 2017-04-28 14:03 ` Johannes Schindelin
2017-04-28 14:03 ` [PATCH v2 11/25] checkout: " Johannes Schindelin
` (15 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Discovered by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/cat-file.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1890d7a6390..9af863e7915 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -165,6 +165,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
die("git cat-file %s: bad file", obj_name);
write_or_die(1, buf, size);
+ free(buf);
return 0;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 11/25] checkout: fix memory leak
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (9 preceding siblings ...)
2017-04-28 14:03 ` [PATCH v2 10/25] cat-file: fix memory leak Johannes Schindelin
@ 2017-04-28 14:03 ` Johannes Schindelin
2017-04-28 14:03 ` [PATCH v2 12/25] split_commit_in_progress(): " Johannes Schindelin
` (14 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
This change addresses part of the NEEDSWORK comment above the code,
therefore the comment needs to be adjusted, too.
Discovered via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/checkout.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfa5419f335..5faea3a05fa 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -235,14 +235,14 @@ static int checkout_merged(int pos, const struct checkout *state)
/*
* NEEDSWORK:
* There is absolutely no reason to write this as a blob object
- * and create a phony cache entry just to leak. This hack is
- * primarily to get to the write_entry() machinery that massages
- * the contents to work-tree format and writes out which only
- * allows it for a cache entry. The code in write_entry() needs
- * to be refactored to allow us to feed a <buffer, size, mode>
- * instead of a cache entry. Such a refactoring would help
- * merge_recursive as well (it also writes the merge result to the
- * object database even when it may contain conflicts).
+ * and create a phony cache entry. This hack is primarily to get
+ * to the write_entry() machinery that massages the contents to
+ * work-tree format and writes out which only allows it for a
+ * cache entry. The code in write_entry() needs to be refactored
+ * to allow us to feed a <buffer, size, mode> instead of a cache
+ * entry. Such a refactoring would help merge_recursive as well
+ * (it also writes the merge result to the object database even
+ * when it may contain conflicts).
*/
if (write_sha1_file(result_buf.ptr, result_buf.size,
blob_type, oid.hash))
@@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct checkout *state)
if (!ce)
die(_("make_cache_entry failed for path '%s'"), path);
status = checkout_entry(ce, state, NULL);
+ free(ce);
return status;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 12/25] split_commit_in_progress(): fix memory leak
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (10 preceding siblings ...)
2017-04-28 14:03 ` [PATCH v2 11/25] checkout: " Johannes Schindelin
@ 2017-04-28 14:03 ` Johannes Schindelin
2017-04-28 14:03 ` [PATCH v2 13/25] setup_bare_git_dir(): help static analysis Johannes Schindelin
` (13 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
wt-status.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/wt-status.c b/wt-status.c
index 0a6e16dbe0f..1f3f6bcb980 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1088,8 +1088,13 @@ static int split_commit_in_progress(struct wt_status *s)
char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
- !s->branch || strcmp(s->branch, "HEAD"))
+ !s->branch || strcmp(s->branch, "HEAD")) {
+ free(head);
+ free(orig_head);
+ free(rebase_amend);
+ free(rebase_orig_head);
return split_in_progress;
+ }
if (!strcmp(rebase_amend, rebase_orig_head)) {
if (strcmp(head, rebase_amend))
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 13/25] setup_bare_git_dir(): help static analysis
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (11 preceding siblings ...)
2017-04-28 14:03 ` [PATCH v2 12/25] split_commit_in_progress(): " Johannes Schindelin
@ 2017-04-28 14:03 ` Johannes Schindelin
2017-04-28 14:03 ` [PATCH v2 14/25] setup_discovered_git_dir(): " Johannes Schindelin
` (12 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Coverity reported a memory leak in this function. However, it can only
be called once, as setup_git_directory() changes global state and hence
is not reentrant.
Mark the variable as static to indicate that this is a singleton.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
setup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/setup.c b/setup.c
index 0309c278218..0320a9ad14c 100644
--- a/setup.c
+++ b/setup.c
@@ -748,7 +748,7 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
- const char *gitdir;
+ static const char *gitdir;
gitdir = offset == cwd->len ? "." : xmemdupz(cwd->buf, offset);
if (chdir(cwd->buf))
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 14/25] setup_discovered_git_dir(): help static analysis
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (12 preceding siblings ...)
2017-04-28 14:03 ` [PATCH v2 13/25] setup_bare_git_dir(): help static analysis Johannes Schindelin
@ 2017-04-28 14:03 ` Johannes Schindelin
2017-05-02 3:57 ` Junio C Hamano
2017-04-28 14:03 ` [PATCH v2 15/25] pack-redundant: plug memory leak Johannes Schindelin
` (11 subsequent siblings)
25 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Coverity reported a memory leak in this function. However, it can only
be called once, as setup_git_directory() changes global state and hence
is not reentrant.
Mark the variable as static to indicate that this is a singleton.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
setup.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/setup.c b/setup.c
index 0320a9ad14c..12efca85a41 100644
--- a/setup.c
+++ b/setup.c
@@ -703,11 +703,16 @@ static const char *setup_discovered_git_dir(const char *gitdir,
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
+ char *p = NULL;
+ const char *ret;
+
if (offset != cwd->len && !is_absolute_path(gitdir))
- gitdir = real_pathdup(gitdir, 1);
+ gitdir = p = real_pathdup(gitdir, 1);
if (chdir(cwd->buf))
die_errno("Could not come back to cwd");
- return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
+ ret = setup_explicit_git_dir(gitdir, cwd, nongit_ok);
+ free(p);
+ return ret;
}
/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* Re: [PATCH v2 14/25] setup_discovered_git_dir(): help static analysis
2017-04-28 14:03 ` [PATCH v2 14/25] setup_discovered_git_dir(): " Johannes Schindelin
@ 2017-05-02 3:57 ` Junio C Hamano
2017-05-02 12:38 ` Johannes Schindelin
0 siblings, 1 reply; 178+ messages in thread
From: Junio C Hamano @ 2017-05-02 3:57 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Stefan Beller, Johannes Sixt, Jeff King
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Coverity reported a memory leak in this function. However, it can only
> be called once, as setup_git_directory() changes global state and hence
> is not reentrant.
>
> Mark the variable as static to indicate that this is a singleton.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
Does something different from what is explained above. Rebase gotcha?
> setup.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 0320a9ad14c..12efca85a41 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -703,11 +703,16 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>
> /* --work-tree is set without --git-dir; use discovered one */
> if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
> + char *p = NULL;
> + const char *ret;
> +
> if (offset != cwd->len && !is_absolute_path(gitdir))
> - gitdir = real_pathdup(gitdir, 1);
> + gitdir = p = real_pathdup(gitdir, 1);
> if (chdir(cwd->buf))
> die_errno("Could not come back to cwd");
> - return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
> + ret = setup_explicit_git_dir(gitdir, cwd, nongit_ok);
> + free(p);
> + return ret;
> }
>
> /* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v2 14/25] setup_discovered_git_dir(): help static analysis
2017-05-02 3:57 ` Junio C Hamano
@ 2017-05-02 12:38 ` Johannes Schindelin
0 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 12:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Stefan Beller, Johannes Sixt, Jeff King
Hi Junio,
On Mon, 1 May 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > Coverity reported a memory leak in this function. However, it can only
> > be called once, as setup_git_directory() changes global state and hence
> > is not reentrant.
> >
> > Mark the variable as static to indicate that this is a singleton.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
>
> Does something different from what is explained above. Rebase gotcha?
That, or just a brain fart.
Thanks, will send an updated iteration.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 178+ messages in thread
* [PATCH v2 15/25] pack-redundant: plug memory leak
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (13 preceding siblings ...)
2017-04-28 14:03 ` [PATCH v2 14/25] setup_discovered_git_dir(): " Johannes Schindelin
@ 2017-04-28 14:03 ` Johannes Schindelin
2017-04-28 14:03 ` [PATCH v2 16/25] mktree: plug memory leaks reported by Coverity Johannes Schindelin
` (10 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Identified via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pack-redundant.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 72c815844dd..cb1df1c7614 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -442,6 +442,7 @@ static void minimize(struct pack_list **min)
/* return if there are no objects missing from the unique set */
if (missing->size == 0) {
*min = unique;
+ free(missing);
return;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 16/25] mktree: plug memory leaks reported by Coverity
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (14 preceding siblings ...)
2017-04-28 14:03 ` [PATCH v2 15/25] pack-redundant: plug memory leak Johannes Schindelin
@ 2017-04-28 14:03 ` Johannes Schindelin
2017-04-28 14:03 ` [PATCH v2 17/25] fast-export: avoid leaking memory in handle_tag() Johannes Schindelin
` (9 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/mktree.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/mktree.c b/builtin/mktree.c
index de9b40fc63b..f0354bc9718 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -72,7 +72,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
unsigned mode;
enum object_type mode_type; /* object type derived from mode */
enum object_type obj_type; /* object type derived from sha */
- char *path;
+ char *path, *p = NULL;
unsigned char sha1[20];
ptr = buf;
@@ -102,7 +102,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
struct strbuf p_uq = STRBUF_INIT;
if (unquote_c_style(&p_uq, path, NULL))
die("invalid quoting");
- path = strbuf_detach(&p_uq, NULL);
+ path = p = strbuf_detach(&p_uq, NULL);
}
/*
@@ -136,6 +136,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
}
append_to_tree(mode, sha1, path);
+ free(p);
}
int cmd_mktree(int ac, const char **av, const char *prefix)
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 17/25] fast-export: avoid leaking memory in handle_tag()
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (15 preceding siblings ...)
2017-04-28 14:03 ` [PATCH v2 16/25] mktree: plug memory leaks reported by Coverity Johannes Schindelin
@ 2017-04-28 14:03 ` Johannes Schindelin
2017-04-28 14:03 ` [PATCH v2 18/25] receive-pack: plug memory leak in update() Johannes Schindelin
` (8 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Reported by, you guessed it, Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/fast-export.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e0220630d00..64617ad8e36 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -734,6 +734,7 @@ static void handle_tag(const char *name, struct tag *tag)
oid_to_hex(&tag->object.oid));
case DROP:
/* Ignore this tag altogether */
+ free(buf);
return;
case REWRITE:
if (tagged->type != OBJ_COMMIT) {
@@ -765,6 +766,7 @@ static void handle_tag(const char *name, struct tag *tag)
(int)(tagger_end - tagger), tagger,
tagger == tagger_end ? "" : "\n",
(int)message_size, (int)message_size, message ? message : "");
+ free(buf);
}
static struct commit *get_commit(struct rev_cmdline_entry *e, char *full_name)
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 18/25] receive-pack: plug memory leak in update()
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (16 preceding siblings ...)
2017-04-28 14:03 ` [PATCH v2 17/25] fast-export: avoid leaking memory in handle_tag() Johannes Schindelin
@ 2017-04-28 14:03 ` Johannes Schindelin
2017-04-28 14:04 ` [PATCH v2 19/25] line-log: avoid memory leak Johannes Schindelin
` (7 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/receive-pack.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f96834f42c9..48c07ddb659 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -986,7 +986,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
{
const char *name = cmd->ref_name;
struct strbuf namespaced_name_buf = STRBUF_INIT;
- const char *namespaced_name, *ret;
+ static char *namespaced_name;
+ const char *ret;
struct object_id *old_oid = &cmd->old_oid;
struct object_id *new_oid = &cmd->new_oid;
@@ -997,6 +998,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
}
strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
+ free(namespaced_name);
namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
if (is_ref_checked_out(namespaced_name)) {
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 19/25] line-log: avoid memory leak
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (17 preceding siblings ...)
2017-04-28 14:03 ` [PATCH v2 18/25] receive-pack: plug memory leak in update() Johannes Schindelin
@ 2017-04-28 14:04 ` Johannes Schindelin
2017-04-28 14:04 ` [PATCH v2 20/25] shallow: " Johannes Schindelin
` (6 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Discovered by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
line-log.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/line-log.c b/line-log.c
index a23b910471b..b9087814b8c 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1125,6 +1125,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c
changed = process_all_files(&parent_range, rev, &queue, range);
if (parent)
add_line_range(rev, parent, parent_range);
+ free_line_log_data(parent_range);
return changed;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 20/25] shallow: avoid memory leak
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (18 preceding siblings ...)
2017-04-28 14:04 ` [PATCH v2 19/25] line-log: avoid memory leak Johannes Schindelin
@ 2017-04-28 14:04 ` Johannes Schindelin
2017-04-28 14:04 ` [PATCH v2 21/25] add_reflog_for_walk: " Johannes Schindelin
` (5 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Reported by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
shallow.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/shallow.c b/shallow.c
index 25b6db989bf..f9370961f99 100644
--- a/shallow.c
+++ b/shallow.c
@@ -473,11 +473,15 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
struct commit_list *head = NULL;
int bitmap_nr = (info->nr_bits + 31) / 32;
size_t bitmap_size = st_mult(sizeof(uint32_t), bitmap_nr);
- uint32_t *tmp = xmalloc(bitmap_size); /* to be freed before return */
- uint32_t *bitmap = paint_alloc(info);
struct commit *c = lookup_commit_reference_gently(sha1, 1);
+ uint32_t *tmp; /* to be freed before return */
+ uint32_t *bitmap;
+
if (!c)
return;
+
+ tmp = xmalloc(bitmap_size);
+ bitmap = paint_alloc(info);
memset(bitmap, 0, bitmap_size);
bitmap[id / 32] |= (1U << (id % 32));
commit_list_insert(c, &head);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 21/25] add_reflog_for_walk: avoid memory leak
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (19 preceding siblings ...)
2017-04-28 14:04 ` [PATCH v2 20/25] shallow: " Johannes Schindelin
@ 2017-04-28 14:04 ` Johannes Schindelin
2017-04-28 14:04 ` [PATCH v2 22/25] remote: plug memory leak in match_explicit() Johannes Schindelin
` (4 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
We free()d the `log` buffer when dwim_log() returned 1, but not when it
returned a larger value (which meant that it still allocated the buffer
but we simply ignored it).
While in the vicinity, make sure that the `reflogs` structure as well as
the `branch` variable are released properly, too.
Identified by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
reflog-walk.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/reflog-walk.c b/reflog-walk.c
index 99679f58255..c63eb1a3fd7 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -183,7 +183,11 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
if (!reflogs || reflogs->nr == 0) {
struct object_id oid;
char *b;
- if (dwim_log(branch, strlen(branch), oid.hash, &b) == 1) {
+ int ret = dwim_log(branch, strlen(branch),
+ oid.hash, &b);
+ if (ret > 1)
+ free(b);
+ else if (ret == 1) {
if (reflogs) {
free(reflogs->ref);
free(reflogs);
@@ -193,17 +197,27 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
reflogs = read_complete_reflog(branch);
}
}
- if (!reflogs || reflogs->nr == 0)
+ if (!reflogs || reflogs->nr == 0) {
+ if (reflogs) {
+ free(reflogs->ref);
+ free(reflogs);
+ }
+ free(branch);
return -1;
+ }
string_list_insert(&info->complete_reflogs, branch)->util
= reflogs;
}
+ free(branch);
commit_reflog = xcalloc(1, sizeof(struct commit_reflog));
if (recno < 0) {
commit_reflog->recno = get_reflog_recno_by_time(reflogs, timestamp);
if (commit_reflog->recno < 0) {
- free(branch);
+ if (reflogs) {
+ free(reflogs->ref);
+ free(reflogs);
+ }
free(commit_reflog);
return -1;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 22/25] remote: plug memory leak in match_explicit()
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (20 preceding siblings ...)
2017-04-28 14:04 ` [PATCH v2 21/25] add_reflog_for_walk: " Johannes Schindelin
@ 2017-04-28 14:04 ` Johannes Schindelin
2017-04-28 14:04 ` [PATCH v2 23/25] name-rev: avoid leaking memory in the `deref` case Johannes Schindelin
` (3 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
The `guess_ref()` returns an allocated buffer of which `make_linked_ref()`
does not take custody (`alloc_ref()` makes a copy), therefore we need to
release the buffer afterwards.
Noticed via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
remote.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/remote.c b/remote.c
index 801137c72eb..72b4591b983 100644
--- a/remote.c
+++ b/remote.c
@@ -1191,9 +1191,10 @@ static int match_explicit(struct ref *src, struct ref *dst,
else if (is_null_oid(&matched_src->new_oid))
error("unable to delete '%s': remote ref does not exist",
dst_value);
- else if ((dst_guess = guess_ref(dst_value, matched_src)))
+ else if ((dst_guess = guess_ref(dst_value, matched_src))) {
matched_dst = make_linked_ref(dst_guess, dst_tail);
- else
+ free(dst_guess);
+ } else
error("unable to push to unqualified destination: %s\n"
"The destination refspec neither matches an "
"existing ref on the remote nor\n"
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v2 23/25] name-rev: avoid leaking memory in the `deref` case
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (21 preceding siblings ...)
2017-04-28 14:04 ` [PATCH v2 22/25] remote: plug memory leak in match_explicit() Johannes Schindelin
@ 2017-04-28 14:04 ` Johannes Schindelin
2017-05-02 3:26 ` Junio C Hamano
2017-04-28 14:04 ` [PATCH v2 24/25] show_worktree(): plug memory leak Johannes Schindelin
` (2 subsequent siblings)
25 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
When the `name_rev()` function is asked to dereference the tip name, it
allocates memory. But when it turns out that another tip already
described the commit better than the current one, we forgot to release
the memory.
Pointed out by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/name-rev.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 92a5d8a5d26..a4ce73fb1e9 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -28,6 +28,7 @@ static void name_rev(struct commit *commit,
struct rev_name *name = (struct rev_name *)commit->util;
struct commit_list *parents;
int parent_number = 1;
+ char *p = NULL;
parse_commit(commit);
@@ -35,7 +36,7 @@ static void name_rev(struct commit *commit,
return;
if (deref) {
- tip_name = xstrfmt("%s^0", tip_name);
+ tip_name = p = xstrfmt("%s^0", tip_name);
if (generation)
die("generation: %d, but deref?", generation);
@@ -53,8 +54,10 @@ static void name_rev(struct commit *commit,
name->taggerdate = taggerdate;
name->generation = generation;
name->distance = distance;
- } else
+ } else {
+ free(p);
return;
+ }
for (parents = commit->parents;
parents;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* Re: [PATCH v2 23/25] name-rev: avoid leaking memory in the `deref` case
2017-04-28 14:04 ` [PATCH v2 23/25] name-rev: avoid leaking memory in the `deref` case Johannes Schindelin
@ 2017-05-02 3:26 ` Junio C Hamano
2017-05-02 3:42 ` Junio C Hamano
0 siblings, 1 reply; 178+ messages in thread
From: Junio C Hamano @ 2017-05-02 3:26 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Stefan Beller, Johannes Sixt, Jeff King
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> When the `name_rev()` function is asked to dereference the tip name, it
> allocates memory. But when it turns out that another tip already
> described the commit better than the current one, we forgot to release
> the memory.
Very well explained.
>
> Pointed out by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/name-rev.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index 92a5d8a5d26..a4ce73fb1e9 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -28,6 +28,7 @@ static void name_rev(struct commit *commit,
> struct rev_name *name = (struct rev_name *)commit->util;
> struct commit_list *parents;
> int parent_number = 1;
> + char *p = NULL;
>
> parse_commit(commit);
>
> @@ -35,7 +36,7 @@ static void name_rev(struct commit *commit,
> return;
>
> if (deref) {
> - tip_name = xstrfmt("%s^0", tip_name);
> + tip_name = p = xstrfmt("%s^0", tip_name);
>
> if (generation)
> die("generation: %d, but deref?", generation);
> @@ -53,8 +54,10 @@ static void name_rev(struct commit *commit,
> name->taggerdate = taggerdate;
> name->generation = generation;
> name->distance = distance;
> - } else
> + } else {
> + free(p);
> return;
> + }
>
> for (parents = commit->parents;
> parents;
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v2 23/25] name-rev: avoid leaking memory in the `deref` case
2017-05-02 3:26 ` Junio C Hamano
@ 2017-05-02 3:42 ` Junio C Hamano
2017-05-02 14:00 ` Johannes Schindelin
0 siblings, 1 reply; 178+ messages in thread
From: Junio C Hamano @ 2017-05-02 3:42 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Stefan Beller, Johannes Sixt, Jeff King
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> When the `name_rev()` function is asked to dereference the tip name, it
>> allocates memory. But when it turns out that another tip already
>> described the commit better than the current one, we forgot to release
>> the memory.
>
> Very well explained.
>>
>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> index 92a5d8a5d26..a4ce73fb1e9 100644
>> --- a/builtin/name-rev.c
>> +++ b/builtin/name-rev.c
>> @@ -28,6 +28,7 @@ static void name_rev(struct commit *commit,
>> struct rev_name *name = (struct rev_name *)commit->util;
>> struct commit_list *parents;
>> int parent_number = 1;
>> + char *p = NULL;
>>
>> parse_commit(commit);
>>
>> @@ -35,7 +36,7 @@ static void name_rev(struct commit *commit,
>> return;
>>
>> if (deref) {
>> - tip_name = xstrfmt("%s^0", tip_name);
>> + tip_name = p = xstrfmt("%s^0", tip_name);
I'll rename 'p' to 'to_free' while queuing, though. Without a
descriptive name, it was confusing to view while resolving conflicts
with another in-flight topic.
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v2 23/25] name-rev: avoid leaking memory in the `deref` case
2017-05-02 3:42 ` Junio C Hamano
@ 2017-05-02 14:00 ` Johannes Schindelin
2017-05-04 4:22 ` Junio C Hamano
0 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 14:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Stefan Beller, Johannes Sixt, Jeff King
Hi Junio,
On Mon, 1 May 2017, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >
> >> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> >> index 92a5d8a5d26..a4ce73fb1e9 100644
> >> --- a/builtin/name-rev.c
> >> +++ b/builtin/name-rev.c
> >> @@ -28,6 +28,7 @@ static void name_rev(struct commit *commit,
> >> struct rev_name *name = (struct rev_name *)commit->util;
> >> struct commit_list *parents;
> >> int parent_number = 1;
> >> + char *p = NULL;
> >>
> >> parse_commit(commit);
> >>
> >> @@ -35,7 +36,7 @@ static void name_rev(struct commit *commit,
> >> return;
> >>
> >> if (deref) {
> >> - tip_name = xstrfmt("%s^0", tip_name);
> >> + tip_name = p = xstrfmt("%s^0", tip_name);
>
> I'll rename 'p' to 'to_free' while queuing, though. Without a
> descriptive name, it was confusing to view while resolving conflicts
> with another in-flight topic.
Good point. I also used `p` in builtin/mktree.c and setup.c. While you
seem to have renamed it in builtin/mktree.c, I think setup.c also needs
the same fixup.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v2 23/25] name-rev: avoid leaking memory in the `deref` case
2017-05-02 14:00 ` Johannes Schindelin
@ 2017-05-04 4:22 ` Junio C Hamano
0 siblings, 0 replies; 178+ messages in thread
From: Junio C Hamano @ 2017-05-04 4:22 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Stefan Beller, Johannes Sixt, Jeff King
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Junio,
>
> On Mon, 1 May 2017, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> >
>> >> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> >> index 92a5d8a5d26..a4ce73fb1e9 100644
>> >> --- a/builtin/name-rev.c
>> >> +++ b/builtin/name-rev.c
>> >> @@ -28,6 +28,7 @@ static void name_rev(struct commit *commit,
>> >> struct rev_name *name = (struct rev_name *)commit->util;
>> >> struct commit_list *parents;
>> >> int parent_number = 1;
>> >> + char *p = NULL;
>> >>
>> >> parse_commit(commit);
>> >>
>> >> @@ -35,7 +36,7 @@ static void name_rev(struct commit *commit,
>> >> return;
>> >>
>> >> if (deref) {
>> >> - tip_name = xstrfmt("%s^0", tip_name);
>> >> + tip_name = p = xstrfmt("%s^0", tip_name);
>>
>> I'll rename 'p' to 'to_free' while queuing, though. Without a
>> descriptive name, it was confusing to view while resolving conflicts
>> with another in-flight topic.
>
> Good point. I also used `p` in builtin/mktree.c and setup.c. While you
> seem to have renamed it in builtin/mktree.c, I think setup.c also needs
> the same fixup.
Yes, no question about it. I just left it as-is as I saw it is
likely to be rerolled anyway due to other issues ;-)
^ permalink raw reply [flat|nested] 178+ messages in thread
* [PATCH v2 24/25] show_worktree(): plug memory leak
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (22 preceding siblings ...)
2017-04-28 14:04 ` [PATCH v2 23/25] name-rev: avoid leaking memory in the `deref` case Johannes Schindelin
@ 2017-04-28 14:04 ` Johannes Schindelin
2017-05-02 3:22 ` Junio C Hamano
2017-04-28 14:04 ` [PATCH v2 25/25] submodule_uses_worktrees(): " Johannes Schindelin
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
25 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
The buffer allocated by shorten_unambiguous_ref() needs to be released.
Discovered by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/worktree.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 1722a9bdc2a..ff5dfd2b102 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -414,9 +414,11 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
if (wt->is_detached)
strbuf_addstr(&sb, "(detached HEAD)");
- else if (wt->head_ref)
- strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
- else
+ else if (wt->head_ref) {
+ char *ref = shorten_unambiguous_ref(wt->head_ref, 0);
+ strbuf_addf(&sb, "[%s]", ref);
+ free(ref);
+ } else
strbuf_addstr(&sb, "(error)");
}
printf("%s\n", sb.buf);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* Re: [PATCH v2 24/25] show_worktree(): plug memory leak
2017-04-28 14:04 ` [PATCH v2 24/25] show_worktree(): plug memory leak Johannes Schindelin
@ 2017-05-02 3:22 ` Junio C Hamano
0 siblings, 0 replies; 178+ messages in thread
From: Junio C Hamano @ 2017-05-02 3:22 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Stefan Beller, Johannes Sixt, Jeff King
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The buffer allocated by shorten_unambiguous_ref() needs to be released.
Yes. Looks good.
>
> Discovered by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/worktree.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 1722a9bdc2a..ff5dfd2b102 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -414,9 +414,11 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
> find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
> if (wt->is_detached)
> strbuf_addstr(&sb, "(detached HEAD)");
> - else if (wt->head_ref)
> - strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
> - else
> + else if (wt->head_ref) {
> + char *ref = shorten_unambiguous_ref(wt->head_ref, 0);
> + strbuf_addf(&sb, "[%s]", ref);
> + free(ref);
> + } else
> strbuf_addstr(&sb, "(error)");
> }
> printf("%s\n", sb.buf);
^ permalink raw reply [flat|nested] 178+ messages in thread
* [PATCH v2 25/25] submodule_uses_worktrees(): plug memory leak
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (23 preceding siblings ...)
2017-04-28 14:04 ` [PATCH v2 24/25] show_worktree(): plug memory leak Johannes Schindelin
@ 2017-04-28 14:04 ` Johannes Schindelin
2017-05-02 3:17 ` Junio C Hamano
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
25 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-04-28 14:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
There is really no reason why we would need to hold onto the allocated
string longer than necessary.
Reported by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
worktree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/worktree.c b/worktree.c
index bae787cf8d7..89a81b13de3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -399,6 +399,7 @@ int submodule_uses_worktrees(const char *path)
/* The env would be set for the superproject. */
get_common_dir_noenv(&sb, submodule_gitdir);
+ free(submodule_gitdir);
/*
* The check below is only known to be good for repository format
@@ -418,7 +419,6 @@ int submodule_uses_worktrees(const char *path)
/* See if there is any file inside the worktrees directory. */
dir = opendir(sb.buf);
strbuf_release(&sb);
- free(submodule_gitdir);
if (!dir)
return 0;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* Re: [PATCH v2 25/25] submodule_uses_worktrees(): plug memory leak
2017-04-28 14:04 ` [PATCH v2 25/25] submodule_uses_worktrees(): " Johannes Schindelin
@ 2017-05-02 3:17 ` Junio C Hamano
0 siblings, 0 replies; 178+ messages in thread
From: Junio C Hamano @ 2017-05-02 3:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Stefan Beller, Johannes Sixt, Jeff King
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> There is really no reason why we would need to hold onto the allocated
> string longer than necessary.
Yup. The longer we make the duration between the allocation and the
standard release, the more likely future code would add early returns
that forget to release the memory.
Looks correct; will queue.
> Reported by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> worktree.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/worktree.c b/worktree.c
> index bae787cf8d7..89a81b13de3 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -399,6 +399,7 @@ int submodule_uses_worktrees(const char *path)
>
> /* The env would be set for the superproject. */
> get_common_dir_noenv(&sb, submodule_gitdir);
> + free(submodule_gitdir);
>
> /*
> * The check below is only known to be good for repository format
> @@ -418,7 +419,6 @@ int submodule_uses_worktrees(const char *path)
> /* See if there is any file inside the worktrees directory. */
> dir = opendir(sb.buf);
> strbuf_release(&sb);
> - free(submodule_gitdir);
>
> if (!dir)
> return 0;
^ permalink raw reply [flat|nested] 178+ messages in thread
* [PATCH v3 00/25] Address a couple of issues identified by Coverity
2017-04-28 13:49 ` [PATCH v2 00/25] " Johannes Schindelin
` (24 preceding siblings ...)
2017-04-28 14:04 ` [PATCH v2 25/25] submodule_uses_worktrees(): " Johannes Schindelin
@ 2017-05-02 16:00 ` Johannes Schindelin
2017-05-02 16:00 ` [PATCH v3 01/25] mingw: avoid memory leak when splitting PATH Johannes Schindelin
` (25 more replies)
25 siblings, 26 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
[-- Attachment #1: Type: text/plain, Size: 8886 bytes --]
I recently registered the git-for-windows fork with Coverity to ensure
that even the Windows-specific patches get some static analysis love.
While at it, I squashed a couple of obvious issues in the part that is
not Windows-specific.
Note: while this patch series squashes some of those issues, the
remaining issues are not necessarily all false positives (e.g. Coverity
getting fooled by our FLEX_ARRAY trick into believing that the last
member of the struct is indeed a 0 or 1 size array) or intentional (e.g.
builtins not releasing memory because exit() is called right after
returning from the function that leaks memory).
Notable examples of the remaining issues are:
- a couple of callers of shorten_unambiguous_ref() assume that they do
not have to release the memory returned from that function, often
assigning the pointer to a `const` variable that typically does not
hold a pointer that needs to be free()d. My hunch is that we will want
to convert that function to have a fixed number of static buffers and
use those in a round robin fashion à la sha1_to_hex().
- pack-redundant.c seems to have hard-to-reason-about code paths that
may eventually leak memory. Essentially, the custody of the allocated
memory is not clear at all.
- fast-import.c calls strbuf_detach() on the command_buf without any
obvious rationale. Turns out that *some* code paths assign
command_buf.buf to a `recent_command` which releases the buffer later.
However, from a cursory look it appears to me as if there are some
other code paths that skip that assignment, effectively causing a memory
leak once strbuf_detach() is called.
Sadly, I lack the time to pursue those remaining issues further.
Changes since v2:
- renamed the `p` variables introduced by the patch series to the more
explanatory `to_free`.
- reworded incorrect commit message claiming that
setup_discovered_git_dir() was using a static variable that is
actually a singleton
- reverted a code move that would have resulted in accessing
uninitialized data of callers of mailinfo() that do not die() right
away but clean up faithfully
Johannes Schindelin (25):
mingw: avoid memory leak when splitting PATH
winansi: avoid use of uninitialized value
winansi: avoid buffer overrun
add_commit_patch_id(): avoid allocating memory unnecessarily
git_config_rename_section_in_file(): avoid resource leak
get_mail_commit_oid(): avoid resource leak
difftool: address a couple of resource/memory leaks
status: close file descriptor after reading git-rebase-todo
mailinfo & mailsplit: check for EOF while parsing
cat-file: fix memory leak
checkout: fix memory leak
split_commit_in_progress(): fix memory leak
setup_bare_git_dir(): help static analysis
setup_discovered_git_dir(): plug memory leak
pack-redundant: plug memory leak
mktree: plug memory leaks reported by Coverity
fast-export: avoid leaking memory in handle_tag()
receive-pack: plug memory leak in update()
line-log: avoid memory leak
shallow: avoid memory leak
add_reflog_for_walk: avoid memory leak
remote: plug memory leak in match_explicit()
name-rev: avoid leaking memory in the `deref` case
show_worktree(): plug memory leak
submodule_uses_worktrees(): plug memory leak
builtin/am.c | 15 ++++++---------
builtin/cat-file.c | 1 +
builtin/checkout.c | 17 +++++++++--------
builtin/difftool.c | 33 +++++++++++++++++++++++----------
builtin/fast-export.c | 2 ++
builtin/mailsplit.c | 10 ++++++++++
builtin/mktree.c | 5 +++--
builtin/name-rev.c | 7 +++++--
builtin/pack-redundant.c | 1 +
builtin/receive-pack.c | 4 +++-
builtin/worktree.c | 8 +++++---
compat/mingw.c | 4 +++-
compat/winansi.c | 7 +++++++
config.c | 5 ++++-
line-log.c | 1 +
mailinfo.c | 9 ++++++++-
patch-ids.c | 3 ++-
reflog-walk.c | 20 +++++++++++++++++---
remote.c | 5 +++--
setup.c | 11 ++++++++---
shallow.c | 8 ++++++--
worktree.c | 2 +-
wt-status.c | 8 +++++++-
23 files changed, 135 insertions(+), 51 deletions(-)
base-commit: d2bbb7c2bcf6e77ebfcabf4e12110fe6d5c91de6
Published-As: https://github.com/dscho/git/releases/tag/coverity-v3
Fetch-It-Via: git fetch https://github.com/dscho/git coverity-v3
Interdiff vs v2:
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 9b3efc8e987..664400b8169 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -232,9 +232,18 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
do {
peek = fgetc(f);
+ if (peek == EOF) {
+ if (f == stdin)
+ /* empty stdin is OK */
+ ret = skip;
+ else {
+ fclose(f);
+ error(_("empty mbox: '%s'"), file);
+ }
+ goto out;
+ }
} while (isspace(peek));
- if (peek != EOF)
- ungetc(peek, f);
+ ungetc(peek, f);
if (strbuf_getwholeline(&buf, f, '\n')) {
/* empty stdin is OK */
diff --git a/builtin/mktree.c b/builtin/mktree.c
index f0354bc9718..da0fd8cd706 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -72,7 +72,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
unsigned mode;
enum object_type mode_type; /* object type derived from mode */
enum object_type obj_type; /* object type derived from sha */
- char *path, *p = NULL;
+ char *path, *to_free = NULL;
unsigned char sha1[20];
ptr = buf;
@@ -102,7 +102,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
struct strbuf p_uq = STRBUF_INIT;
if (unquote_c_style(&p_uq, path, NULL))
die("invalid quoting");
- path = p = strbuf_detach(&p_uq, NULL);
+ path = to_free = strbuf_detach(&p_uq, NULL);
}
/*
@@ -136,7 +136,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
}
append_to_tree(mode, sha1, path);
- free(p);
+ free(to_free);
}
int cmd_mktree(int ac, const char **av, const char *prefix)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index a4ce73fb1e9..e7a3fe7ee70 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -28,7 +28,7 @@ static void name_rev(struct commit *commit,
struct rev_name *name = (struct rev_name *)commit->util;
struct commit_list *parents;
int parent_number = 1;
- char *p = NULL;
+ char *to_free = NULL;
parse_commit(commit);
@@ -36,7 +36,7 @@ static void name_rev(struct commit *commit,
return;
if (deref) {
- tip_name = p = xstrfmt("%s^0", tip_name);
+ tip_name = to_free = xstrfmt("%s^0", tip_name);
if (generation)
die("generation: %d, but deref?", generation);
@@ -55,7 +55,7 @@ static void name_rev(struct commit *commit,
name->generation = generation;
name->distance = distance;
} else {
- free(p);
+ free(to_free);
return;
}
diff --git a/mailinfo.c b/mailinfo.c
index a319911b510..f92cb9f729c 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -1097,6 +1097,9 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch)
return -1;
}
+ mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
+ mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
+
do {
peek = fgetc(mi->input);
if (peek == EOF) {
@@ -1106,9 +1109,6 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch)
} while (isspace(peek));
ungetc(peek, mi->input);
- mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
- mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
-
/* process the email header */
while (read_one_header_line(&line, mi->input))
check_header(mi, &line, mi->p_hdr_data, 1);
diff --git a/setup.c b/setup.c
index 12efca85a41..e3f7699a902 100644
--- a/setup.c
+++ b/setup.c
@@ -703,15 +703,15 @@ static const char *setup_discovered_git_dir(const char *gitdir,
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
- char *p = NULL;
+ char *to_free = NULL;
const char *ret;
if (offset != cwd->len && !is_absolute_path(gitdir))
- gitdir = p = real_pathdup(gitdir, 1);
+ gitdir = to_free = real_pathdup(gitdir, 1);
if (chdir(cwd->buf))
die_errno("Could not come back to cwd");
ret = setup_explicit_git_dir(gitdir, cwd, nongit_ok);
- free(p);
+ free(to_free);
return ret;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply [flat|nested] 178+ messages in thread
* [PATCH v3 01/25] mingw: avoid memory leak when splitting PATH
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
@ 2017-05-02 16:00 ` Johannes Schindelin
2017-05-03 19:48 ` René Scharfe
2017-05-02 16:01 ` [PATCH v3 02/25] winansi: avoid use of uninitialized value Johannes Schindelin
` (24 subsequent siblings)
25 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
In the (admittedly, concocted) case that PATH consists only of colons, we
would leak the duplicated string.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/mingw.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 3fbfda5978b..fe0e3ccd248 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -961,8 +961,10 @@ static char **get_path_split(void)
++n;
}
}
- if (!n)
+ if (!n) {
+ free(envpath);
return NULL;
+ }
ALLOC_ARRAY(path, n + 1);
p = envpath;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* Re: [PATCH v3 01/25] mingw: avoid memory leak when splitting PATH
2017-05-02 16:00 ` [PATCH v3 01/25] mingw: avoid memory leak when splitting PATH Johannes Schindelin
@ 2017-05-03 19:48 ` René Scharfe
2017-05-04 10:29 ` Johannes Schindelin
0 siblings, 1 reply; 178+ messages in thread
From: René Scharfe @ 2017-05-03 19:48 UTC (permalink / raw)
To: Johannes Schindelin, git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Am 02.05.2017 um 18:00 schrieb Johannes Schindelin:
> In the (admittedly, concocted) case that PATH consists only of colons, we
> would leak the duplicated string.
Nit: It's about semicolons, right? At least that's what get_path_split
is searching for. Or is there some kind of translation going on?
René
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v3 01/25] mingw: avoid memory leak when splitting PATH
2017-05-03 19:48 ` René Scharfe
@ 2017-05-04 10:29 ` Johannes Schindelin
0 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 10:29 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
[-- Attachment #1: Type: text/plain, Size: 613 bytes --]
Hi René,
On Wed, 3 May 2017, René Scharfe wrote:
> Am 02.05.2017 um 18:00 schrieb Johannes Schindelin:
> > In the (admittedly, concocted) case that PATH consists only of colons, we
> > would leak the duplicated string.
>
> Nit: It's about semicolons, right? At least that's what get_path_split
> is searching for. Or is there some kind of translation going on?
True, it is semicolons.
I'll reword the commit message to actually use `path delimiters`, to make
it more understandable to the Unix folk (who would most likely be puzzled
that PATH is separated by semicolons).
Thanks,
Dscho
^ permalink raw reply [flat|nested] 178+ messages in thread
* [PATCH v3 02/25] winansi: avoid use of uninitialized value
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
2017-05-02 16:00 ` [PATCH v3 01/25] mingw: avoid memory leak when splitting PATH Johannes Schindelin
@ 2017-05-02 16:01 ` Johannes Schindelin
2017-05-03 19:48 ` René Scharfe
2017-05-02 16:01 ` [PATCH v3 03/25] winansi: avoid buffer overrun Johannes Schindelin
` (23 subsequent siblings)
25 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
When stdout is not connected to a Win32 console, we incorrectly used an
uninitialized value for the "plain" character attributes.
Detected by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/winansi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/compat/winansi.c b/compat/winansi.c
index 793420f9d0d..fd6910746c8 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -105,6 +105,8 @@ static int is_console(int fd)
if (!fd) {
if (!GetConsoleMode(hcon, &mode))
return 0;
+ sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN |
+ FOREGROUND_RED;
} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
return 0;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* Re: [PATCH v3 02/25] winansi: avoid use of uninitialized value
2017-05-02 16:01 ` [PATCH v3 02/25] winansi: avoid use of uninitialized value Johannes Schindelin
@ 2017-05-03 19:48 ` René Scharfe
2017-05-04 10:23 ` Johannes Schindelin
0 siblings, 1 reply; 178+ messages in thread
From: René Scharfe @ 2017-05-03 19:48 UTC (permalink / raw)
To: Johannes Schindelin, git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Am 02.05.2017 um 18:01 schrieb Johannes Schindelin:
> When stdout is not connected to a Win32 console, we incorrectly used an
> uninitialized value for the "plain" character attributes.
>
> Detected by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> compat/winansi.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/compat/winansi.c b/compat/winansi.c
> index 793420f9d0d..fd6910746c8 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -105,6 +105,8 @@ static int is_console(int fd)
> if (!fd) {
> if (!GetConsoleMode(hcon, &mode))
> return 0;
> + sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN |
> + FOREGROUND_RED;
> } else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
> return 0;
>
So is_console is called with fd being 1 (stdout), 2 (stderr) and 0
(stdin), in that order. If the first two calls abort early for some
reason we may end up here. The added code is for white text on black
background. An alias for that combination, FOREGROUND_ALL, is defined
a few lines down; for a minimal fix it's not worth moving it up. attr
and plain_attr are both initialized to sbi.wAttributes.
That as a bit more complicated than it looked initially. The order of
calls is important, "stdout" in the commit message includes stderr as
well and it doesn't just affect plain_attr.
Would a value of 0 (black text on black background) suffice if only
stdin is connected to a console? Colors don't matter if there is
nothing to see, right?
Anyway, the change makes sense as is, but it took me a while to get it.
René
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v3 02/25] winansi: avoid use of uninitialized value
2017-05-03 19:48 ` René Scharfe
@ 2017-05-04 10:23 ` Johannes Schindelin
0 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 10:23 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
[-- Attachment #1: Type: text/plain, Size: 4181 bytes --]
Hi René,
On Wed, 3 May 2017, René Scharfe wrote:
> Am 02.05.2017 um 18:01 schrieb Johannes Schindelin:
> >
> > diff --git a/compat/winansi.c b/compat/winansi.c
> > index 793420f9d0d..fd6910746c8 100644
> > --- a/compat/winansi.c
> > +++ b/compat/winansi.c
> > @@ -105,6 +105,8 @@ static int is_console(int fd)
> > if (!fd) {
> > if (!GetConsoleMode(hcon, &mode))
> > return 0;
> > + sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN |
> > + FOREGROUND_RED;
> > } else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
> > return 0;
> >
>
> So is_console is called with fd being 1 (stdout), 2 (stderr) and 0
> (stdin), in that order.
Correct. I guess this is the important part missing from the commit
message.
Oh, I also saw that I talked about stdout in the commit message, but !fd
tests for stdin!
> If the first two calls abort early for some reason we may end up here.
Yep. "some reason" being: there is no console attached to stdout nor
stderr.
> The added code is for white text on black background. An alias for that
> combination, FOREGROUND_ALL, is defined a few lines down; for a minimal
> fix it's not worth moving it up. attr and plain_attr are both
> initialized to sbi.wAttributes.
Exactly.
> That as a bit more complicated than it looked initially. The order of
> calls is important, "stdout" in the commit message includes stderr as
> well and it doesn't just affect plain_attr.
Right, it also affects the "current" attributes.
> Would a value of 0 (black text on black background) suffice if only
> stdin is connected to a console? Colors don't matter if there is
> nothing to see, right?
I think that would make it both easier to understand the patch and to
catch regressions in case anybody feels the order of the is_console()
calls should be changed...
This is my current squash! commit (the original commit message will be
replaced by the commit message body of this commit):
-- snipsnap --
Subject: [PATCH] squash! winansi: avoid use of uninitialized value
winansi: avoid use of uninitialized value
To initialize the foreground color attributes of "plain text", our ANSI
emulation tries to infer them from the currently attached console while
running the is_console() function. This function first tries to detect any
console attached to stdout, then it is called with stderr.
If neither stdout nor stderr has any console attached, it does not
actually matter what we use for "plain text" attributes, as we never need
to output any text to any console in that case.
However, after working on stdout and stderr, is_console() is called with
stdin, and it still tries to initialize the "plain text" attributes if
they had not been initialized earlier. In this case, we cannot detect any
attributes, and we used an uninitialized value for them.
Naturally, Coverity complained about this use case because it could not
reason about the code deeply enough to figure out that we do not even use
those attributes in that case.
Let's just initialize the value to 0 in that case, both to avoid future
Coverity reports, and to help catch future regressions in case anybody
changes the order of the is_console() calls (which would make the text
black on black).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/winansi.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/compat/winansi.c b/compat/winansi.c
index 861b79d8c31..a11a0f16d27 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -105,8 +105,13 @@ static int is_console(int fd)
if (!fd) {
if (!GetConsoleMode(hcon, &mode))
return 0;
- sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN |
- FOREGROUND_RED;
+ /*
+ * This code path is only reached if there is no console
+ * attached to stdout/stderr, i.e. we will not need to output
+ * any text to any console, therefore we might just as well
+ * use black as foreground color.
+ */
+ sbi.wAttributes = 0;
} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
return 0;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 03/25] winansi: avoid buffer overrun
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
2017-05-02 16:00 ` [PATCH v3 01/25] mingw: avoid memory leak when splitting PATH Johannes Schindelin
2017-05-02 16:01 ` [PATCH v3 02/25] winansi: avoid use of uninitialized value Johannes Schindelin
@ 2017-05-02 16:01 ` Johannes Schindelin
2017-05-02 16:01 ` [PATCH v3 04/25] add_commit_patch_id(): avoid allocating memory unnecessarily Johannes Schindelin
` (22 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
When we could not convert the UTF-8 sequence into Unicode for writing to
the Console, we should not try to write an insanely-long sequence of
invalid wide characters (mistaking the negative return value for an
unsigned length).
Reported by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/winansi.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/compat/winansi.c b/compat/winansi.c
index fd6910746c8..861b79d8c31 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -135,6 +135,11 @@ static void write_console(unsigned char *str, size_t len)
/* convert utf-8 to utf-16 */
int wlen = xutftowcsn(wbuf, (char*) str, ARRAY_SIZE(wbuf), len);
+ if (wlen < 0) {
+ wchar_t *err = L"[invalid]";
+ WriteConsoleW(console, err, wcslen(err), &dummy, NULL);
+ return;
+ }
/* write directly to console */
WriteConsoleW(console, wbuf, wlen, &dummy, NULL);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 04/25] add_commit_patch_id(): avoid allocating memory unnecessarily
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (2 preceding siblings ...)
2017-05-02 16:01 ` [PATCH v3 03/25] winansi: avoid buffer overrun Johannes Schindelin
@ 2017-05-02 16:01 ` Johannes Schindelin
2017-05-02 16:01 ` [PATCH v3 05/25] git_config_rename_section_in_file(): avoid resource leak Johannes Schindelin
` (21 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
It would appear that we allocate (and forget to release) memory if the
patch ID is not even defined.
Reported by the Coverity tool.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
patch-ids.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/patch-ids.c b/patch-ids.c
index fa8f11de826..92eba7a059e 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -99,11 +99,12 @@ struct patch_id *has_commit_patch_id(struct commit *commit,
struct patch_id *add_commit_patch_id(struct commit *commit,
struct patch_ids *ids)
{
- struct patch_id *key = xcalloc(1, sizeof(*key));
+ struct patch_id *key;
if (!patch_id_defined(commit))
return NULL;
+ key = xcalloc(1, sizeof(*key));
if (init_patch_id_entry(key, commit, ids)) {
free(key);
return NULL;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 05/25] git_config_rename_section_in_file(): avoid resource leak
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (3 preceding siblings ...)
2017-05-02 16:01 ` [PATCH v3 04/25] add_commit_patch_id(): avoid allocating memory unnecessarily Johannes Schindelin
@ 2017-05-02 16:01 ` Johannes Schindelin
2017-05-02 16:01 ` [PATCH v3 06/25] get_mail_commit_oid(): " Johannes Schindelin
` (20 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
In case of errors, we really want the file descriptor to be closed.
Discovered by a Coverity scan.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index b4a3205da32..a30056ec7e9 100644
--- a/config.c
+++ b/config.c
@@ -2621,7 +2621,7 @@ int git_config_rename_section_in_file(const char *config_filename,
struct lock_file *lock;
int out_fd;
char buf[1024];
- FILE *config_file;
+ FILE *config_file = NULL;
struct stat st;
if (new_name && !section_name_is_ok(new_name)) {
@@ -2703,11 +2703,14 @@ int git_config_rename_section_in_file(const char *config_filename,
}
}
fclose(config_file);
+ config_file = NULL;
commit_and_out:
if (commit_lock_file(lock) < 0)
ret = error_errno("could not write config file %s",
config_filename);
out:
+ if (config_file)
+ fclose(config_file);
rollback_lock_file(lock);
out_no_rollback:
free(filename_buf);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 06/25] get_mail_commit_oid(): avoid resource leak
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (4 preceding siblings ...)
2017-05-02 16:01 ` [PATCH v3 05/25] git_config_rename_section_in_file(): avoid resource leak Johannes Schindelin
@ 2017-05-02 16:01 ` Johannes Schindelin
2017-05-02 16:01 ` [PATCH v3 07/25] difftool: address a couple of resource/memory leaks Johannes Schindelin
` (19 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
When we fail to read, or parse, the file, we still want to close the file
descriptor and release the strbuf.
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/am.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index a95dd8b4e6c..9c5c2c778e2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1351,19 +1351,16 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail)
struct strbuf sb = STRBUF_INIT;
FILE *fp = xfopen(mail, "r");
const char *x;
+ int ret = 0;
- if (strbuf_getline_lf(&sb, fp))
- return -1;
-
- if (!skip_prefix(sb.buf, "From ", &x))
- return -1;
-
- if (get_oid_hex(x, commit_id) < 0)
- return -1;
+ if (strbuf_getline_lf(&sb, fp) ||
+ !skip_prefix(sb.buf, "From ", &x) ||
+ get_oid_hex(x, commit_id) < 0)
+ ret = -1;
strbuf_release(&sb);
fclose(fp);
- return 0;
+ return ret;
}
/**
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 07/25] difftool: address a couple of resource/memory leaks
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (5 preceding siblings ...)
2017-05-02 16:01 ` [PATCH v3 06/25] get_mail_commit_oid(): " Johannes Schindelin
@ 2017-05-02 16:01 ` Johannes Schindelin
2017-05-02 16:01 ` [PATCH v3 08/25] status: close file descriptor after reading git-rebase-todo Johannes Schindelin
` (18 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
This change plugs a couple of memory leaks and makes sure that the file
descriptor is closed in run_dir_diff().
Spotted by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/difftool.c | 33 +++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 1354d0e4625..b9a892f2693 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -226,6 +226,7 @@ static void changed_files(struct hashmap *result, const char *index_path,
hashmap_entry_init(entry, strhash(buf.buf));
hashmap_add(result, entry);
}
+ fclose(fp);
if (finish_command(&diff_files))
die("diff-files did not exit properly");
strbuf_release(&index_env);
@@ -439,8 +440,10 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
}
if (lmode && status != 'C') {
- if (checkout_path(lmode, &loid, src_path, &lstate))
- return error("could not write '%s'", src_path);
+ if (checkout_path(lmode, &loid, src_path, &lstate)) {
+ ret = error("could not write '%s'", src_path);
+ goto finish;
+ }
}
if (rmode && !S_ISLNK(rmode)) {
@@ -456,9 +459,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
hashmap_add(&working_tree_dups, entry);
if (!use_wt_file(workdir, dst_path, &roid)) {
- if (checkout_path(rmode, &roid, dst_path, &rstate))
- return error("could not write '%s'",
- dst_path);
+ if (checkout_path(rmode, &roid, dst_path,
+ &rstate)) {
+ ret = error("could not write '%s'",
+ dst_path);
+ goto finish;
+ }
} else if (!is_null_oid(&roid)) {
/*
* Changes in the working tree need special
@@ -473,10 +479,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
ADD_CACHE_JUST_APPEND);
add_path(&rdir, rdir_len, dst_path);
- if (ensure_leading_directories(rdir.buf))
- return error("could not create "
- "directory for '%s'",
- dst_path);
+ if (ensure_leading_directories(rdir.buf)) {
+ ret = error("could not create "
+ "directory for '%s'",
+ dst_path);
+ goto finish;
+ }
add_path(&wtdir, wtdir_len, dst_path);
if (symlinks) {
if (symlink(wtdir.buf, rdir.buf)) {
@@ -497,13 +505,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
}
}
+ fclose(fp);
+ fp = NULL;
if (finish_command(&child)) {
ret = error("error occurred running diff --raw");
goto finish;
}
if (!i)
- return 0;
+ goto finish;
/*
* Changes to submodules require special treatment.This loop writes a
@@ -626,6 +636,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
exit_cleanup(tmpdir, rc);
finish:
+ if (fp)
+ fclose(fp);
+
free(lbase_dir);
free(rbase_dir);
strbuf_release(&ldir);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 08/25] status: close file descriptor after reading git-rebase-todo
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (6 preceding siblings ...)
2017-05-02 16:01 ` [PATCH v3 07/25] difftool: address a couple of resource/memory leaks Johannes Schindelin
@ 2017-05-02 16:01 ` Johannes Schindelin
2017-05-02 16:01 ` [PATCH v3 09/25] mailinfo & mailsplit: check for EOF while parsing Johannes Schindelin
` (17 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
wt-status.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/wt-status.c b/wt-status.c
index 03754849626..0a6e16dbe0f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1168,6 +1168,7 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines)
abbrev_sha1_in_line(&line);
string_list_append(lines, line.buf);
}
+ fclose(f);
return 0;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 09/25] mailinfo & mailsplit: check for EOF while parsing
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (7 preceding siblings ...)
2017-05-02 16:01 ` [PATCH v3 08/25] status: close file descriptor after reading git-rebase-todo Johannes Schindelin
@ 2017-05-02 16:01 ` Johannes Schindelin
2017-05-02 16:01 ` [PATCH v3 10/25] cat-file: fix memory leak Johannes Schindelin
` (16 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
While POSIX states that it is okay to pass EOF to isspace() (and it seems
to be implied that EOF should *not* be treated as whitespace), and also to
pass EOF to ungetc() (which seems to be intended to fail without buffering
the character), it is much better to handle these cases explicitly. Not
only does it reduce head-scratching (and helps static analysis avoid
reporting false positives), it also lets us handle files containing
nothing but whitespace by erroring out.
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/mailsplit.c | 10 ++++++++++
mailinfo.c | 9 ++++++++-
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 30681681c13..664400b8169 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -232,6 +232,16 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
do {
peek = fgetc(f);
+ if (peek == EOF) {
+ if (f == stdin)
+ /* empty stdin is OK */
+ ret = skip;
+ else {
+ fclose(f);
+ error(_("empty mbox: '%s'"), file);
+ }
+ goto out;
+ }
} while (isspace(peek));
ungetc(peek, f);
diff --git a/mailinfo.c b/mailinfo.c
index 68037758f2f..f92cb9f729c 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -882,7 +882,10 @@ static int read_one_header_line(struct strbuf *line, FILE *in)
for (;;) {
int peek;
- peek = fgetc(in); ungetc(peek, in);
+ peek = fgetc(in);
+ if (peek == EOF)
+ break;
+ ungetc(peek, in);
if (peek != ' ' && peek != '\t')
break;
if (strbuf_getline_lf(&continuation, in))
@@ -1099,6 +1102,10 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch)
do {
peek = fgetc(mi->input);
+ if (peek == EOF) {
+ fclose(cmitmsg);
+ return error("empty patch: '%s'", patch);
+ }
} while (isspace(peek));
ungetc(peek, mi->input);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 10/25] cat-file: fix memory leak
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (8 preceding siblings ...)
2017-05-02 16:01 ` [PATCH v3 09/25] mailinfo & mailsplit: check for EOF while parsing Johannes Schindelin
@ 2017-05-02 16:01 ` Johannes Schindelin
2017-05-02 16:02 ` [PATCH v3 11/25] checkout: " Johannes Schindelin
` (15 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Discovered by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/cat-file.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1890d7a6390..9af863e7915 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -165,6 +165,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
die("git cat-file %s: bad file", obj_name);
write_or_die(1, buf, size);
+ free(buf);
return 0;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 11/25] checkout: fix memory leak
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (9 preceding siblings ...)
2017-05-02 16:01 ` [PATCH v3 10/25] cat-file: fix memory leak Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-02 16:02 ` [PATCH v3 12/25] split_commit_in_progress(): " Johannes Schindelin
` (14 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
This change addresses part of the NEEDSWORK comment above the code,
therefore the comment needs to be adjusted, too.
Discovered via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/checkout.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfa5419f335..5faea3a05fa 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -235,14 +235,14 @@ static int checkout_merged(int pos, const struct checkout *state)
/*
* NEEDSWORK:
* There is absolutely no reason to write this as a blob object
- * and create a phony cache entry just to leak. This hack is
- * primarily to get to the write_entry() machinery that massages
- * the contents to work-tree format and writes out which only
- * allows it for a cache entry. The code in write_entry() needs
- * to be refactored to allow us to feed a <buffer, size, mode>
- * instead of a cache entry. Such a refactoring would help
- * merge_recursive as well (it also writes the merge result to the
- * object database even when it may contain conflicts).
+ * and create a phony cache entry. This hack is primarily to get
+ * to the write_entry() machinery that massages the contents to
+ * work-tree format and writes out which only allows it for a
+ * cache entry. The code in write_entry() needs to be refactored
+ * to allow us to feed a <buffer, size, mode> instead of a cache
+ * entry. Such a refactoring would help merge_recursive as well
+ * (it also writes the merge result to the object database even
+ * when it may contain conflicts).
*/
if (write_sha1_file(result_buf.ptr, result_buf.size,
blob_type, oid.hash))
@@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct checkout *state)
if (!ce)
die(_("make_cache_entry failed for path '%s'"), path);
status = checkout_entry(ce, state, NULL);
+ free(ce);
return status;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 12/25] split_commit_in_progress(): fix memory leak
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (10 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 11/25] checkout: " Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-03 20:59 ` René Scharfe
2017-05-02 16:02 ` [PATCH v3 13/25] setup_bare_git_dir(): help static analysis Johannes Schindelin
` (13 subsequent siblings)
25 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
wt-status.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/wt-status.c b/wt-status.c
index 0a6e16dbe0f..1f3f6bcb980 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1088,8 +1088,13 @@ static int split_commit_in_progress(struct wt_status *s)
char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
- !s->branch || strcmp(s->branch, "HEAD"))
+ !s->branch || strcmp(s->branch, "HEAD")) {
+ free(head);
+ free(orig_head);
+ free(rebase_amend);
+ free(rebase_orig_head);
return split_in_progress;
+ }
if (!strcmp(rebase_amend, rebase_orig_head)) {
if (strcmp(head, rebase_amend))
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* Re: [PATCH v3 12/25] split_commit_in_progress(): fix memory leak
2017-05-02 16:02 ` [PATCH v3 12/25] split_commit_in_progress(): " Johannes Schindelin
@ 2017-05-03 20:59 ` René Scharfe
2017-05-04 10:59 ` Johannes Schindelin
0 siblings, 1 reply; 178+ messages in thread
From: René Scharfe @ 2017-05-03 20:59 UTC (permalink / raw)
To: Johannes Schindelin, git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Am 02.05.2017 um 18:02 schrieb Johannes Schindelin:
> Reported via Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> wt-status.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index 0a6e16dbe0f..1f3f6bcb980 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1088,8 +1088,13 @@ static int split_commit_in_progress(struct wt_status *s)
> char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
>
> if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
> - !s->branch || strcmp(s->branch, "HEAD"))
> + !s->branch || strcmp(s->branch, "HEAD")) {
> + free(head);
> + free(orig_head);
> + free(rebase_amend);
> + free(rebase_orig_head);
> return split_in_progress;
> + }
>
> if (!strcmp(rebase_amend, rebase_orig_head)) {
> if (strcmp(head, rebase_amend))
>
The return line could be replaced by "; else" to achieve the same
result, without duplicating the free calls.
René
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v3 12/25] split_commit_in_progress(): fix memory leak
2017-05-03 20:59 ` René Scharfe
@ 2017-05-04 10:59 ` Johannes Schindelin
2017-05-06 17:13 ` René Scharfe
0 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 10:59 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
[-- Attachment #1: Type: text/plain, Size: 6477 bytes --]
Hi René,
On Wed, 3 May 2017, René Scharfe wrote:
> Am 02.05.2017 um 18:02 schrieb Johannes Schindelin:
> > Reported via Coverity.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > wt-status.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/wt-status.c b/wt-status.c
> > index 0a6e16dbe0f..1f3f6bcb980 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -1088,8 +1088,13 @@ static int split_commit_in_progress(struct wt_status
> > *s)
> > char *rebase_orig_head =
> > read_line_from_git_path("rebase-merge/orig-head");
> >
> > if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
> > - !s->branch || strcmp(s->branch, "HEAD"))
> > + !s->branch || strcmp(s->branch, "HEAD")) {
> > + free(head);
> > + free(orig_head);
> > + free(rebase_amend);
> > + free(rebase_orig_head);
> > return split_in_progress;
> > + }
> >
> > if (!strcmp(rebase_amend, rebase_orig_head)) {
> > if (strcmp(head, rebase_amend))
> >
>
> The return line could be replaced by "; else" to achieve the same
> result, without duplicating the free calls.
True. It is much harder to explain it that way, though: the context looks
like this:
static int split_commit_in_progress(struct wt_status *s)
{
int split_in_progress = 0;
char *head = read_line_from_git_path("HEAD");
char *orig_head = read_line_from_git_path("ORIG_HEAD");
char *rebase_amend = read_line_from_git_path("rebase-merge/amend");
char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
- !s->branch || strcmp(s->branch, "HEAD"))
+ !s->branch || strcmp(s->branch, "HEAD")) {
+ free(head);
+ free(orig_head);
+ free(rebase_amend);
+ free(rebase_orig_head);
return split_in_progress;
+ }
if (!strcmp(rebase_amend, rebase_orig_head)) {
if (strcmp(head, rebase_amend))
split_in_progress = 1;
} else if (strcmp(orig_head, rebase_orig_head)) {
split_in_progress = 1;
}
if (!s->amend && !s->nowarn && !s->workdir_dirty)
split_in_progress = 0;
free(head);
free(orig_head);
free(rebase_amend);
free(rebase_orig_head);
return split_in_progress;
}
So as you see, if we make the change you suggested, the next if() is hit
which possibly sets split_in_progress = 0.
The thing is: this variable has been initialized to 0 in the beginning! So
this conditional assignment would be a noop, unless any of the code paths
before this conditional modified split_in_progress.
After you fully wrapped your head around this code, I am sure you agree
that the code is unnecessarily confusing to begin with: why do we go out
of our way to allocate and read all those strings, compare them to figure
out whether there is a split in progress, just to look at bits in the
wt_status struct (that we had available from the beginning) to potentially
undo all of that.
What's worse, I cannot even find a logical explanation why the code is so
confusing, as it has been added as it is today in commit 2d1ccebae41
(status: better advices when splitting a commit (during rebase -i),
2012-06-10).
So I think this function really wants to be fixed more fully (although I
feel bad for inserting such a non-trivial fix into a patch series
otherwise populated by trivial memory/resource leak plugs):
-- snipsnap --
Subject: [PATCH] squash! split_commit_in_progress(): fix memory leak
split_commit_in_progress(): simplify & fix memory leak
This function did a whole lot of unnecessary work, such as reading in
four files just to figure out that, oh, hey, we do not need to look at
them after all because the HEAD is not detached.
Simplify the entire function to return early when possible, to read in
the files only when necessary, and to release the allocated memory
always (there was a leak, reported via Coverity, where we failed to
release the allocated strings if the HEAD is not detached).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
wt-status.c | 39 +++++++++++++++++----------------------
1 file changed, 17 insertions(+), 22 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 1f3f6bcb980..117ac8cfb01 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1082,34 +1082,29 @@ static char *read_line_from_git_path(const char *filename)
static int split_commit_in_progress(struct wt_status *s)
{
int split_in_progress = 0;
- char *head = read_line_from_git_path("HEAD");
- char *orig_head = read_line_from_git_path("ORIG_HEAD");
- char *rebase_amend = read_line_from_git_path("rebase-merge/amend");
- char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
-
- if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
- !s->branch || strcmp(s->branch, "HEAD")) {
- free(head);
- free(orig_head);
- free(rebase_amend);
- free(rebase_orig_head);
- return split_in_progress;
- }
-
- if (!strcmp(rebase_amend, rebase_orig_head)) {
- if (strcmp(head, rebase_amend))
- split_in_progress = 1;
- } else if (strcmp(orig_head, rebase_orig_head)) {
- split_in_progress = 1;
- }
+ char *head, *orig_head, *rebase_amend, *rebase_orig_head;
+
+ if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
+ !s->branch || strcmp(s->branch, "HEAD"))
+ return 0;
- if (!s->amend && !s->nowarn && !s->workdir_dirty)
- split_in_progress = 0;
+ head = read_line_from_git_path("HEAD");
+ orig_head = read_line_from_git_path("ORIG_HEAD");
+ rebase_amend = read_line_from_git_path("rebase-merge/amend");
+ rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
+
+ if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
+ ; /* fall through, no split in progress */
+ else if (!strcmp(rebase_amend, rebase_orig_head))
+ split_in_progress = !!strcmp(head, rebase_amend);
+ else if (strcmp(orig_head, rebase_orig_head))
+ split_in_progress = 1;
free(head);
free(orig_head);
free(rebase_amend);
free(rebase_orig_head);
+
return split_in_progress;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* Re: [PATCH v3 12/25] split_commit_in_progress(): fix memory leak
2017-05-04 10:59 ` Johannes Schindelin
@ 2017-05-06 17:13 ` René Scharfe
2017-05-09 13:39 ` Johannes Schindelin
0 siblings, 1 reply; 178+ messages in thread
From: René Scharfe @ 2017-05-06 17:13 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Am 04.05.2017 um 12:59 schrieb Johannes Schindelin:
> Hi René,
>
> On Wed, 3 May 2017, René Scharfe wrote:
>
>> Am 02.05.2017 um 18:02 schrieb Johannes Schindelin:
>>> Reported via Coverity.
>>>
>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>> ---
>>> wt-status.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/wt-status.c b/wt-status.c
>>> index 0a6e16dbe0f..1f3f6bcb980 100644
>>> --- a/wt-status.c
>>> +++ b/wt-status.c
>>> @@ -1088,8 +1088,13 @@ static int split_commit_in_progress(struct wt_status
>>> *s)
>>> char *rebase_orig_head =
>>> read_line_from_git_path("rebase-merge/orig-head");
>>>
>>> if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
>>> - !s->branch || strcmp(s->branch, "HEAD"))
>>> + !s->branch || strcmp(s->branch, "HEAD")) {
>>> + free(head);
>>> + free(orig_head);
>>> + free(rebase_amend);
>>> + free(rebase_orig_head);
>>> return split_in_progress;
>>> + }
>>>
>>> if (!strcmp(rebase_amend, rebase_orig_head)) {
>>> if (strcmp(head, rebase_amend))
>>>
>>
>> The return line could be replaced by "; else" to achieve the same
>> result, without duplicating the free calls.
>
> True. It is much harder to explain it that way, though: the context looks
> like this:
>
> static int split_commit_in_progress(struct wt_status *s)
> {
> int split_in_progress = 0;
> char *head = read_line_from_git_path("HEAD");
> char *orig_head = read_line_from_git_path("ORIG_HEAD");
> char *rebase_amend = read_line_from_git_path("rebase-merge/amend");
> char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
>
> if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
> - !s->branch || strcmp(s->branch, "HEAD"))
> + !s->branch || strcmp(s->branch, "HEAD")) {
> + free(head);
> + free(orig_head);
> + free(rebase_amend);
> + free(rebase_orig_head);
> return split_in_progress;
> + }
>
> if (!strcmp(rebase_amend, rebase_orig_head)) {
> if (strcmp(head, rebase_amend))
> split_in_progress = 1;
> } else if (strcmp(orig_head, rebase_orig_head)) {
> split_in_progress = 1;
> }
>
> if (!s->amend && !s->nowarn && !s->workdir_dirty)
> split_in_progress = 0;
>
> free(head);
> free(orig_head);
> free(rebase_amend);
> free(rebase_orig_head);
> return split_in_progress;
> }
>
> So as you see, if we make the change you suggested, the next if() is hit
> which possibly sets split_in_progress = 0.
>
> The thing is: this variable has been initialized to 0 in the beginning! So
> this conditional assignment would be a noop, unless any of the code paths
> before this conditional modified split_in_progress.
Right. It could have been used as a quick two-line fix.
> After you fully wrapped your head around this code, I am sure you agree
> that the code is unnecessarily confusing to begin with: why do we go out
> of our way to allocate and read all those strings, compare them to figure
> out whether there is a split in progress, just to look at bits in the
> wt_status struct (that we had available from the beginning) to potentially
> undo all of that.
The only function with a higher cyclomatic complexity in this file is
wt_longstatus_print(), which spans more than 100 lines. Amazing.
> What's worse, I cannot even find a logical explanation why the code is so
> confusing, as it has been added as it is today in commit 2d1ccebae41
> (status: better advices when splitting a commit (during rebase -i),
> 2012-06-10).
Repeatedly I get the impression that defects or shortcomings tend to
cluster. It pays to take a good look at the code surrounding a find of
an automatic checker for other possible improvements.
> So I think this function really wants to be fixed more fully (although I
> feel bad for inserting such a non-trivial fix into a patch series
> otherwise populated by trivial memory/resource leak plugs):
It could be done in two steps. Doing it in one is probably OK as well.
> -- snipsnap --
> Subject: [PATCH] squash! split_commit_in_progress(): fix memory leak
>
> split_commit_in_progress(): simplify & fix memory leak
>
> This function did a whole lot of unnecessary work, such as reading in
> four files just to figure out that, oh, hey, we do not need to look at
> them after all because the HEAD is not detached.
>
> Simplify the entire function to return early when possible, to read in
> the files only when necessary, and to release the allocated memory
> always (there was a leak, reported via Coverity, where we failed to
> release the allocated strings if the HEAD is not detached).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> wt-status.c | 39 +++++++++++++++++----------------------
> 1 file changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index 1f3f6bcb980..117ac8cfb01 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1082,34 +1082,29 @@ static char *read_line_from_git_path(const char *filename)
> static int split_commit_in_progress(struct wt_status *s)
> {
> int split_in_progress = 0;
> - char *head = read_line_from_git_path("HEAD");
> - char *orig_head = read_line_from_git_path("ORIG_HEAD");
> - char *rebase_amend = read_line_from_git_path("rebase-merge/amend");
> - char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
> -
> - if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
> - !s->branch || strcmp(s->branch, "HEAD")) {
> - free(head);
> - free(orig_head);
> - free(rebase_amend);
> - free(rebase_orig_head);
> - return split_in_progress;
> - }
> -
> - if (!strcmp(rebase_amend, rebase_orig_head)) {
> - if (strcmp(head, rebase_amend))
> - split_in_progress = 1;
> - } else if (strcmp(orig_head, rebase_orig_head)) {
> - split_in_progress = 1;
> - }
> + char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> +
> + if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
> + !s->branch || strcmp(s->branch, "HEAD"))
> + return 0;
>
> - if (!s->amend && !s->nowarn && !s->workdir_dirty)
> - split_in_progress = 0;
> + head = read_line_from_git_path("HEAD");
> + orig_head = read_line_from_git_path("ORIG_HEAD");
> + rebase_amend = read_line_from_git_path("rebase-merge/amend");
> + rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
Further improvement idea (for a later series): Use rebase_path_amend()
and rebase_path_orig_head() somehow, to build each path only once.
Accessing the files HEAD and ORIG_HEAD directly seems odd.
The second part of the function should probably be moved to sequencer.c.
> +
> + if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
> + ; /* fall through, no split in progress */
You could set split_in_progress to zero here. Would save a comment
without losing readability.
> + else if (!strcmp(rebase_amend, rebase_orig_head))
> + split_in_progress = !!strcmp(head, rebase_amend);
> + else if (strcmp(orig_head, rebase_orig_head))
> + split_in_progress = 1;
>
> free(head);
> free(orig_head);
> free(rebase_amend);
> free(rebase_orig_head);
> +
Isn't the patch big enough already without rearranging the else blocks
and adding that blank line? :)
> return split_in_progress;
> }
>
>
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v3 12/25] split_commit_in_progress(): fix memory leak
2017-05-06 17:13 ` René Scharfe
@ 2017-05-09 13:39 ` Johannes Schindelin
0 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-09 13:39 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
[-- Attachment #1: Type: text/plain, Size: 3160 bytes --]
Hi René,
On Sat, 6 May 2017, René Scharfe wrote:
> Am 04.05.2017 um 12:59 schrieb Johannes Schindelin:
>
> > diff --git a/wt-status.c b/wt-status.c
> > index 1f3f6bcb980..117ac8cfb01 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -1082,34 +1082,29 @@ static char *read_line_from_git_path(const char
> > *filename)
> > static int split_commit_in_progress(struct wt_status *s)
> > {
> > int split_in_progress = 0;
> > - char *head = read_line_from_git_path("HEAD");
> > - char *orig_head = read_line_from_git_path("ORIG_HEAD");
> > - char *rebase_amend = read_line_from_git_path("rebase-merge/amend");
> > - char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
> > -
> > - if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
> > - !s->branch || strcmp(s->branch, "HEAD")) {
> > - free(head);
> > - free(orig_head);
> > - free(rebase_amend);
> > - free(rebase_orig_head);
> > - return split_in_progress;
> > - }
> > -
> > - if (!strcmp(rebase_amend, rebase_orig_head)) {
> > - if (strcmp(head, rebase_amend))
> > - split_in_progress = 1;
> > - } else if (strcmp(orig_head, rebase_orig_head)) {
> > - split_in_progress = 1;
> > - }
> > + char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> > +
> > + if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
> > + !s->branch || strcmp(s->branch, "HEAD"))
> > + return 0;
> > - if (!s->amend && !s->nowarn && !s->workdir_dirty)
> > - split_in_progress = 0;
> > + head = read_line_from_git_path("HEAD");
> > + orig_head = read_line_from_git_path("ORIG_HEAD");
> > + rebase_amend = read_line_from_git_path("rebase-merge/amend");
> > + rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
>
> Further improvement idea (for a later series): Use rebase_path_amend()
> and rebase_path_orig_head() somehow, to build each path only once.
>
> Accessing the files HEAD and ORIG_HEAD directly seems odd.
>
> The second part of the function should probably be moved to sequencer.c.
Sure. On all four accounts.
> > +
> > + if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
> > + ; /* fall through, no split in progress */
>
> You could set split_in_progress to zero here. Would save a comment
> without losing readability.
But that would confuse e.g. myself, 6 months down the road: why assign
that variable when it already has been assigned? (And we have to assign it
because there is still a code path entering none of these if/else if's
arms).
> > + else if (!strcmp(rebase_amend, rebase_orig_head))
> > + split_in_progress = !!strcmp(head, rebase_amend);
> > + else if (strcmp(orig_head, rebase_orig_head))
> > + split_in_progress = 1;
> >
> > free(head);
> > free(orig_head);
> > free(rebase_amend);
> > free(rebase_orig_head);
> > +
>
> Isn't the patch big enough already without rearranging the else blocks
> and adding that blank line? :)
The else blocks are not really rearranged; apart from the early return,
the rest of the logic has been painstakingly kept in the same order.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 178+ messages in thread
* [PATCH v3 13/25] setup_bare_git_dir(): help static analysis
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (11 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 12/25] split_commit_in_progress(): " Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-02 16:02 ` [PATCH v3 14/25] setup_discovered_git_dir(): plug memory leak Johannes Schindelin
` (12 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Coverity reported a memory leak in this function. However, it can only
be called once, as setup_git_directory() changes global state and hence
is not reentrant.
Mark the variable as static to indicate that this is a singleton.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
setup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/setup.c b/setup.c
index 0309c278218..0320a9ad14c 100644
--- a/setup.c
+++ b/setup.c
@@ -748,7 +748,7 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
- const char *gitdir;
+ static const char *gitdir;
gitdir = offset == cwd->len ? "." : xmemdupz(cwd->buf, offset);
if (chdir(cwd->buf))
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 14/25] setup_discovered_git_dir(): plug memory leak
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (12 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 13/25] setup_bare_git_dir(): help static analysis Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-02 17:20 ` Stefan Beller
2017-05-02 16:02 ` [PATCH v3 15/25] pack-redundant: " Johannes Schindelin
` (11 subsequent siblings)
25 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
The setup_explicit_git_dir() function does not take custody of the string
passed as first parameter; we have to release it if we turned the value of
git_dir into an absolute path.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
setup.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/setup.c b/setup.c
index 0320a9ad14c..e3f7699a902 100644
--- a/setup.c
+++ b/setup.c
@@ -703,11 +703,16 @@ static const char *setup_discovered_git_dir(const char *gitdir,
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
+ char *to_free = NULL;
+ const char *ret;
+
if (offset != cwd->len && !is_absolute_path(gitdir))
- gitdir = real_pathdup(gitdir, 1);
+ gitdir = to_free = real_pathdup(gitdir, 1);
if (chdir(cwd->buf))
die_errno("Could not come back to cwd");
- return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
+ ret = setup_explicit_git_dir(gitdir, cwd, nongit_ok);
+ free(to_free);
+ return ret;
}
/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* Re: [PATCH v3 14/25] setup_discovered_git_dir(): plug memory leak
2017-05-02 16:02 ` [PATCH v3 14/25] setup_discovered_git_dir(): plug memory leak Johannes Schindelin
@ 2017-05-02 17:20 ` Stefan Beller
2017-05-02 18:15 ` Jeff King
0 siblings, 1 reply; 178+ messages in thread
From: Stefan Beller @ 2017-05-02 17:20 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git@vger.kernel.org, Junio C Hamano, Johannes Sixt, Jeff King
On Tue, May 2, 2017 at 9:02 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> The setup_explicit_git_dir() function does not take custody of the string
> passed as first parameter; we have to release it if we turned the value of
> git_dir into an absolute path.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> setup.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 0320a9ad14c..e3f7699a902 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -703,11 +703,16 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>
> /* --work-tree is set without --git-dir; use discovered one */
> if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
> + char *to_free = NULL;
> + const char *ret;
> +
> if (offset != cwd->len && !is_absolute_path(gitdir))
> - gitdir = real_pathdup(gitdir, 1);
> + gitdir = to_free = real_pathdup(gitdir, 1);
> if (chdir(cwd->buf))
> die_errno("Could not come back to cwd");
As the original motivation was to shut up Coverity, this may not
accomplish that goal, as in the path of taking the die_errno, we do not
free `to_free`. But that is ok as the actual goal is to hav no memleaks
in the good case. A memleak just before a die is no big deal.
> - return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
> + ret = setup_explicit_git_dir(gitdir, cwd, nongit_ok);
> + free(to_free);
> + return ret;
And we free our pathdup from above, makes sense.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v3 14/25] setup_discovered_git_dir(): plug memory leak
2017-05-02 17:20 ` Stefan Beller
@ 2017-05-02 18:15 ` Jeff King
2017-05-03 9:35 ` Johannes Schindelin
0 siblings, 1 reply; 178+ messages in thread
From: Jeff King @ 2017-05-02 18:15 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Schindelin, git@vger.kernel.org, Junio C Hamano,
Johannes Sixt
On Tue, May 02, 2017 at 10:20:29AM -0700, Stefan Beller wrote:
> > - gitdir = real_pathdup(gitdir, 1);
> > + gitdir = to_free = real_pathdup(gitdir, 1);
> > if (chdir(cwd->buf))
> > die_errno("Could not come back to cwd");
>
> As the original motivation was to shut up Coverity, this may not
> accomplish that goal, as in the path of taking the die_errno, we do not
> free `to_free`. But that is ok as the actual goal is to hav no memleaks
> in the good case. A memleak just before a die is no big deal.
I think Coverity understands our NORETURN attributes, so this should be
fine (and if it doesn't, then we should fix that in the model file; but
from the general results I've seen, it does).
-Peff
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v3 14/25] setup_discovered_git_dir(): plug memory leak
2017-05-02 18:15 ` Jeff King
@ 2017-05-03 9:35 ` Johannes Schindelin
0 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-03 9:35 UTC (permalink / raw)
To: Jeff King
Cc: Stefan Beller, git@vger.kernel.org, Junio C Hamano, Johannes Sixt
Hi Peff,
On Tue, 2 May 2017, Jeff King wrote:
> On Tue, May 02, 2017 at 10:20:29AM -0700, Stefan Beller wrote:
>
> > > - gitdir = real_pathdup(gitdir, 1);
> > > + gitdir = to_free = real_pathdup(gitdir, 1);
> > > if (chdir(cwd->buf))
> > > die_errno("Could not come back to cwd");
> >
> > As the original motivation was to shut up Coverity, this may not
> > accomplish that goal, as in the path of taking the die_errno, we do not
> > free `to_free`. But that is ok as the actual goal is to hav no memleaks
> > in the good case. A memleak just before a die is no big deal.
>
> I think Coverity understands our NORETURN attributes, so this should be
> fine (and if it doesn't, then we should fix that in the model file; but
> from the general results I've seen, it does).
That matches my impression.
Note that Coverity cannot figure out that all of our cmd_*() functions are
called in code paths that exit() with their return value. Otherwise we
would not see reports about those tons of intentional "let's abuse exit()
as our garbage collector" instances.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 178+ messages in thread
* [PATCH v3 15/25] pack-redundant: plug memory leak
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (13 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 14/25] setup_discovered_git_dir(): plug memory leak Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-02 16:02 ` [PATCH v3 16/25] mktree: plug memory leaks reported by Coverity Johannes Schindelin
` (10 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Identified via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pack-redundant.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 72c815844dd..cb1df1c7614 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -442,6 +442,7 @@ static void minimize(struct pack_list **min)
/* return if there are no objects missing from the unique set */
if (missing->size == 0) {
*min = unique;
+ free(missing);
return;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 16/25] mktree: plug memory leaks reported by Coverity
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (14 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 15/25] pack-redundant: " Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-02 16:02 ` [PATCH v3 17/25] fast-export: avoid leaking memory in handle_tag() Johannes Schindelin
` (9 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/mktree.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/mktree.c b/builtin/mktree.c
index de9b40fc63b..da0fd8cd706 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -72,7 +72,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
unsigned mode;
enum object_type mode_type; /* object type derived from mode */
enum object_type obj_type; /* object type derived from sha */
- char *path;
+ char *path, *to_free = NULL;
unsigned char sha1[20];
ptr = buf;
@@ -102,7 +102,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
struct strbuf p_uq = STRBUF_INIT;
if (unquote_c_style(&p_uq, path, NULL))
die("invalid quoting");
- path = strbuf_detach(&p_uq, NULL);
+ path = to_free = strbuf_detach(&p_uq, NULL);
}
/*
@@ -136,6 +136,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
}
append_to_tree(mode, sha1, path);
+ free(to_free);
}
int cmd_mktree(int ac, const char **av, const char *prefix)
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 17/25] fast-export: avoid leaking memory in handle_tag()
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (15 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 16/25] mktree: plug memory leaks reported by Coverity Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-02 16:02 ` [PATCH v3 18/25] receive-pack: plug memory leak in update() Johannes Schindelin
` (8 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Reported by, you guessed it, Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/fast-export.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e0220630d00..64617ad8e36 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -734,6 +734,7 @@ static void handle_tag(const char *name, struct tag *tag)
oid_to_hex(&tag->object.oid));
case DROP:
/* Ignore this tag altogether */
+ free(buf);
return;
case REWRITE:
if (tagged->type != OBJ_COMMIT) {
@@ -765,6 +766,7 @@ static void handle_tag(const char *name, struct tag *tag)
(int)(tagger_end - tagger), tagger,
tagger == tagger_end ? "" : "\n",
(int)message_size, (int)message_size, message ? message : "");
+ free(buf);
}
static struct commit *get_commit(struct rev_cmdline_entry *e, char *full_name)
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 18/25] receive-pack: plug memory leak in update()
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (16 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 17/25] fast-export: avoid leaking memory in handle_tag() Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-02 16:02 ` [PATCH v3 19/25] line-log: avoid memory leak Johannes Schindelin
` (7 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/receive-pack.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f96834f42c9..48c07ddb659 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -986,7 +986,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
{
const char *name = cmd->ref_name;
struct strbuf namespaced_name_buf = STRBUF_INIT;
- const char *namespaced_name, *ret;
+ static char *namespaced_name;
+ const char *ret;
struct object_id *old_oid = &cmd->old_oid;
struct object_id *new_oid = &cmd->new_oid;
@@ -997,6 +998,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
}
strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
+ free(namespaced_name);
namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
if (is_ref_checked_out(namespaced_name)) {
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 19/25] line-log: avoid memory leak
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (17 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 18/25] receive-pack: plug memory leak in update() Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-02 16:02 ` [PATCH v3 20/25] shallow: " Johannes Schindelin
` (6 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Discovered by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
line-log.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/line-log.c b/line-log.c
index a23b910471b..b9087814b8c 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1125,6 +1125,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c
changed = process_all_files(&parent_range, rev, &queue, range);
if (parent)
add_line_range(rev, parent, parent_range);
+ free_line_log_data(parent_range);
return changed;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 20/25] shallow: avoid memory leak
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (18 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 19/25] line-log: avoid memory leak Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-02 16:02 ` [PATCH v3 21/25] add_reflog_for_walk: " Johannes Schindelin
` (5 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Reported by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
shallow.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/shallow.c b/shallow.c
index 25b6db989bf..f9370961f99 100644
--- a/shallow.c
+++ b/shallow.c
@@ -473,11 +473,15 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
struct commit_list *head = NULL;
int bitmap_nr = (info->nr_bits + 31) / 32;
size_t bitmap_size = st_mult(sizeof(uint32_t), bitmap_nr);
- uint32_t *tmp = xmalloc(bitmap_size); /* to be freed before return */
- uint32_t *bitmap = paint_alloc(info);
struct commit *c = lookup_commit_reference_gently(sha1, 1);
+ uint32_t *tmp; /* to be freed before return */
+ uint32_t *bitmap;
+
if (!c)
return;
+
+ tmp = xmalloc(bitmap_size);
+ bitmap = paint_alloc(info);
memset(bitmap, 0, bitmap_size);
bitmap[id / 32] |= (1U << (id % 32));
commit_list_insert(c, &head);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 21/25] add_reflog_for_walk: avoid memory leak
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (19 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 20/25] shallow: " Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-02 16:02 ` [PATCH v3 22/25] remote: plug memory leak in match_explicit() Johannes Schindelin
` (4 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
We free()d the `log` buffer when dwim_log() returned 1, but not when it
returned a larger value (which meant that it still allocated the buffer
but we simply ignored it).
While in the vicinity, make sure that the `reflogs` structure as well as
the `branch` variable are released properly, too.
Identified by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
reflog-walk.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/reflog-walk.c b/reflog-walk.c
index 99679f58255..c63eb1a3fd7 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -183,7 +183,11 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
if (!reflogs || reflogs->nr == 0) {
struct object_id oid;
char *b;
- if (dwim_log(branch, strlen(branch), oid.hash, &b) == 1) {
+ int ret = dwim_log(branch, strlen(branch),
+ oid.hash, &b);
+ if (ret > 1)
+ free(b);
+ else if (ret == 1) {
if (reflogs) {
free(reflogs->ref);
free(reflogs);
@@ -193,17 +197,27 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
reflogs = read_complete_reflog(branch);
}
}
- if (!reflogs || reflogs->nr == 0)
+ if (!reflogs || reflogs->nr == 0) {
+ if (reflogs) {
+ free(reflogs->ref);
+ free(reflogs);
+ }
+ free(branch);
return -1;
+ }
string_list_insert(&info->complete_reflogs, branch)->util
= reflogs;
}
+ free(branch);
commit_reflog = xcalloc(1, sizeof(struct commit_reflog));
if (recno < 0) {
commit_reflog->recno = get_reflog_recno_by_time(reflogs, timestamp);
if (commit_reflog->recno < 0) {
- free(branch);
+ if (reflogs) {
+ free(reflogs->ref);
+ free(reflogs);
+ }
free(commit_reflog);
return -1;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 22/25] remote: plug memory leak in match_explicit()
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (20 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 21/25] add_reflog_for_walk: " Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-02 16:02 ` [PATCH v3 23/25] name-rev: avoid leaking memory in the `deref` case Johannes Schindelin
` (3 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
The `guess_ref()` returns an allocated buffer of which `make_linked_ref()`
does not take custody (`alloc_ref()` makes a copy), therefore we need to
release the buffer afterwards.
Noticed via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
remote.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/remote.c b/remote.c
index 801137c72eb..72b4591b983 100644
--- a/remote.c
+++ b/remote.c
@@ -1191,9 +1191,10 @@ static int match_explicit(struct ref *src, struct ref *dst,
else if (is_null_oid(&matched_src->new_oid))
error("unable to delete '%s': remote ref does not exist",
dst_value);
- else if ((dst_guess = guess_ref(dst_value, matched_src)))
+ else if ((dst_guess = guess_ref(dst_value, matched_src))) {
matched_dst = make_linked_ref(dst_guess, dst_tail);
- else
+ free(dst_guess);
+ } else
error("unable to push to unqualified destination: %s\n"
"The destination refspec neither matches an "
"existing ref on the remote nor\n"
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 23/25] name-rev: avoid leaking memory in the `deref` case
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (21 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 22/25] remote: plug memory leak in match_explicit() Johannes Schindelin
@ 2017-05-02 16:02 ` Johannes Schindelin
2017-05-02 16:03 ` [PATCH v3 24/25] show_worktree(): plug memory leak Johannes Schindelin
` (2 subsequent siblings)
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
When the `name_rev()` function is asked to dereference the tip name, it
allocates memory. But when it turns out that another tip already
described the commit better than the current one, we forgot to release
the memory.
Pointed out by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/name-rev.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 92a5d8a5d26..e7a3fe7ee70 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -28,6 +28,7 @@ static void name_rev(struct commit *commit,
struct rev_name *name = (struct rev_name *)commit->util;
struct commit_list *parents;
int parent_number = 1;
+ char *to_free = NULL;
parse_commit(commit);
@@ -35,7 +36,7 @@ static void name_rev(struct commit *commit,
return;
if (deref) {
- tip_name = xstrfmt("%s^0", tip_name);
+ tip_name = to_free = xstrfmt("%s^0", tip_name);
if (generation)
die("generation: %d, but deref?", generation);
@@ -53,8 +54,10 @@ static void name_rev(struct commit *commit,
name->taggerdate = taggerdate;
name->generation = generation;
name->distance = distance;
- } else
+ } else {
+ free(to_free);
return;
+ }
for (parents = commit->parents;
parents;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 24/25] show_worktree(): plug memory leak
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (22 preceding siblings ...)
2017-05-02 16:02 ` [PATCH v3 23/25] name-rev: avoid leaking memory in the `deref` case Johannes Schindelin
@ 2017-05-02 16:03 ` Johannes Schindelin
2017-05-02 16:03 ` [PATCH v3 25/25] submodule_uses_worktrees(): " Johannes Schindelin
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
The buffer allocated by shorten_unambiguous_ref() needs to be released.
Discovered by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/worktree.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 1722a9bdc2a..ff5dfd2b102 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -414,9 +414,11 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
if (wt->is_detached)
strbuf_addstr(&sb, "(detached HEAD)");
- else if (wt->head_ref)
- strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
- else
+ else if (wt->head_ref) {
+ char *ref = shorten_unambiguous_ref(wt->head_ref, 0);
+ strbuf_addf(&sb, "[%s]", ref);
+ free(ref);
+ } else
strbuf_addstr(&sb, "(error)");
}
printf("%s\n", sb.buf);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v3 25/25] submodule_uses_worktrees(): plug memory leak
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (23 preceding siblings ...)
2017-05-02 16:03 ` [PATCH v3 24/25] show_worktree(): plug memory leak Johannes Schindelin
@ 2017-05-02 16:03 ` Johannes Schindelin
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
25 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-02 16:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
There is really no reason why we would need to hold onto the allocated
string longer than necessary.
Reported by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
worktree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/worktree.c b/worktree.c
index bae787cf8d7..89a81b13de3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -399,6 +399,7 @@ int submodule_uses_worktrees(const char *path)
/* The env would be set for the superproject. */
get_common_dir_noenv(&sb, submodule_gitdir);
+ free(submodule_gitdir);
/*
* The check below is only known to be good for repository format
@@ -418,7 +419,6 @@ int submodule_uses_worktrees(const char *path)
/* See if there is any file inside the worktrees directory. */
dir = opendir(sb.buf);
strbuf_release(&sb);
- free(submodule_gitdir);
if (!dir)
return 0;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 00/25] Address a couple of issues identified by Coverity
2017-05-02 16:00 ` [PATCH v3 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (24 preceding siblings ...)
2017-05-02 16:03 ` [PATCH v3 25/25] submodule_uses_worktrees(): " Johannes Schindelin
@ 2017-05-04 13:54 ` Johannes Schindelin
2017-05-04 13:55 ` [PATCH v4 01/25] mingw: avoid memory leak when splitting PATH Johannes Schindelin
` (24 more replies)
25 siblings, 25 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:54 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
[-- Attachment #1: Type: text/plain, Size: 7580 bytes --]
I recently registered the git-for-windows fork with Coverity to ensure
that even the Windows-specific patches get some static analysis love.
While at it, I squashed a couple of obvious issues in the part that is
not Windows-specific.
Note: while this patch series squashes some of those issues, the
remaining issues are not necessarily all false positives (e.g. Coverity
getting fooled by our FLEX_ARRAY trick into believing that the last
member of the struct is indeed a 0 or 1 size array) or intentional (e.g.
builtins not releasing memory because exit() is called right after
returning from the function that leaks memory).
Notable examples of the remaining issues are:
- a couple of callers of shorten_unambiguous_ref() assume that they do
not have to release the memory returned from that function, often
assigning the pointer to a `const` variable that typically does not
hold a pointer that needs to be free()d. My hunch is that we will want
to convert that function to have a fixed number of static buffers and
use those in a round robin fashion à la sha1_to_hex().
- pack-redundant.c seems to have hard-to-reason-about code paths that
may eventually leak memory. Essentially, the custody of the allocated
memory is not clear at all.
- fast-import.c calls strbuf_detach() on the command_buf without any
obvious rationale. Turns out that *some* code paths assign
command_buf.buf to a `recent_command` which releases the buffer later.
However, from a cursory look it appears to me as if there are some
other code paths that skip that assignment, effectively causing a memory
leak once strbuf_detach() is called.
Sadly, I lack the time to pursue those remaining issues further.
Changes since v3:
- used 0 (black) for the foreground color attributes in winansi when we
have no console to print to, anyway.
- clarified in the commit message when we hit the path, and why, where
we set the foreground color attributes to "all black".
- reworded the commit message talking about splitting the PATH (on
Windows, it is delimited by semicolons, not colons, but it is even
better to just talk about the path delimiters because it does not
really happen which character is used, but it is important which role
it plays).
- rewrote the split_commit_in_progress() function to have a more natural
flow while still fixing the memory leak.
Johannes Schindelin (25):
mingw: avoid memory leak when splitting PATH
winansi: avoid use of uninitialized value
winansi: avoid buffer overrun
add_commit_patch_id(): avoid allocating memory unnecessarily
git_config_rename_section_in_file(): avoid resource leak
get_mail_commit_oid(): avoid resource leak
difftool: address a couple of resource/memory leaks
status: close file descriptor after reading git-rebase-todo
mailinfo & mailsplit: check for EOF while parsing
cat-file: fix memory leak
checkout: fix memory leak
split_commit_in_progress(): simplify & fix memory leak
setup_bare_git_dir(): help static analysis
setup_discovered_git_dir(): plug memory leak
pack-redundant: plug memory leak
mktree: plug memory leaks reported by Coverity
fast-export: avoid leaking memory in handle_tag()
receive-pack: plug memory leak in update()
line-log: avoid memory leak
shallow: avoid memory leak
add_reflog_for_walk: avoid memory leak
remote: plug memory leak in match_explicit()
name-rev: avoid leaking memory in the `deref` case
show_worktree(): plug memory leak
submodule_uses_worktrees(): plug memory leak
builtin/am.c | 15 ++++++---------
builtin/cat-file.c | 1 +
builtin/checkout.c | 17 +++++++++--------
builtin/difftool.c | 33 +++++++++++++++++++++++----------
builtin/fast-export.c | 2 ++
builtin/mailsplit.c | 10 ++++++++++
builtin/mktree.c | 5 +++--
builtin/name-rev.c | 7 +++++--
builtin/pack-redundant.c | 1 +
builtin/receive-pack.c | 4 +++-
builtin/worktree.c | 8 +++++---
compat/mingw.c | 4 +++-
compat/winansi.c | 12 ++++++++++++
config.c | 5 ++++-
line-log.c | 1 +
mailinfo.c | 9 ++++++++-
patch-ids.c | 3 ++-
reflog-walk.c | 20 +++++++++++++++++---
remote.c | 5 +++--
setup.c | 11 ++++++++---
shallow.c | 8 ++++++--
worktree.c | 2 +-
wt-status.c | 29 +++++++++++++++--------------
23 files changed, 148 insertions(+), 64 deletions(-)
base-commit: d2bbb7c2bcf6e77ebfcabf4e12110fe6d5c91de6
Published-As: https://github.com/dscho/git/releases/tag/coverity-v4
Fetch-It-Via: git fetch https://github.com/dscho/git coverity-v4
Interdiff vs v3:
diff --git a/compat/winansi.c b/compat/winansi.c
index 861b79d8c31..a11a0f16d27 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -105,8 +105,13 @@ static int is_console(int fd)
if (!fd) {
if (!GetConsoleMode(hcon, &mode))
return 0;
- sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN |
- FOREGROUND_RED;
+ /*
+ * This code path is only reached if there is no console
+ * attached to stdout/stderr, i.e. we will not need to output
+ * any text to any console, therefore we might just as well
+ * use black as foreground color.
+ */
+ sbi.wAttributes = 0;
} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
return 0;
diff --git a/wt-status.c b/wt-status.c
index 1f3f6bcb980..117ac8cfb01 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1082,34 +1082,29 @@ static char *read_line_from_git_path(const char *filename)
static int split_commit_in_progress(struct wt_status *s)
{
int split_in_progress = 0;
- char *head = read_line_from_git_path("HEAD");
- char *orig_head = read_line_from_git_path("ORIG_HEAD");
- char *rebase_amend = read_line_from_git_path("rebase-merge/amend");
- char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
-
- if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
- !s->branch || strcmp(s->branch, "HEAD")) {
- free(head);
- free(orig_head);
- free(rebase_amend);
- free(rebase_orig_head);
- return split_in_progress;
- }
-
- if (!strcmp(rebase_amend, rebase_orig_head)) {
- if (strcmp(head, rebase_amend))
- split_in_progress = 1;
- } else if (strcmp(orig_head, rebase_orig_head)) {
- split_in_progress = 1;
- }
+ char *head, *orig_head, *rebase_amend, *rebase_orig_head;
+
+ if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
+ !s->branch || strcmp(s->branch, "HEAD"))
+ return 0;
- if (!s->amend && !s->nowarn && !s->workdir_dirty)
- split_in_progress = 0;
+ head = read_line_from_git_path("HEAD");
+ orig_head = read_line_from_git_path("ORIG_HEAD");
+ rebase_amend = read_line_from_git_path("rebase-merge/amend");
+ rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
+
+ if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
+ ; /* fall through, no split in progress */
+ else if (!strcmp(rebase_amend, rebase_orig_head))
+ split_in_progress = !!strcmp(head, rebase_amend);
+ else if (strcmp(orig_head, rebase_orig_head))
+ split_in_progress = 1;
free(head);
free(orig_head);
free(rebase_amend);
free(rebase_orig_head);
+
return split_in_progress;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply [flat|nested] 178+ messages in thread
* [PATCH v4 01/25] mingw: avoid memory leak when splitting PATH
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
@ 2017-05-04 13:55 ` Johannes Schindelin
2017-05-04 13:55 ` [PATCH v4 02/25] winansi: avoid use of uninitialized value Johannes Schindelin
` (23 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
In the (admittedly, concocted) case that PATH consists only of path
delimiters, we would leak the duplicated string.
Reported by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/mingw.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 3fbfda5978b..fe0e3ccd248 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -961,8 +961,10 @@ static char **get_path_split(void)
++n;
}
}
- if (!n)
+ if (!n) {
+ free(envpath);
return NULL;
+ }
ALLOC_ARRAY(path, n + 1);
p = envpath;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 02/25] winansi: avoid use of uninitialized value
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
2017-05-04 13:55 ` [PATCH v4 01/25] mingw: avoid memory leak when splitting PATH Johannes Schindelin
@ 2017-05-04 13:55 ` Johannes Schindelin
2017-05-04 13:55 ` [PATCH v4 03/25] winansi: avoid buffer overrun Johannes Schindelin
` (22 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
To initialize the foreground color attributes of "plain text", our ANSI
emulation tries to infer them from the currently attached console while
running the is_console() function. This function first tries to detect any
console attached to stdout, then it is called with stderr.
If neither stdout nor stderr has any console attached, it does not
actually matter what we use for "plain text" attributes, as we never need
to output any text to any console in that case.
However, after working on stdout and stderr, is_console() is called with
stdin, and it still tries to initialize the "plain text" attributes if
they had not been initialized earlier. In this case, we cannot detect any
attributes, and we used an uninitialized value for them.
Naturally, Coverity complained about this use case because it could not
reason about the code deeply enough to figure out that we do not even use
those attributes in that case.
Let's just initialize the value to 0 in that case, both to avoid future
Coverity reports, and to help catch future regressions in case anybody
changes the order of the is_console() calls (which would make the text
black on black).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/winansi.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/compat/winansi.c b/compat/winansi.c
index 793420f9d0d..a551de90eb0 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -105,6 +105,13 @@ static int is_console(int fd)
if (!fd) {
if (!GetConsoleMode(hcon, &mode))
return 0;
+ /*
+ * This code path is only reached if there is no console
+ * attached to stdout/stderr, i.e. we will not need to output
+ * any text to any console, therefore we might just as well
+ * use black as foreground color.
+ */
+ sbi.wAttributes = 0;
} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
return 0;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 03/25] winansi: avoid buffer overrun
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
2017-05-04 13:55 ` [PATCH v4 01/25] mingw: avoid memory leak when splitting PATH Johannes Schindelin
2017-05-04 13:55 ` [PATCH v4 02/25] winansi: avoid use of uninitialized value Johannes Schindelin
@ 2017-05-04 13:55 ` Johannes Schindelin
2017-05-04 13:55 ` [PATCH v4 04/25] add_commit_patch_id(): avoid allocating memory unnecessarily Johannes Schindelin
` (21 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
When we could not convert the UTF-8 sequence into Unicode for writing to
the Console, we should not try to write an insanely-long sequence of
invalid wide characters (mistaking the negative return value for an
unsigned length).
Reported by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/winansi.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/compat/winansi.c b/compat/winansi.c
index a551de90eb0..a11a0f16d27 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -140,6 +140,11 @@ static void write_console(unsigned char *str, size_t len)
/* convert utf-8 to utf-16 */
int wlen = xutftowcsn(wbuf, (char*) str, ARRAY_SIZE(wbuf), len);
+ if (wlen < 0) {
+ wchar_t *err = L"[invalid]";
+ WriteConsoleW(console, err, wcslen(err), &dummy, NULL);
+ return;
+ }
/* write directly to console */
WriteConsoleW(console, wbuf, wlen, &dummy, NULL);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 04/25] add_commit_patch_id(): avoid allocating memory unnecessarily
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (2 preceding siblings ...)
2017-05-04 13:55 ` [PATCH v4 03/25] winansi: avoid buffer overrun Johannes Schindelin
@ 2017-05-04 13:55 ` Johannes Schindelin
2017-05-04 13:55 ` [PATCH v4 05/25] git_config_rename_section_in_file(): avoid resource leak Johannes Schindelin
` (20 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
It would appear that we allocate (and forget to release) memory if the
patch ID is not even defined.
Reported by the Coverity tool.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
patch-ids.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/patch-ids.c b/patch-ids.c
index fa8f11de826..92eba7a059e 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -99,11 +99,12 @@ struct patch_id *has_commit_patch_id(struct commit *commit,
struct patch_id *add_commit_patch_id(struct commit *commit,
struct patch_ids *ids)
{
- struct patch_id *key = xcalloc(1, sizeof(*key));
+ struct patch_id *key;
if (!patch_id_defined(commit))
return NULL;
+ key = xcalloc(1, sizeof(*key));
if (init_patch_id_entry(key, commit, ids)) {
free(key);
return NULL;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 05/25] git_config_rename_section_in_file(): avoid resource leak
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (3 preceding siblings ...)
2017-05-04 13:55 ` [PATCH v4 04/25] add_commit_patch_id(): avoid allocating memory unnecessarily Johannes Schindelin
@ 2017-05-04 13:55 ` Johannes Schindelin
2017-05-04 13:55 ` [PATCH v4 06/25] get_mail_commit_oid(): " Johannes Schindelin
` (19 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
In case of errors, we really want the file descriptor to be closed.
Discovered by a Coverity scan.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index b4a3205da32..a30056ec7e9 100644
--- a/config.c
+++ b/config.c
@@ -2621,7 +2621,7 @@ int git_config_rename_section_in_file(const char *config_filename,
struct lock_file *lock;
int out_fd;
char buf[1024];
- FILE *config_file;
+ FILE *config_file = NULL;
struct stat st;
if (new_name && !section_name_is_ok(new_name)) {
@@ -2703,11 +2703,14 @@ int git_config_rename_section_in_file(const char *config_filename,
}
}
fclose(config_file);
+ config_file = NULL;
commit_and_out:
if (commit_lock_file(lock) < 0)
ret = error_errno("could not write config file %s",
config_filename);
out:
+ if (config_file)
+ fclose(config_file);
rollback_lock_file(lock);
out_no_rollback:
free(filename_buf);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 06/25] get_mail_commit_oid(): avoid resource leak
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (4 preceding siblings ...)
2017-05-04 13:55 ` [PATCH v4 05/25] git_config_rename_section_in_file(): avoid resource leak Johannes Schindelin
@ 2017-05-04 13:55 ` Johannes Schindelin
2017-05-04 13:55 ` [PATCH v4 07/25] difftool: address a couple of resource/memory leaks Johannes Schindelin
` (18 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
When we fail to read, or parse, the file, we still want to close the file
descriptor and release the strbuf.
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/am.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index a95dd8b4e6c..9c5c2c778e2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1351,19 +1351,16 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail)
struct strbuf sb = STRBUF_INIT;
FILE *fp = xfopen(mail, "r");
const char *x;
+ int ret = 0;
- if (strbuf_getline_lf(&sb, fp))
- return -1;
-
- if (!skip_prefix(sb.buf, "From ", &x))
- return -1;
-
- if (get_oid_hex(x, commit_id) < 0)
- return -1;
+ if (strbuf_getline_lf(&sb, fp) ||
+ !skip_prefix(sb.buf, "From ", &x) ||
+ get_oid_hex(x, commit_id) < 0)
+ ret = -1;
strbuf_release(&sb);
fclose(fp);
- return 0;
+ return ret;
}
/**
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 07/25] difftool: address a couple of resource/memory leaks
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (5 preceding siblings ...)
2017-05-04 13:55 ` [PATCH v4 06/25] get_mail_commit_oid(): " Johannes Schindelin
@ 2017-05-04 13:55 ` Johannes Schindelin
2017-05-04 13:55 ` [PATCH v4 08/25] status: close file descriptor after reading git-rebase-todo Johannes Schindelin
` (17 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
This change plugs a couple of memory leaks and makes sure that the file
descriptor is closed in run_dir_diff().
Spotted by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/difftool.c | 33 +++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 1354d0e4625..b9a892f2693 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -226,6 +226,7 @@ static void changed_files(struct hashmap *result, const char *index_path,
hashmap_entry_init(entry, strhash(buf.buf));
hashmap_add(result, entry);
}
+ fclose(fp);
if (finish_command(&diff_files))
die("diff-files did not exit properly");
strbuf_release(&index_env);
@@ -439,8 +440,10 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
}
if (lmode && status != 'C') {
- if (checkout_path(lmode, &loid, src_path, &lstate))
- return error("could not write '%s'", src_path);
+ if (checkout_path(lmode, &loid, src_path, &lstate)) {
+ ret = error("could not write '%s'", src_path);
+ goto finish;
+ }
}
if (rmode && !S_ISLNK(rmode)) {
@@ -456,9 +459,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
hashmap_add(&working_tree_dups, entry);
if (!use_wt_file(workdir, dst_path, &roid)) {
- if (checkout_path(rmode, &roid, dst_path, &rstate))
- return error("could not write '%s'",
- dst_path);
+ if (checkout_path(rmode, &roid, dst_path,
+ &rstate)) {
+ ret = error("could not write '%s'",
+ dst_path);
+ goto finish;
+ }
} else if (!is_null_oid(&roid)) {
/*
* Changes in the working tree need special
@@ -473,10 +479,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
ADD_CACHE_JUST_APPEND);
add_path(&rdir, rdir_len, dst_path);
- if (ensure_leading_directories(rdir.buf))
- return error("could not create "
- "directory for '%s'",
- dst_path);
+ if (ensure_leading_directories(rdir.buf)) {
+ ret = error("could not create "
+ "directory for '%s'",
+ dst_path);
+ goto finish;
+ }
add_path(&wtdir, wtdir_len, dst_path);
if (symlinks) {
if (symlink(wtdir.buf, rdir.buf)) {
@@ -497,13 +505,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
}
}
+ fclose(fp);
+ fp = NULL;
if (finish_command(&child)) {
ret = error("error occurred running diff --raw");
goto finish;
}
if (!i)
- return 0;
+ goto finish;
/*
* Changes to submodules require special treatment.This loop writes a
@@ -626,6 +636,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
exit_cleanup(tmpdir, rc);
finish:
+ if (fp)
+ fclose(fp);
+
free(lbase_dir);
free(rbase_dir);
strbuf_release(&ldir);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 08/25] status: close file descriptor after reading git-rebase-todo
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (6 preceding siblings ...)
2017-05-04 13:55 ` [PATCH v4 07/25] difftool: address a couple of resource/memory leaks Johannes Schindelin
@ 2017-05-04 13:55 ` Johannes Schindelin
2017-05-04 13:56 ` [PATCH v4 09/25] mailinfo & mailsplit: check for EOF while parsing Johannes Schindelin
` (16 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
wt-status.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/wt-status.c b/wt-status.c
index 03754849626..0a6e16dbe0f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1168,6 +1168,7 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines)
abbrev_sha1_in_line(&line);
string_list_append(lines, line.buf);
}
+ fclose(f);
return 0;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 09/25] mailinfo & mailsplit: check for EOF while parsing
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (7 preceding siblings ...)
2017-05-04 13:55 ` [PATCH v4 08/25] status: close file descriptor after reading git-rebase-todo Johannes Schindelin
@ 2017-05-04 13:56 ` Johannes Schindelin
2017-05-04 13:56 ` [PATCH v4 10/25] cat-file: fix memory leak Johannes Schindelin
` (15 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
While POSIX states that it is okay to pass EOF to isspace() (and it seems
to be implied that EOF should *not* be treated as whitespace), and also to
pass EOF to ungetc() (which seems to be intended to fail without buffering
the character), it is much better to handle these cases explicitly. Not
only does it reduce head-scratching (and helps static analysis avoid
reporting false positives), it also lets us handle files containing
nothing but whitespace by erroring out.
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/mailsplit.c | 10 ++++++++++
mailinfo.c | 9 ++++++++-
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 30681681c13..664400b8169 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -232,6 +232,16 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
do {
peek = fgetc(f);
+ if (peek == EOF) {
+ if (f == stdin)
+ /* empty stdin is OK */
+ ret = skip;
+ else {
+ fclose(f);
+ error(_("empty mbox: '%s'"), file);
+ }
+ goto out;
+ }
} while (isspace(peek));
ungetc(peek, f);
diff --git a/mailinfo.c b/mailinfo.c
index 68037758f2f..f92cb9f729c 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -882,7 +882,10 @@ static int read_one_header_line(struct strbuf *line, FILE *in)
for (;;) {
int peek;
- peek = fgetc(in); ungetc(peek, in);
+ peek = fgetc(in);
+ if (peek == EOF)
+ break;
+ ungetc(peek, in);
if (peek != ' ' && peek != '\t')
break;
if (strbuf_getline_lf(&continuation, in))
@@ -1099,6 +1102,10 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch)
do {
peek = fgetc(mi->input);
+ if (peek == EOF) {
+ fclose(cmitmsg);
+ return error("empty patch: '%s'", patch);
+ }
} while (isspace(peek));
ungetc(peek, mi->input);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 10/25] cat-file: fix memory leak
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (8 preceding siblings ...)
2017-05-04 13:56 ` [PATCH v4 09/25] mailinfo & mailsplit: check for EOF while parsing Johannes Schindelin
@ 2017-05-04 13:56 ` Johannes Schindelin
2017-05-04 13:56 ` [PATCH v4 11/25] checkout: " Johannes Schindelin
` (14 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
Discovered by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/cat-file.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1890d7a6390..9af863e7915 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -165,6 +165,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
die("git cat-file %s: bad file", obj_name);
write_or_die(1, buf, size);
+ free(buf);
return 0;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 11/25] checkout: fix memory leak
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (9 preceding siblings ...)
2017-05-04 13:56 ` [PATCH v4 10/25] cat-file: fix memory leak Johannes Schindelin
@ 2017-05-04 13:56 ` Johannes Schindelin
2017-05-06 17:14 ` René Scharfe
2017-05-04 13:56 ` [PATCH v4 12/25] split_commit_in_progress(): simplify & " Johannes Schindelin
` (13 subsequent siblings)
24 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
This change addresses part of the NEEDSWORK comment above the code,
therefore the comment needs to be adjusted, too.
Discovered via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/checkout.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfa5419f335..5faea3a05fa 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -235,14 +235,14 @@ static int checkout_merged(int pos, const struct checkout *state)
/*
* NEEDSWORK:
* There is absolutely no reason to write this as a blob object
- * and create a phony cache entry just to leak. This hack is
- * primarily to get to the write_entry() machinery that massages
- * the contents to work-tree format and writes out which only
- * allows it for a cache entry. The code in write_entry() needs
- * to be refactored to allow us to feed a <buffer, size, mode>
- * instead of a cache entry. Such a refactoring would help
- * merge_recursive as well (it also writes the merge result to the
- * object database even when it may contain conflicts).
+ * and create a phony cache entry. This hack is primarily to get
+ * to the write_entry() machinery that massages the contents to
+ * work-tree format and writes out which only allows it for a
+ * cache entry. The code in write_entry() needs to be refactored
+ * to allow us to feed a <buffer, size, mode> instead of a cache
+ * entry. Such a refactoring would help merge_recursive as well
+ * (it also writes the merge result to the object database even
+ * when it may contain conflicts).
*/
if (write_sha1_file(result_buf.ptr, result_buf.size,
blob_type, oid.hash))
@@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct checkout *state)
if (!ce)
die(_("make_cache_entry failed for path '%s'"), path);
status = checkout_entry(ce, state, NULL);
+ free(ce);
return status;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* Re: [PATCH v4 11/25] checkout: fix memory leak
2017-05-04 13:56 ` [PATCH v4 11/25] checkout: " Johannes Schindelin
@ 2017-05-06 17:14 ` René Scharfe
2017-05-08 0:41 ` Junio C Hamano
0 siblings, 1 reply; 178+ messages in thread
From: René Scharfe @ 2017-05-06 17:14 UTC (permalink / raw)
To: Johannes Schindelin, git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King
Am 04.05.2017 um 15:56 schrieb Johannes Schindelin:
> This change addresses part of the NEEDSWORK comment above the code,
> therefore the comment needs to be adjusted, too.
>
> Discovered via Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/checkout.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bfa5419f335..5faea3a05fa 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -235,14 +235,14 @@ static int checkout_merged(int pos, const struct checkout *state)
> /*
> * NEEDSWORK:
> * There is absolutely no reason to write this as a blob object
> - * and create a phony cache entry just to leak. This hack is
> - * primarily to get to the write_entry() machinery that massages
> - * the contents to work-tree format and writes out which only
> - * allows it for a cache entry. The code in write_entry() needs
> - * to be refactored to allow us to feed a <buffer, size, mode>
> - * instead of a cache entry. Such a refactoring would help
> - * merge_recursive as well (it also writes the merge result to the
> - * object database even when it may contain conflicts).
> + * and create a phony cache entry. This hack is primarily to get
> + * to the write_entry() machinery that massages the contents to
> + * work-tree format and writes out which only allows it for a
> + * cache entry. The code in write_entry() needs to be refactored
> + * to allow us to feed a <buffer, size, mode> instead of a cache
> + * entry. Such a refactoring would help merge_recursive as well
> + * (it also writes the merge result to the object database even
> + * when it may contain conflicts).
> */
> if (write_sha1_file(result_buf.ptr, result_buf.size,
> blob_type, oid.hash))
Random observation: Using pretend_sha1_file here would at least avoid
writing the blob.
> @@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct checkout *state)
> if (!ce)
> die(_("make_cache_entry failed for path '%s'"), path);
> status = checkout_entry(ce, state, NULL);
> + free(ce);
> return status;
> }
I wonder if that's safe. Why document a leak when it could have been
plugged this easily instead? A leak is better than a use after free, so
let's be extra careful here. Would it leave the index inconsistent? Or
perhaps freeing it has become safe in the meantime?
@Junio: Do you remember the reason for the leaks in 0cf8581e330
(checkout -m: recreate merge when checking out of unmerged index).
And result_buf is still leaked here, right?
FWIW, after fixing a different issue (patch just sent separately)
t*checkout*.sh complete successfully for me with --valgrind, with and
without freeing both ce and result_buf.ptr. So it *seems* safe..
René
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v4 11/25] checkout: fix memory leak
2017-05-06 17:14 ` René Scharfe
@ 2017-05-08 0:41 ` Junio C Hamano
2017-05-09 13:42 ` Johannes Schindelin
0 siblings, 1 reply; 178+ messages in thread
From: Junio C Hamano @ 2017-05-08 0:41 UTC (permalink / raw)
To: René Scharfe
Cc: Johannes Schindelin, git, Stefan Beller, Johannes Sixt, Jeff King
René Scharfe <l.s.r@web.de> writes:
>> /*
>> * NEEDSWORK:
>> * There is absolutely no reason to write this as a blob object
>> - * and create a phony cache entry just to leak. This hack is
>> - * primarily to get to the write_entry() machinery that massages
>> - * the contents to work-tree format and writes out which only
>> - * allows it for a cache entry. The code in write_entry() needs
>> - * to be refactored to allow us to feed a <buffer, size, mode>
>> - * instead of a cache entry. Such a refactoring would help
>> - * merge_recursive as well (it also writes the merge result to the
>> - * object database even when it may contain conflicts).
>> + * and create a phony cache entry. This hack is primarily to get
>> + * to the write_entry() machinery that massages the contents to
>> + * work-tree format and writes out which only allows it for a
>> + * cache entry. The code in write_entry() needs to be refactored
>> + * to allow us to feed a <buffer, size, mode> instead of a cache
>> + * entry. Such a refactoring would help merge_recursive as well
>> + * (it also writes the merge result to the object database even
>> + * when it may contain conflicts).
>> */
>> if (write_sha1_file(result_buf.ptr, result_buf.size,
>> blob_type, oid.hash))
>
> Random observation: Using pretend_sha1_file here would at least avoid
> writing the blob.
Yup, you should have told that to me back in Aug 2008 ;-) when I did
0cf8581e ("checkout -m: recreate merge when checking out of unmerged
index", 2008-08-30); pretend_sha1_file() was available since early
2007, and there is no excuse that this codepath did not use it.
>
>> @@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct checkout *state)
>> if (!ce)
>> die(_("make_cache_entry failed for path '%s'"), path);
>> status = checkout_entry(ce, state, NULL);
>> + free(ce);
>> return status;
>> }
>
> I wonder if that's safe. Why document a leak when it could have been
> plugged this easily instead?
>
> A leak is better than a use after free, so
> let's be extra careful here. Would it leave the index inconsistent? Or
> perhaps freeing it has become safe in the meantime?
>
> @Junio: Do you remember the reason for the leaks in 0cf8581e330
> (checkout -m: recreate merge when checking out of unmerged index).
Yes.
In the very old days it was not allowed to free(3) contents of
active_cache[] and this was an old brain fart that came out of
inertia. We are manufacturing a brand new ce, only to feed it to
checkout_entry() without even registering it to the active_cache[]
array, and the ancient rule doesn't even apply to such a case.
So I think it was safe to free(3) even back then.
> And result_buf is still leaked here, right?
Good spotting. It typically would make a larger leak than a single
ce, I would suppose ;-)
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v4 11/25] checkout: fix memory leak
2017-05-08 0:41 ` Junio C Hamano
@ 2017-05-09 13:42 ` Johannes Schindelin
2017-05-09 22:51 ` Junio C Hamano
0 siblings, 1 reply; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-09 13:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, git, Stefan Beller, Johannes Sixt, Jeff King
[-- Attachment #1: Type: text/plain, Size: 3391 bytes --]
Hi Junio & René,
On Mon, 8 May 2017, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
> >> /*
> >> * NEEDSWORK:
> >> * There is absolutely no reason to write this as a blob object
> >> - * and create a phony cache entry just to leak. This hack is
> >> - * primarily to get to the write_entry() machinery that massages
> >> - * the contents to work-tree format and writes out which only
> >> - * allows it for a cache entry. The code in write_entry() needs
> >> - * to be refactored to allow us to feed a <buffer, size, mode>
> >> - * instead of a cache entry. Such a refactoring would help
> >> - * merge_recursive as well (it also writes the merge result to the
> >> - * object database even when it may contain conflicts).
> >> + * and create a phony cache entry. This hack is primarily to get
> >> + * to the write_entry() machinery that massages the contents to
> >> + * work-tree format and writes out which only allows it for a
> >> + * cache entry. The code in write_entry() needs to be refactored
> >> + * to allow us to feed a <buffer, size, mode> instead of a cache
> >> + * entry. Such a refactoring would help merge_recursive as well
> >> + * (it also writes the merge result to the object database even
> >> + * when it may contain conflicts).
> >> */
> >> if (write_sha1_file(result_buf.ptr, result_buf.size,
> >> blob_type, oid.hash))
> >
> > Random observation: Using pretend_sha1_file here would at least avoid
> > writing the blob.
>
> Yup, you should have told that to me back in Aug 2008 ;-) when I did
> 0cf8581e ("checkout -m: recreate merge when checking out of unmerged
> index", 2008-08-30); pretend_sha1_file() was available since early
> 2007, and there is no excuse that this codepath did not use it.
I hope y'all agree that this is outside the scope of my patch series...
> >> @@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct checkout *state)
> >> if (!ce)
> >> die(_("make_cache_entry failed for path '%s'"), path);
> >> status = checkout_entry(ce, state, NULL);
> >> + free(ce);
> >> return status;
> >> }
> >
> > I wonder if that's safe. Why document a leak when it could have been
> > plugged this easily instead?
> >
> > A leak is better than a use after free, so
> > let's be extra careful here. Would it leave the index inconsistent? Or
> > perhaps freeing it has become safe in the meantime?
> >
> > @Junio: Do you remember the reason for the leaks in 0cf8581e330
> > (checkout -m: recreate merge when checking out of unmerged index).
>
> Yes.
>
> In the very old days it was not allowed to free(3) contents of
> active_cache[] and this was an old brain fart that came out of
> inertia. We are manufacturing a brand new ce, only to feed it to
> checkout_entry() without even registering it to the active_cache[]
> array, and the ancient rule doesn't even apply to such a case.
>
> So I think it was safe to free(3) even back then.
So this patch is good, then?
> > And result_buf is still leaked here, right?
>
> Good spotting. It typically would make a larger leak than a single
> ce, I would suppose ;-)
I saw you added this as a fixup! commit. If you don't mind, and if no
other changes are requested, would you mind rebase'ing this yourself?
Thanks,
Dscho
^ permalink raw reply [flat|nested] 178+ messages in thread
* Re: [PATCH v4 11/25] checkout: fix memory leak
2017-05-09 13:42 ` Johannes Schindelin
@ 2017-05-09 22:51 ` Junio C Hamano
0 siblings, 0 replies; 178+ messages in thread
From: Junio C Hamano @ 2017-05-09 22:51 UTC (permalink / raw)
To: Johannes Schindelin
Cc: René Scharfe, git, Stefan Beller, Johannes Sixt, Jeff King
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > A leak is better than a use after free, so
>> > let's be extra careful here. Would it leave the index inconsistent? Or
>> > perhaps freeing it has become safe in the meantime?
>> >
>> > @Junio: Do you remember the reason for the leaks in 0cf8581e330
>> > (checkout -m: recreate merge when checking out of unmerged index).
>>
>> Yes.
>>
>> In the very old days it was not allowed to free(3) contents of
>> active_cache[] and this was an old brain fart that came out of
>> inertia. We are manufacturing a brand new ce, only to feed it to
>> checkout_entry() without even registering it to the active_cache[]
>> array, and the ancient rule doesn't even apply to such a case.
>>
>> So I think it was safe to free(3) even back then.
>
> So this patch is good, then?
Unless I from this year is failing to spot a breakage that will be
caused in the codepath that I from year 2008 and René spotted, I
think freeing ce after we are done updating the working tree file
with it is safe. I'd need to find time to make sure, though.
>> > And result_buf is still leaked here, right?
>>
>> Good spotting. It typically would make a larger leak than a single
>> ce, I would suppose ;-)
>
> I saw you added this as a fixup! commit. If you don't mind, and if no
> other changes are requested, would you mind rebase'ing this yourself?
I think it would be better to leave the fix as a separate patch. It
wasn't spotted by Coverity in the first place ;-)
^ permalink raw reply [flat|nested] 178+ messages in thread
* [PATCH v4 12/25] split_commit_in_progress(): simplify & fix memory leak
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (10 preceding siblings ...)
2017-05-04 13:56 ` [PATCH v4 11/25] checkout: " Johannes Schindelin
@ 2017-05-04 13:56 ` Johannes Schindelin
2017-05-04 13:56 ` [PATCH v4 13/25] setup_bare_git_dir(): help static analysis Johannes Schindelin
` (12 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
This function did a whole lot of unnecessary work, such as reading in
four files just to figure out that, oh, hey, we do not need to look at
them after all because the HEAD is not detached.
Simplify the entire function to return early when possible, to read in
the files only when necessary, and to release the allocated memory
always (there was a leak, reported via Coverity, where we failed to
release the allocated strings if the HEAD is not detached).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
wt-status.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 0a6e16dbe0f..117ac8cfb01 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1082,29 +1082,29 @@ static char *read_line_from_git_path(const char *filename)
static int split_commit_in_progress(struct wt_status *s)
{
int split_in_progress = 0;
- char *head = read_line_from_git_path("HEAD");
- char *orig_head = read_line_from_git_path("ORIG_HEAD");
- char *rebase_amend = read_line_from_git_path("rebase-merge/amend");
- char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
+ char *head, *orig_head, *rebase_amend, *rebase_orig_head;
- if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
+ if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
!s->branch || strcmp(s->branch, "HEAD"))
- return split_in_progress;
+ return 0;
- if (!strcmp(rebase_amend, rebase_orig_head)) {
- if (strcmp(head, rebase_amend))
- split_in_progress = 1;
- } else if (strcmp(orig_head, rebase_orig_head)) {
- split_in_progress = 1;
- }
+ head = read_line_from_git_path("HEAD");
+ orig_head = read_line_from_git_path("ORIG_HEAD");
+ rebase_amend = read_line_from_git_path("rebase-merge/amend");
+ rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
- if (!s->amend && !s->nowarn && !s->workdir_dirty)
- split_in_progress = 0;
+ if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
+ ; /* fall through, no split in progress */
+ else if (!strcmp(rebase_amend, rebase_orig_head))
+ split_in_progress = !!strcmp(head, rebase_amend);
+ else if (strcmp(orig_head, rebase_orig_head))
+ split_in_progress = 1;
free(head);
free(orig_head);
free(rebase_amend);
free(rebase_orig_head);
+
return split_in_progress;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 13/25] setup_bare_git_dir(): help static analysis
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (11 preceding siblings ...)
2017-05-04 13:56 ` [PATCH v4 12/25] split_commit_in_progress(): simplify & " Johannes Schindelin
@ 2017-05-04 13:56 ` Johannes Schindelin
2017-05-04 13:56 ` [PATCH v4 14/25] setup_discovered_git_dir(): plug memory leak Johannes Schindelin
` (11 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
Coverity reported a memory leak in this function. However, it can only
be called once, as setup_git_directory() changes global state and hence
is not reentrant.
Mark the variable as static to indicate that this is a singleton.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
setup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/setup.c b/setup.c
index 0309c278218..0320a9ad14c 100644
--- a/setup.c
+++ b/setup.c
@@ -748,7 +748,7 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
- const char *gitdir;
+ static const char *gitdir;
gitdir = offset == cwd->len ? "." : xmemdupz(cwd->buf, offset);
if (chdir(cwd->buf))
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 14/25] setup_discovered_git_dir(): plug memory leak
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (12 preceding siblings ...)
2017-05-04 13:56 ` [PATCH v4 13/25] setup_bare_git_dir(): help static analysis Johannes Schindelin
@ 2017-05-04 13:56 ` Johannes Schindelin
2017-05-04 13:56 ` [PATCH v4 15/25] pack-redundant: " Johannes Schindelin
` (10 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
The setup_explicit_git_dir() function does not take custody of the string
passed as first parameter; we have to release it if we turned the value of
git_dir into an absolute path.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
setup.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/setup.c b/setup.c
index 0320a9ad14c..e3f7699a902 100644
--- a/setup.c
+++ b/setup.c
@@ -703,11 +703,16 @@ static const char *setup_discovered_git_dir(const char *gitdir,
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
+ char *to_free = NULL;
+ const char *ret;
+
if (offset != cwd->len && !is_absolute_path(gitdir))
- gitdir = real_pathdup(gitdir, 1);
+ gitdir = to_free = real_pathdup(gitdir, 1);
if (chdir(cwd->buf))
die_errno("Could not come back to cwd");
- return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
+ ret = setup_explicit_git_dir(gitdir, cwd, nongit_ok);
+ free(to_free);
+ return ret;
}
/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 15/25] pack-redundant: plug memory leak
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (13 preceding siblings ...)
2017-05-04 13:56 ` [PATCH v4 14/25] setup_discovered_git_dir(): plug memory leak Johannes Schindelin
@ 2017-05-04 13:56 ` Johannes Schindelin
2017-05-04 13:57 ` [PATCH v4 16/25] mktree: plug memory leaks reported by Coverity Johannes Schindelin
` (9 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
Identified via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pack-redundant.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 72c815844dd..cb1df1c7614 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -442,6 +442,7 @@ static void minimize(struct pack_list **min)
/* return if there are no objects missing from the unique set */
if (missing->size == 0) {
*min = unique;
+ free(missing);
return;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 16/25] mktree: plug memory leaks reported by Coverity
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (14 preceding siblings ...)
2017-05-04 13:56 ` [PATCH v4 15/25] pack-redundant: " Johannes Schindelin
@ 2017-05-04 13:57 ` Johannes Schindelin
2017-05-04 13:57 ` [PATCH v4 17/25] fast-export: avoid leaking memory in handle_tag() Johannes Schindelin
` (8 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/mktree.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/mktree.c b/builtin/mktree.c
index de9b40fc63b..da0fd8cd706 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -72,7 +72,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
unsigned mode;
enum object_type mode_type; /* object type derived from mode */
enum object_type obj_type; /* object type derived from sha */
- char *path;
+ char *path, *to_free = NULL;
unsigned char sha1[20];
ptr = buf;
@@ -102,7 +102,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
struct strbuf p_uq = STRBUF_INIT;
if (unquote_c_style(&p_uq, path, NULL))
die("invalid quoting");
- path = strbuf_detach(&p_uq, NULL);
+ path = to_free = strbuf_detach(&p_uq, NULL);
}
/*
@@ -136,6 +136,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss
}
append_to_tree(mode, sha1, path);
+ free(to_free);
}
int cmd_mktree(int ac, const char **av, const char *prefix)
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 17/25] fast-export: avoid leaking memory in handle_tag()
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (15 preceding siblings ...)
2017-05-04 13:57 ` [PATCH v4 16/25] mktree: plug memory leaks reported by Coverity Johannes Schindelin
@ 2017-05-04 13:57 ` Johannes Schindelin
2017-05-04 13:57 ` [PATCH v4 18/25] receive-pack: plug memory leak in update() Johannes Schindelin
` (7 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
Reported by, you guessed it, Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/fast-export.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e0220630d00..64617ad8e36 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -734,6 +734,7 @@ static void handle_tag(const char *name, struct tag *tag)
oid_to_hex(&tag->object.oid));
case DROP:
/* Ignore this tag altogether */
+ free(buf);
return;
case REWRITE:
if (tagged->type != OBJ_COMMIT) {
@@ -765,6 +766,7 @@ static void handle_tag(const char *name, struct tag *tag)
(int)(tagger_end - tagger), tagger,
tagger == tagger_end ? "" : "\n",
(int)message_size, (int)message_size, message ? message : "");
+ free(buf);
}
static struct commit *get_commit(struct rev_cmdline_entry *e, char *full_name)
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 18/25] receive-pack: plug memory leak in update()
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (16 preceding siblings ...)
2017-05-04 13:57 ` [PATCH v4 17/25] fast-export: avoid leaking memory in handle_tag() Johannes Schindelin
@ 2017-05-04 13:57 ` Johannes Schindelin
2017-05-04 13:58 ` [PATCH v4 19/25] line-log: avoid memory leak Johannes Schindelin
` (6 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
Reported via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/receive-pack.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f96834f42c9..48c07ddb659 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -986,7 +986,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
{
const char *name = cmd->ref_name;
struct strbuf namespaced_name_buf = STRBUF_INIT;
- const char *namespaced_name, *ret;
+ static char *namespaced_name;
+ const char *ret;
struct object_id *old_oid = &cmd->old_oid;
struct object_id *new_oid = &cmd->new_oid;
@@ -997,6 +998,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
}
strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
+ free(namespaced_name);
namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
if (is_ref_checked_out(namespaced_name)) {
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 19/25] line-log: avoid memory leak
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (17 preceding siblings ...)
2017-05-04 13:57 ` [PATCH v4 18/25] receive-pack: plug memory leak in update() Johannes Schindelin
@ 2017-05-04 13:58 ` Johannes Schindelin
2017-05-04 13:58 ` [PATCH v4 20/25] shallow: " Johannes Schindelin
` (5 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
Discovered by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
line-log.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/line-log.c b/line-log.c
index a23b910471b..b9087814b8c 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1125,6 +1125,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c
changed = process_all_files(&parent_range, rev, &queue, range);
if (parent)
add_line_range(rev, parent, parent_range);
+ free_line_log_data(parent_range);
return changed;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 20/25] shallow: avoid memory leak
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (18 preceding siblings ...)
2017-05-04 13:58 ` [PATCH v4 19/25] line-log: avoid memory leak Johannes Schindelin
@ 2017-05-04 13:58 ` Johannes Schindelin
2017-05-04 13:58 ` [PATCH v4 21/25] add_reflog_for_walk: " Johannes Schindelin
` (4 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
Reported by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
shallow.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/shallow.c b/shallow.c
index 25b6db989bf..f9370961f99 100644
--- a/shallow.c
+++ b/shallow.c
@@ -473,11 +473,15 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
struct commit_list *head = NULL;
int bitmap_nr = (info->nr_bits + 31) / 32;
size_t bitmap_size = st_mult(sizeof(uint32_t), bitmap_nr);
- uint32_t *tmp = xmalloc(bitmap_size); /* to be freed before return */
- uint32_t *bitmap = paint_alloc(info);
struct commit *c = lookup_commit_reference_gently(sha1, 1);
+ uint32_t *tmp; /* to be freed before return */
+ uint32_t *bitmap;
+
if (!c)
return;
+
+ tmp = xmalloc(bitmap_size);
+ bitmap = paint_alloc(info);
memset(bitmap, 0, bitmap_size);
bitmap[id / 32] |= (1U << (id % 32));
commit_list_insert(c, &head);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 21/25] add_reflog_for_walk: avoid memory leak
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (19 preceding siblings ...)
2017-05-04 13:58 ` [PATCH v4 20/25] shallow: " Johannes Schindelin
@ 2017-05-04 13:58 ` Johannes Schindelin
2017-05-04 13:59 ` [PATCH v4 22/25] remote: plug memory leak in match_explicit() Johannes Schindelin
` (3 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
We free()d the `log` buffer when dwim_log() returned 1, but not when it
returned a larger value (which meant that it still allocated the buffer
but we simply ignored it).
While in the vicinity, make sure that the `reflogs` structure as well as
the `branch` variable are released properly, too.
Identified by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
reflog-walk.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/reflog-walk.c b/reflog-walk.c
index 99679f58255..c63eb1a3fd7 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -183,7 +183,11 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
if (!reflogs || reflogs->nr == 0) {
struct object_id oid;
char *b;
- if (dwim_log(branch, strlen(branch), oid.hash, &b) == 1) {
+ int ret = dwim_log(branch, strlen(branch),
+ oid.hash, &b);
+ if (ret > 1)
+ free(b);
+ else if (ret == 1) {
if (reflogs) {
free(reflogs->ref);
free(reflogs);
@@ -193,17 +197,27 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
reflogs = read_complete_reflog(branch);
}
}
- if (!reflogs || reflogs->nr == 0)
+ if (!reflogs || reflogs->nr == 0) {
+ if (reflogs) {
+ free(reflogs->ref);
+ free(reflogs);
+ }
+ free(branch);
return -1;
+ }
string_list_insert(&info->complete_reflogs, branch)->util
= reflogs;
}
+ free(branch);
commit_reflog = xcalloc(1, sizeof(struct commit_reflog));
if (recno < 0) {
commit_reflog->recno = get_reflog_recno_by_time(reflogs, timestamp);
if (commit_reflog->recno < 0) {
- free(branch);
+ if (reflogs) {
+ free(reflogs->ref);
+ free(reflogs);
+ }
free(commit_reflog);
return -1;
}
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 22/25] remote: plug memory leak in match_explicit()
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (20 preceding siblings ...)
2017-05-04 13:58 ` [PATCH v4 21/25] add_reflog_for_walk: " Johannes Schindelin
@ 2017-05-04 13:59 ` Johannes Schindelin
2017-05-04 13:59 ` [PATCH v4 23/25] name-rev: avoid leaking memory in the `deref` case Johannes Schindelin
` (2 subsequent siblings)
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:59 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
The `guess_ref()` returns an allocated buffer of which `make_linked_ref()`
does not take custody (`alloc_ref()` makes a copy), therefore we need to
release the buffer afterwards.
Noticed via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
remote.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/remote.c b/remote.c
index 801137c72eb..72b4591b983 100644
--- a/remote.c
+++ b/remote.c
@@ -1191,9 +1191,10 @@ static int match_explicit(struct ref *src, struct ref *dst,
else if (is_null_oid(&matched_src->new_oid))
error("unable to delete '%s': remote ref does not exist",
dst_value);
- else if ((dst_guess = guess_ref(dst_value, matched_src)))
+ else if ((dst_guess = guess_ref(dst_value, matched_src))) {
matched_dst = make_linked_ref(dst_guess, dst_tail);
- else
+ free(dst_guess);
+ } else
error("unable to push to unqualified destination: %s\n"
"The destination refspec neither matches an "
"existing ref on the remote nor\n"
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 23/25] name-rev: avoid leaking memory in the `deref` case
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (21 preceding siblings ...)
2017-05-04 13:59 ` [PATCH v4 22/25] remote: plug memory leak in match_explicit() Johannes Schindelin
@ 2017-05-04 13:59 ` Johannes Schindelin
2017-05-04 13:59 ` [PATCH v4 24/25] show_worktree(): plug memory leak Johannes Schindelin
2017-05-04 13:59 ` [PATCH v4 25/25] submodule_uses_worktrees(): " Johannes Schindelin
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:59 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
When the `name_rev()` function is asked to dereference the tip name, it
allocates memory. But when it turns out that another tip already
described the commit better than the current one, we forgot to release
the memory.
Pointed out by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/name-rev.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 92a5d8a5d26..e7a3fe7ee70 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -28,6 +28,7 @@ static void name_rev(struct commit *commit,
struct rev_name *name = (struct rev_name *)commit->util;
struct commit_list *parents;
int parent_number = 1;
+ char *to_free = NULL;
parse_commit(commit);
@@ -35,7 +36,7 @@ static void name_rev(struct commit *commit,
return;
if (deref) {
- tip_name = xstrfmt("%s^0", tip_name);
+ tip_name = to_free = xstrfmt("%s^0", tip_name);
if (generation)
die("generation: %d, but deref?", generation);
@@ -53,8 +54,10 @@ static void name_rev(struct commit *commit,
name->taggerdate = taggerdate;
name->generation = generation;
name->distance = distance;
- } else
+ } else {
+ free(to_free);
return;
+ }
for (parents = commit->parents;
parents;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 24/25] show_worktree(): plug memory leak
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (22 preceding siblings ...)
2017-05-04 13:59 ` [PATCH v4 23/25] name-rev: avoid leaking memory in the `deref` case Johannes Schindelin
@ 2017-05-04 13:59 ` Johannes Schindelin
2017-05-04 13:59 ` [PATCH v4 25/25] submodule_uses_worktrees(): " Johannes Schindelin
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:59 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
The buffer allocated by shorten_unambiguous_ref() needs to be released.
Discovered by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/worktree.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 1722a9bdc2a..ff5dfd2b102 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -414,9 +414,11 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
if (wt->is_detached)
strbuf_addstr(&sb, "(detached HEAD)");
- else if (wt->head_ref)
- strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
- else
+ else if (wt->head_ref) {
+ char *ref = shorten_unambiguous_ref(wt->head_ref, 0);
+ strbuf_addf(&sb, "[%s]", ref);
+ free(ref);
+ } else
strbuf_addstr(&sb, "(error)");
}
printf("%s\n", sb.buf);
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread
* [PATCH v4 25/25] submodule_uses_worktrees(): plug memory leak
2017-05-04 13:54 ` [PATCH v4 00/25] Address a couple of issues identified by Coverity Johannes Schindelin
` (23 preceding siblings ...)
2017-05-04 13:59 ` [PATCH v4 24/25] show_worktree(): plug memory leak Johannes Schindelin
@ 2017-05-04 13:59 ` Johannes Schindelin
24 siblings, 0 replies; 178+ messages in thread
From: Johannes Schindelin @ 2017-05-04 13:59 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Stefan Beller, Johannes Sixt, Jeff King,
René Scharfe
There is really no reason why we would need to hold onto the allocated
string longer than necessary.
Reported by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
worktree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/worktree.c b/worktree.c
index bae787cf8d7..89a81b13de3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -399,6 +399,7 @@ int submodule_uses_worktrees(const char *path)
/* The env would be set for the superproject. */
get_common_dir_noenv(&sb, submodule_gitdir);
+ free(submodule_gitdir);
/*
* The check below is only known to be good for repository format
@@ -418,7 +419,6 @@ int submodule_uses_worktrees(const char *path)
/* See if there is any file inside the worktrees directory. */
dir = opendir(sb.buf);
strbuf_release(&sb);
- free(submodule_gitdir);
if (!dir)
return 0;
--
2.12.2.windows.2.800.gede8f145e06
^ permalink raw reply related [flat|nested] 178+ messages in thread