[PATCH] builtin-tag.c: Fix two memory leaks and minor notation changes.

[PATCH] builtin-tag.c: Fix two memory leaks and minor notation changes.

[Carlos Rica]

A repeated call to read_sha1_file was not freing memory
when the buffer was allocated but returned size was zero.

Also, now the program does not allow many -F or -m options,
which was a bug too because it was not freing the memory
allocated for any previous -F or -m options.

Tests are provided for ensuring that only one option
-F or -m is given. Also, another test is shipped here,
to check that "git tag" fails when a non-existing file
is passed to the -F option, something that git-tag.sh
allowed creating the tag with an empty message.

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---

  Those were commented in the responses to my previous patch:
  http://article.gmane.org/gmane.comp.version-control.git/53138

  I committed them on top of that my other patch already in "next".
  Unfortunately, I was not able to commit before the patches
  from Johannes supporting the EDITOR environment variable
  in builtin-tag.c and fixing its bug with a non-initialized buffer.

 builtin-tag.c  |   38 ++++++++++++++++++++++----------------
 t/t7004-tag.sh |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 507f510..9231d72 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -85,14 +85,19 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 		printf("%-15s ", refname);

 		sp = buf = read_sha1_file(sha1, &type, &size);
-		if (!buf || !size)
+		if (!buf)
 			return 0;
+		if (!size) {
+			free(buf);
+			return 0;
+		}
 		/* skip header */
 		while (sp + 1 < buf + size &&
 				!(sp[0] == '\n' && sp[1] == '\n'))
 			sp++;
 		/* only take up to "lines" lines, and strip the signature */
-		for (i = 0, sp += 2; i < filter->lines && sp < buf + size &&
+		for (i = 0, sp += 2;
+				i < filter->lines && sp < buf + size &&
 				prefixcmp(sp, PGP_SIGNATURE "\n");
 				i++) {
 			if (i)
@@ -133,10 +138,10 @@ static int list_tags(const char *pattern, int lines)
 	return 0;
 }

-typedef int (*func_tag)(const char *name, const char *ref,
+typedef int (*each_tag_name_fn)(const char *name, const char *ref,
 				const unsigned char *sha1);

-static int do_tag_names(const char **argv, func_tag fn)
+static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
 {
 	const char **p;
 	char ref[PATH_MAX];
@@ -191,7 +196,7 @@ static ssize_t do_sign(char *buffer, size_t size, size_t max)

 	if (!*signingkey) {
 		if (strlcpy(signingkey, git_committer_info(1),
-				sizeof(signingkey)) >= sizeof(signingkey))
+				sizeof(signingkey)) > sizeof(signingkey) - 1)
 			return error("committer info too long.");
 		bracket = strchr(signingkey, '>');
 		if (bracket)
@@ -254,7 +259,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 	unsigned long size;

 	type = sha1_object_info(object, NULL);
-	if (type <= 0)
+	if (type <= OBJ_NONE)
 	    die("bad object type.");

 	header_len = snprintf(header_buf, sizeof(header_buf),
@@ -267,7 +272,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 			  tag,
 			  git_committer_info(1));

-	if (header_len >= sizeof(header_buf))
+	if (header_len > sizeof(header_buf) - 1)
 		die("tag header too big.");

 	if (!message) {
@@ -362,6 +367,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			i++;
 			if (i == argc)
 				die("option -m needs an argument.");
+			if (message)
+				die("only one -F or -m option is allowed.");
 			message = xstrdup(argv[i]);
 			continue;
 		}
@@ -373,6 +380,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			i++;
 			if (i == argc)
 				die("option -F needs an argument.");
+			if (message)
+				die("only one -F or -m option is allowed.");

 			if (!strcmp(argv[i], "-"))
 				fd = 0;
@@ -401,15 +410,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 				die("argument to option -u too long");
 			continue;
 		}
-		if (!strcmp(arg, "-l")) {
+		if (!strcmp(arg, "-l"))
 			return list_tags(argv[i + 1], lines);
-		}
-		if (!strcmp(arg, "-d")) {
-			return do_tag_names(argv + i + 1, delete_tag);
-		}
-		if (!strcmp(arg, "-v")) {
-			return do_tag_names(argv + i + 1, verify_tag);
-		}
+		if (!strcmp(arg, "-d"))
+			return for_each_tag_name(argv + i + 1, delete_tag);
+		if (!strcmp(arg, "-v"))
+			return for_each_tag_name(argv + i + 1, verify_tag);
 		usage(builtin_tag_usage);
 	}

@@ -427,7 +433,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die("Failed to resolve '%s' as a valid ref.", object_ref);

-	if (snprintf(ref, sizeof(ref), "refs/tags/%s", tag) >= sizeof(ref))
+	if (snprintf(ref, sizeof(ref), "refs/tags/%s", tag) > sizeof(ref) - 1)
 		die("tag name too long: %.*s...", 50, tag);
 	if (check_ref_format(ref))
 		die("'%s' is not a valid tag name.", tag);
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index a0be164..c4fa446 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -332,6 +332,33 @@ test_expect_success 'creating an annotated tag with -F - should succeed' '
 	git diff expect actual
 '

+test_expect_success \
+	'trying to create a tag with a non-existing -F file should fail' '
+	! test -f nonexistingfile &&
+	! tag_exists notag &&
+	! git-tag -F nonexistingfile notag &&
+	! tag_exists notag
+'
+
+test_expect_success \
+	'trying to create tags giving many -m or -F options should fail' '
+	echo "message file 1" >msgfile1 &&
+	echo "message file 2" >msgfile2 &&
+	! tag_exists msgtag &&
+	! git-tag -m "message 1" -m "message 2" msgtag &&
+	! tag_exists msgtag &&
+	! git-tag -F msgfile1 -F msgfile2 msgtag &&
+	! tag_exists msgtag &&
+	! git-tag -m "message 1" -F msgfile1 msgtag &&
+	! tag_exists msgtag &&
+	! git-tag -F msgfile1 -m "message 1" msgtag &&
+	! tag_exists msgtag &&
+	! git-tag -F msgfile1 -m "message 1" -F msgfile2 msgtag &&
+	! tag_exists msgtag &&
+	! git-tag -m "message 1" -F msgfile1 -m "message 2" msgtag &&
+	! tag_exists msgtag
+'
+
 # blank and empty messages:

 get_tag_header empty-annotated-tag $commit commit $time >expect
@@ -648,6 +675,14 @@ test_expect_success 'creating a signed tag with -F - should succeed' '
 	git diff expect actual
 '

+test_expect_success \
+	'trying to create a signed tag with non-existing -F file should fail' '
+	! test -f nonexistingfile &&
+	! tag_exists nosigtag &&
+	! git-tag -s -F nonexistingfile nosigtag &&
+	! tag_exists nosigtag
+'
+
 test_expect_success 'verifying a signed tag should succeed' \
 	'git-tag -v signed-tag'

-- 
1.5.0

Re: [PATCH] builtin-tag.c: Fix two memory leaks and minor notation changes.

[Carlos Rica]

Carlos Rica escribió:
>   Those were commented in the responses to my previous patch:
>   http://article.gmane.org/gmane.comp.version-control.git/53138

A little error here when copying the link, the thread is:
http://thread.gmane.org/gmane.comp.version-control.git/53036

Re: [PATCH] builtin-tag.c: Fix two memory leaks and minor notation changes.

[Johannes Schindelin]

Hi,

On Sat, 21 Jul 2007, Carlos Rica wrote:

> A repeated call to read_sha1_file was not freing memory
> when the buffer was allocated but returned size was zero.
> 
> Also, now the program does not allow many -F or -m options,
> which was a bug too because it was not freing the memory
> allocated for any previous -F or -m options.
> 
> Tests are provided for ensuring that only one option
> -F or -m is given. Also, another test is shipped here,
> to check that "git tag" fails when a non-existing file
> is passed to the -F option, something that git-tag.sh
> allowed creating the tag with an empty message.
> 
> Signed-off-by: Carlos Rica <jasampler@gmail.com>

Thank you very much!

Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

Ciao,
Dscho