threads / patch / 13448

patch, 5 partsgit-tag: call verify_tag_buffer to validate the generated tag object

Subject: [PATCH 5/5] git-tag: call verify_tag_buffer to validate the generated tag object

## tl;dr

13 messages between May 9, 2008 and May 12, 2008. Diffs are folded; open one to read it.

replies: 12people: 3as markdown or json

drafnel@gmail.com· May 9, 2008, 02:19 UTC · lore

[PATCH 1/5] mktag.c: adjust verify_tag parameters

From: Brandon Casey <casey@nrlssc.navy.mil>
The size parameter should be a size_t since it is a string length.

There is no reason that the buffer argument should not be constant except for the nul termination that is performed in the first few lines of this function. This is not necessary, since a valid c string must always be nul terminated and we can check whether we have exceeded the caller's size parameter at the end of parsing the buffer.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 mktag.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)
Show changes to mktag.c +8 −3
diff --git a/mktag.c b/mktag.c
index 0b34341..5489ad6 100644
--- a/mktag.c
+++ b/mktag.c
@@ -39,7 +39,7 @@ static int verify_object(unsigned char *sha1, const char *expected_type)
 #define PD_FMT "%td"
 #endif
 
-static int verify_tag(char *buffer, unsigned long size)
+static int verify_tag(const char *buffer, size_t size)
 {
 	int typelen;
 	char type[20];
@@ -50,8 +50,6 @@ static int verify_tag(char *buffer, unsigned long size)
 	if (size < 84)
 		return error("wanna fool me ? you obviously got the size wrong !");
 
-	buffer[size] = 0;
-
 	/* Verify object line */
 	object = buffer;
 	if (memcmp(object, "object ", 7))
@@ -145,6 +143,13 @@ static int verify_tag(char *buffer, unsigned long size)
 		return error("char" PD_FMT ": trailing garbage in tag header",
 			tagger_line - buffer);
 
+	/*
+	 * Make sure we haven't advanced past what the caller said the
+	 * buffer size was.
+	 */
+	if (tagger_line - buffer >= size)
+		return error("char" PD_FMT ": tag truncated", size);
+
 	/* The actual stuff afterwards we don't care about.. */
 	return 0;
 }
-- 
1.5.5.67.g9a49
Junio C Hamano· May 11, 2008, 18:39 UTC · re: drafnel@gmail.com · lore

Re: [PATCH 1/5] mktag.c: adjust verify_tag parameters

drafnel@gmail.com writes:
> From: Brandon Casey <casey@nrlssc.navy.mil>
>
> The size parameter should be a size_t since it is a string length.
Correct.
Show 5 quoted lines
> There is no reason that the buffer argument should not be constant except
> for the nul termination that is performed in the first few lines of this
> function. This is not necessary, since a valid c string must always be
> nul terminated and we can check whether we have exceeded the caller's
> size parameter at the end of parsing the buffer.

Wait a minute. The point of passing a stringlet as a tuple of <pointer to the beginning, length> is that you may not have a valid C string to begin with, isn't it? We shouldn't be assuming that reading past the given size is Ok --- you may not be lucky enough to have any NUL byte after the given string before you hit the page boundary and encounter unmapped page.

The generic-looking argument you made is bogus, but for this particular code it is true, as the parameter you are passing to the function is prepared by strbuf_read() which gives you a NUL terminated buffer.

So the code is correct -- justification is not.
Show 43 quoted lines
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>  mktag.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/mktag.c b/mktag.c
> index 0b34341..5489ad6 100644
> --- a/mktag.c
> +++ b/mktag.c
> @@ -39,7 +39,7 @@ static int verify_object(unsigned char *sha1, const char *expected_type)
>  #define PD_FMT "%td"
>  #endif
>  
> -static int verify_tag(char *buffer, unsigned long size)
> +static int verify_tag(const char *buffer, size_t size)
>  {
>  	int typelen;
>  	char type[20];
> @@ -50,8 +50,6 @@ static int verify_tag(char *buffer, unsigned long size)
>  	if (size < 84)
>  		return error("wanna fool me ? you obviously got the size wrong !");
>  
> -	buffer[size] = 0;
> -
>  	/* Verify object line */
>  	object = buffer;
>  	if (memcmp(object, "object ", 7))
> @@ -145,6 +143,13 @@ static int verify_tag(char *buffer, unsigned long size)
>  		return error("char" PD_FMT ": trailing garbage in tag header",
>  			tagger_line - buffer);
>  
> +	/*
> +	 * Make sure we haven't advanced past what the caller said the
> +	 * buffer size was.
> +	 */
> +	if (tagger_line - buffer >= size)
> +		return error("char" PD_FMT ": tag truncated", size);
> +
>  	/* The actual stuff afterwards we don't care about.. */
>  	return 0;
>  }
> -- 
> 1.5.5.67.g9a49
Brandon Casey· May 12, 2008, 15:43 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/5] mktag.c: adjust verify_tag parameters

Junio C Hamano wrote:
Show 17 quoted lines
> drafnel@gmail.com writes:
> 
>> From: Brandon Casey <casey@nrlssc.navy.mil>
>>
>> The size parameter should be a size_t since it is a string length.
> 
> Correct.
> 
>> There is no reason that the buffer argument should not be constant except
>> for the nul termination that is performed in the first few lines of this
>> function. This is not necessary, since a valid c string must always be
>> nul terminated and we can check whether we have exceeded the caller's
>> size parameter at the end of parsing the buffer.
> 
> Wait a minute.  The point of passing a stringlet as a tuple of <pointer to
> the beginning, length> is that you may not have a valid C string to begin
> with, isn't it?
I thought it was just to limit the scope of parsing.

Other functions that deal with non-nul-terminated memory segments (like memchr) do not use char*, they instead use void*.

If we want to handle non-nul-terminated memory segments here, we should probably allocate a new string to copy the memory segment into so we can ensure it is nul-terminated. Otherwise we can't use any of the str*() functions safely.

-brandon
drafnel@gmail.com· May 9, 2008, 02:19 UTC · lore

[PATCH 2/5] Make mktag a builtin.

From: Brandon Casey <drafnel@gmail.com>
Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 Makefile  |    3 ++-
 builtin.h |    1 +
 git.c     |    1 +
 mktag.c   |    4 +---
 4 files changed, 5 insertions(+), 4 deletions(-)
Show changes to 4 files +5 −4

Makefile, builtin.h, git.c, mktag.c

diff --git a/Makefile b/Makefile
index 9d84c8d..1128cef 100644
--- a/Makefile
+++ b/Makefile
@@ -282,7 +282,6 @@ PROGRAMS += git-imap-send$X
 PROGRAMS += git-index-pack$X
 PROGRAMS += git-merge-index$X
 PROGRAMS += git-merge-tree$X
-PROGRAMS += git-mktag$X
 PROGRAMS += git-mktree$X
 PROGRAMS += git-pack-redundant$X
 PROGRAMS += git-patch-id$X
@@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
 BUILT_INS += git-get-tar-commit-id$X
 BUILT_INS += git-init$X
 BUILT_INS += git-merge-subtree$X
+BUILT_INS += git-mktag$X
 BUILT_INS += git-peek-remote$X
 BUILT_INS += git-repo-config$X
 BUILT_INS += git-show$X
@@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
 LIB_OBJS += mailmap.o
 LIB_OBJS += match-trees.o
 LIB_OBJS += merge-file.o
+LIB_OBJS += mktag.o
 LIB_OBJS += object.o
 LIB_OBJS += pack-check.o
 LIB_OBJS += pack-revindex.o
diff --git a/builtin.h b/builtin.h
index 95126fd..a8d3a11 100644
--- a/builtin.h
+++ b/builtin.h
@@ -58,6 +58,7 @@ extern int cmd_merge_base(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_ours(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_file(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
+extern int cmd_mktag(int argc, const char **argv, const char *prefix);
 extern int cmd_mv(int argc, const char **argv, const char *prefix);
 extern int cmd_name_rev(int argc, const char **argv, const char *prefix);
 extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index 89b431f..81aa31c 100644
--- a/git.c
+++ b/git.c
@@ -325,6 +325,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "merge-ours", cmd_merge_ours, RUN_SETUP },
 		{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
 		{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
+		{ "mktag", cmd_mktag, RUN_SETUP },
 		{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
 		{ "name-rev", cmd_name_rev, RUN_SETUP },
 		{ "pack-objects", cmd_pack_objects, RUN_SETUP },
diff --git a/mktag.c b/mktag.c
index 5489ad6..352747b 100644
--- a/mktag.c
+++ b/mktag.c
@@ -156,7 +156,7 @@ static int verify_tag(const char *buffer, size_t size)
 
 #undef PD_FMT
 
-int main(int argc, char **argv)
+int cmd_mktag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf;
 	unsigned char result_sha1[20];
@@ -164,8 +164,6 @@ int main(int argc, char **argv)
 	if (argc != 1)
 		usage("git-mktag < signaturefile");
 
-	setup_git_directory();
-
 	strbuf_init(&buf, 0);
 	if (strbuf_read(&buf, 0, 4096) < 0) {
 		die("could not read from stdin");
-- 
1.5.5.67.g9a49
Junio C Hamano· May 11, 2008, 17:28 UTC · re: drafnel@gmail.com · lore

Re: [PATCH 2/5] Make mktag a builtin.

drafnel@gmail.com writes:
> From: Brandon Casey <drafnel@gmail.com>
>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>
Show 13 quoted lines
> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
>  BUILT_INS += git-get-tar-commit-id$X
>  BUILT_INS += git-init$X
>  BUILT_INS += git-merge-subtree$X
> +BUILT_INS += git-mktag$X
>  BUILT_INS += git-peek-remote$X
>  BUILT_INS += git-repo-config$X
>  BUILT_INS += git-show$X
> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
>  LIB_OBJS += mailmap.o
>  LIB_OBJS += match-trees.o
>  LIB_OBJS += merge-file.o
> +LIB_OBJS += mktag.o
This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?
Junio C Hamano· May 11, 2008, 17:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/5] Make mktag a builtin.

Junio C Hamano <junio@pobox.com> writes:
Show 21 quoted lines
> drafnel@gmail.com writes:
>
>> From: Brandon Casey <drafnel@gmail.com>
>>
>> Signed-off-by: Brandon Casey <drafnel@gmail.com>
>
>> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
>>  BUILT_INS += git-get-tar-commit-id$X
>>  BUILT_INS += git-init$X
>>  BUILT_INS += git-merge-subtree$X
>> +BUILT_INS += git-mktag$X
>>  BUILT_INS += git-peek-remote$X
>>  BUILT_INS += git-repo-config$X
>>  BUILT_INS += git-show$X
>> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
>>  LIB_OBJS += mailmap.o
>>  LIB_OBJS += match-trees.o
>>  LIB_OBJS += merge-file.o
>> +LIB_OBJS += mktag.o
>
> This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?
That is, something along this line, instead.
-- >8 --
From: Brandon Casey <drafnel@gmail.com>
Date: Thu, 8 May 2008 21:19:46 -0500
Subject: [PATCH] Make mktag a builtin
Signed-off-by: Brandon Casey <drafnel@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile                   |    2 +-
 mktag.c => builtin-mktag.c |    4 +---
 builtin.h                  |    1 +
 git.c                      |    1 +
 4 files changed, 4 insertions(+), 4 deletions(-)
 rename mktag.c => builtin-mktag.c (98%)
Show changes to 5 files +4 −4

Makefile, mktag.c, builtin-mktag.c, builtin.h, git.c

diff --git a/Makefile b/Makefile
index 9d84c8d..3980dc9 100644
--- a/Makefile
+++ b/Makefile
@@ -282,7 +282,6 @@ PROGRAMS += git-imap-send$X
 PROGRAMS += git-index-pack$X
 PROGRAMS += git-merge-index$X
 PROGRAMS += git-merge-tree$X
-PROGRAMS += git-mktag$X
 PROGRAMS += git-mktree$X
 PROGRAMS += git-pack-redundant$X
 PROGRAMS += git-patch-id$X
@@ -510,6 +509,7 @@ BUILTIN_OBJS += builtin-merge-base.o
 BUILTIN_OBJS += builtin-merge-file.o
 BUILTIN_OBJS += builtin-merge-ours.o
 BUILTIN_OBJS += builtin-merge-recursive.o
+BUILTIN_OBJS += builtin-mktag.o
 BUILTIN_OBJS += builtin-mv.o
 BUILTIN_OBJS += builtin-name-rev.o
 BUILTIN_OBJS += builtin-pack-objects.o
diff --git a/mktag.c b/builtin-mktag.c
similarity index 98%
rename from mktag.c
rename to builtin-mktag.c
index a45a5e7..1596900 100644
--- a/mktag.c
+++ b/builtin-mktag.c
@@ -149,7 +149,7 @@ static int verify_tag(const char *buffer, size_t size)
 
 #undef PD_FMT
 
-int main(int argc, char **argv)
+int cmd_mktag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf;
 	unsigned char result_sha1[20];
@@ -157,8 +157,6 @@ int main(int argc, char **argv)
 	if (argc != 1)
 		usage("git-mktag < signaturefile");
 
-	setup_git_directory();
-
 	strbuf_init(&buf, 0);
 	if (strbuf_read(&buf, 0, 4096) < 0) {
 		die("could not read from stdin");
diff --git a/builtin.h b/builtin.h
index 95126fd..a8d3a11 100644
--- a/builtin.h
+++ b/builtin.h
@@ -58,6 +58,7 @@ extern int cmd_merge_base(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_ours(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_file(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
+extern int cmd_mktag(int argc, const char **argv, const char *prefix);
 extern int cmd_mv(int argc, const char **argv, const char *prefix);
 extern int cmd_name_rev(int argc, const char **argv, const char *prefix);
 extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index 89b431f..81aa31c 100644
--- a/git.c
+++ b/git.c
@@ -325,6 +325,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "merge-ours", cmd_merge_ours, RUN_SETUP },
 		{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
 		{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
+		{ "mktag", cmd_mktag, RUN_SETUP },
 		{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
 		{ "name-rev", cmd_name_rev, RUN_SETUP },
 		{ "pack-objects", cmd_pack_objects, RUN_SETUP },
-- 
1.5.5.1.295.g4c42f
Brandon Casey· May 12, 2008, 15:09 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/5] Make mktag a builtin.

Junio C Hamano wrote:
Show 21 quoted lines
> drafnel@gmail.com writes:
> 
>> From: Brandon Casey <drafnel@gmail.com>
>>
>> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> 
>> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
>>  BUILT_INS += git-get-tar-commit-id$X
>>  BUILT_INS += git-init$X
>>  BUILT_INS += git-merge-subtree$X
>> +BUILT_INS += git-mktag$X
>>  BUILT_INS += git-peek-remote$X
>>  BUILT_INS += git-repo-config$X
>>  BUILT_INS += git-show$X
>> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
>>  LIB_OBJS += mailmap.o
>>  LIB_OBJS += match-trees.o
>>  LIB_OBJS += merge-file.o
>> +LIB_OBJS += mktag.o
> 
> This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?
I didn't know how to do it.

I was trying not to do a code move and a code change at the same time. I didn't think I should move the non-builtin mktag.c to builtin-mktag.c, and then after I modified mktag to be a builtin I knew I was moving it to builtin-tag.c so I didn't see a point to renaming it.

Also, I decided about those things _before_ I realized how small the changes would be to mktag to make it a builtin.

Do you think the modified patch you posted conflicts with the idea that "code move should be separate from code change"?

-brandon
Johannes Schindelin· May 12, 2008, 17:04 UTC · re: Brandon Casey · lore

Re: [PATCH 2/5] Make mktag a builtin.

Hi,
On Mon, 12 May 2008, Brandon Casey wrote:
Show 26 quoted lines
> Junio C Hamano wrote:
> > drafnel@gmail.com writes:
> > 
> >> From: Brandon Casey <drafnel@gmail.com>
> >>
> >> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> > 
> >> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
> >>  BUILT_INS += git-get-tar-commit-id$X
> >>  BUILT_INS += git-init$X
> >>  BUILT_INS += git-merge-subtree$X
> >> +BUILT_INS += git-mktag$X
> >>  BUILT_INS += git-peek-remote$X
> >>  BUILT_INS += git-repo-config$X
> >>  BUILT_INS += git-show$X
> >> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
> >>  LIB_OBJS += mailmap.o
> >>  LIB_OBJS += match-trees.o
> >>  LIB_OBJS += merge-file.o
> >> +LIB_OBJS += mktag.o
> > 
> > This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?
> 
> I didn't know how to do it.
> 
> I was trying not to do a code move and a code change at the same time.
Why did you not consult Git's own history for guidance?  See e.g.
	$ git log next --diff-filter=A builtin-*.c

Ciao, Dscho

Brandon Casey· May 12, 2008, 17:32 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/5] Make mktag a builtin.

Johannes Schindelin wrote:
Show 29 quoted lines
> Hi,
> 
> On Mon, 12 May 2008, Brandon Casey wrote:
> 
>> Junio C Hamano wrote:
>>> drafnel@gmail.com writes:
>>>
>>>> From: Brandon Casey <drafnel@gmail.com>
>>>>
>>>> Signed-off-by: Brandon Casey <drafnel@gmail.com>
>>>> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
>>>>  BUILT_INS += git-get-tar-commit-id$X
>>>>  BUILT_INS += git-init$X
>>>>  BUILT_INS += git-merge-subtree$X
>>>> +BUILT_INS += git-mktag$X
>>>>  BUILT_INS += git-peek-remote$X
>>>>  BUILT_INS += git-repo-config$X
>>>>  BUILT_INS += git-show$X
>>>> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
>>>>  LIB_OBJS += mailmap.o
>>>>  LIB_OBJS += match-trees.o
>>>>  LIB_OBJS += merge-file.o
>>>> +LIB_OBJS += mktag.o
>>> This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?
>> I didn't know how to do it.
>>
>> I was trying not to do a code move and a code change at the same time.
> 
> Why did you not consult Git's own history for guidance?  See e.g.

I did, but not exhaustively. I found only examples of converting a shell/perl script to a c version. That is straight-forward.

> 	$ git log next --diff-filter=A builtin-*.c

And indeed the first two results here are examples of converting scripts to builtin c versions.

The third result is applicable and converts merge-recursive from a standalone c version to builtin.

If I had known about (or looked for) the --diff-filter option I would have used the 'R' argument, since I am interested in a rename event, but it produces no results.

	$ git log next --diff-filter=R builtin-*.c
Adding -M does not help.
-brandon
Junio C Hamano· May 12, 2008, 18:41 UTC · re: Brandon Casey · lore

Re: [PATCH 2/5] Make mktag a builtin.

Brandon Casey <casey@nrlssc.navy.mil> writes:
> I didn't think I should move the non-builtin mktag.c to builtin-mktag.c,
> and then after I modified mktag to be a builtin I knew I was moving it
> to builtin-tag.c so I didn't see a point to renaming it.

I see. I did not realize that the eventual shape would be to have both mktag and tag to be in builtin-tag.c, just like builtin-log.c supports many other commands from the log family, and that was where my question came from.

But I think the arrangement to have both in builtin-tag.c actually makes sense --- mktag needs to be kept supported but most of its internal should be shared with tag anyway.

Show 5 quoted lines
> Also, I decided about those things _before_ I realized how small the changes
> would be to mktag to make it a builtin.
>
> Do you think the modified patch you posted conflicts with the idea that
> "code move should be separate from code change"?

Yes, it does, but I do not subscribe to the "idea". Therefor I do not see any problem.

If you were to stop at making mktag a builtin, then the patch I sent would be the change that is necessary to do so. A code movement can and often does need some adjustment (e.g. if you move "a.c" to "src/a.c", its '#include "a.h"' may need to become '#include "../a.h"' (or preferably to '#include <a.h>' with appropriate -I.. option in the Makefile). It does not help anybody to insist on a blanket dogma that forbids modification and movement at the same time.

We do discourage rolling unrelated things in one commit, but creating a builtin "foo" typically involves creation of builtin-foo.c and associated changes to the Makefile and builtin.h, and in this case the initial contents of builtin-mktag.c happens to come from an existing file mktag.c, while making the original mktag.c obsolete and unnecessary along the way. That is a single logical change, and I do not think there is anything wrong to do it in one commit. In fact, splitting such a change into more than one commit is just plain silly and wrong, isn't it?

drafnel@gmail.com· May 9, 2008, 02:19 UTC · lore

[PATCH 3/5] mktag.c: rename verify_tag to verify_tag_buffer

From: Brandon Casey <drafnel@gmail.com>

This is in preparation for merging mktag.c with builtin-tag.c and using verify_tag_buffer within the git-tag code path.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 mktag.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to mktag.c +2 −2
diff --git a/mktag.c b/mktag.c
index 352747b..6204001 100644
--- a/mktag.c
+++ b/mktag.c
@@ -39,7 +39,7 @@ static int verify_object(unsigned char *sha1, const char *expected_type)
 #define PD_FMT "%td"
 #endif
 
-static int verify_tag(const char *buffer, size_t size)
+static int verify_tag_buffer(const char *buffer, size_t size)
 {
 	int typelen;
 	char type[20];
@@ -171,7 +171,7 @@ int cmd_mktag(int argc, const char **argv, const char *prefix)
 
 	/* Verify it for some basic sanity: it needs to start with
 	   "object <sha1>\ntype\ntagger " */
-	if (verify_tag(buf.buf, buf.len) < 0)
+	if (verify_tag_buffer(buf.buf, buf.len) < 0)
 		die("invalid tag signature file");
 
 	if (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)
-- 
1.5.5.67.g9a49
drafnel@gmail.com· May 9, 2008, 02:19 UTC · lore

[PATCH 4/5] mktag.c: consolidate tag functions by merging mktag.c into builtin-tag.c

From: Brandon Casey <drafnel@gmail.com>
Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 Makefile      |    1 -
 builtin-tag.c |  181 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 mktag.c       |  183 ---------------------------------------------------------
 3 files changed, 181 insertions(+), 184 deletions(-)
 delete mode 100644 mktag.c
Show changes to 3 files +181 −184

Makefile, builtin-tag.c, mktag.c

diff --git a/Makefile b/Makefile
index 1128cef..572459f 100644
--- a/Makefile
+++ b/Makefile
@@ -423,7 +423,6 @@ LIB_OBJS += log-tree.o
 LIB_OBJS += mailmap.o
 LIB_OBJS += match-trees.o
 LIB_OBJS += merge-file.o
-LIB_OBJS += mktag.o
 LIB_OBJS += object.o
 LIB_OBJS += pack-check.o
 LIB_OBJS += pack-revindex.o
diff --git a/builtin-tag.c b/builtin-tag.c
index 129ff57..85ec2f3 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -296,6 +296,161 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 	free(buf);
 }
 
+/*
+ * We refuse to tag something we can't verify. Just because.
+ */
+static int verify_object(unsigned char *sha1, const char *expected_type)
+{
+	int ret = -1;
+	enum object_type type;
+	unsigned long size;
+	void *buffer = read_sha1_file(sha1, &type, &size);
+
+	if (buffer) {
+		if (type == type_from_string(expected_type))
+			ret = check_sha1_signature(sha1, buffer, size, expected_type);
+		free(buffer);
+	}
+	return ret;
+}
+
+#ifdef NO_C99_FORMAT
+#define PD_FMT "%d"
+#else
+#define PD_FMT "%td"
+#endif
+
+/*
+ * A signature file has a very simple fixed format: four lines
+ * of "object <sha1>" + "type <typename>" + "tag <tagname>" +
+ * "tagger <committer>", followed by a blank line, a free-form tag
+ * message and a signature block that git itself doesn't care about,
+ * but that can be verified with gpg or similar.
+ *
+ * The first four lines are guaranteed to be at least 83 bytes:
+ * "object <sha1>\n" is 48 bytes, "type tag\n" at 9 bytes is the
+ * shortest possible type-line, "tag .\n" at 6 bytes is the shortest
+ * single-character-tag line, and "tagger . <> 0 +0000\n" at 20 bytes is
+ * the shortest possible tagger-line.
+ */
+
+static int verify_tag_buffer(const char *buffer, size_t size)
+{
+	int typelen;
+	char type[20];
+	unsigned char sha1[20];
+	const char *object, *type_line, *tag_line, *tagger_line, *lb, *rb;
+	size_t len;
+
+	if (size < 84)
+		return error("wanna fool me ? you obviously got the size wrong !");
+
+	/* Verify object line */
+	object = buffer;
+	if (memcmp(object, "object ", 7))
+		return error("char%d: does not start with \"object \"", 0);
+
+	if (get_sha1_hex(object + 7, sha1))
+		return error("char%d: could not get SHA1 hash", 7);
+
+	/* Verify type line */
+	type_line = object + 48;
+	if (memcmp(type_line - 1, "\ntype ", 6))
+		return error("char%d: could not find \"\\ntype \"", 47);
+
+	/* Verify tag-line */
+	tag_line = strchr(type_line, '\n');
+	if (!tag_line)
+		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - buffer);
+	tag_line++;
+	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
+		return error("char" PD_FMT ": no \"tag \" found", tag_line - buffer);
+
+	/* Get the actual type */
+	typelen = tag_line - type_line - strlen("type \n");
+	if (typelen >= sizeof(type))
+		return error("char" PD_FMT ": type too long", type_line+5 - buffer);
+
+	memcpy(type, type_line+5, typelen);
+	type[typelen] = 0;
+
+	/* Verify that the object matches */
+	if (verify_object(sha1, type))
+		return error("char%d: could not verify object %s", 7, sha1_to_hex(sha1));
+
+	/* Verify the tag-name: we don't allow control characters or spaces in it */
+	tag_line += 4;
+	for (;;) {
+		unsigned char c = *tag_line++;
+		if (c == '\n')
+			break;
+		if (c > ' ')
+			continue;
+		return error("char" PD_FMT ": could not verify tag name", tag_line - buffer);
+	}
+
+	/* Verify the tagger line */
+	tagger_line = tag_line;
+
+	if (memcmp(tagger_line, "tagger ", 7))
+		return error("char" PD_FMT ": could not find \"tagger \"",
+			tagger_line - buffer);
+
+	/*
+	 * Check for correct form for name and email
+	 * i.e. " <" followed by "> " on _this_ line
+	 * No angle brackets within the name or email address fields.
+	 * No spaces within the email address field.
+	 */
+	tagger_line += 7;
+	if (!(lb = strstr(tagger_line, " <")) || !(rb = strstr(lb+2, "> ")) ||
+		strpbrk(tagger_line, "<>\n") != lb+1 ||
+		strpbrk(lb+2, "><\n ") != rb)
+		return error("char" PD_FMT ": malformed tagger field",
+			tagger_line - buffer);
+
+	/* Check for author name, at least one character, space is acceptable */
+	if (lb == tagger_line)
+		return error("char" PD_FMT ": missing tagger name",
+			tagger_line - buffer);
+
+	/* timestamp, 1 or more digits followed by space */
+	tagger_line = rb + 2;
+	if (!(len = strspn(tagger_line, "0123456789")))
+		return error("char" PD_FMT ": missing tag timestamp",
+			tagger_line - buffer);
+	tagger_line += len;
+	if (*tagger_line != ' ')
+		return error("char" PD_FMT ": malformed tag timestamp",
+			tagger_line - buffer);
+	tagger_line++;
+
+	/* timezone, 5 digits [+-]hhmm, max. 1400 */
+	if (!((tagger_line[0] == '+' || tagger_line[0] == '-') &&
+	      strspn(tagger_line+1, "0123456789") == 4 &&
+	      tagger_line[5] == '\n' && atoi(tagger_line+1) <= 1400))
+		return error("char" PD_FMT ": malformed tag timezone",
+			tagger_line - buffer);
+	tagger_line += 6;
+
+	/* Verify the blank line separating the header from the body */
+	if (*tagger_line != '\n')
+		return error("char" PD_FMT ": trailing garbage in tag header",
+			tagger_line - buffer);
+
+	/*
+	 * Make sure we haven't advanced past what the caller said the
+	 * buffer size was.
+	 */
+	if (tagger_line - buffer >= size)
+		return error("char" PD_FMT ": tag truncated", size);
+
+	/* The actual stuff afterwards we don't care about.. */
+	return 0;
+}
+
+#undef PD_FMT
+
 static void create_tag(const unsigned char *object, const char *tag,
 		       struct strbuf *buf, int message, int sign,
 		       unsigned char *prev, unsigned char *result)
@@ -482,3 +637,29 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	strbuf_release(&buf);
 	return 0;
 }
+
+int cmd_mktag(int argc, const char **argv, const char *prefix)
+{
+	struct strbuf buf;
+	unsigned char result_sha1[20];
+
+	if (argc != 1)
+		usage("git-mktag < signaturefile");
+
+	strbuf_init(&buf, 0);
+	if (strbuf_read(&buf, 0, 4096) < 0) {
+		die("could not read from stdin");
+	}
+
+	/* Verify it for some basic sanity: it needs to start with
+	   "object <sha1>\ntype\ntagger " */
+	if (verify_tag_buffer(buf.buf, buf.len) < 0)
+		die("invalid tag signature file");
+
+	if (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)
+		die("unable to write tag file");
+
+	strbuf_release(&buf);
+	printf("%s\n", sha1_to_hex(result_sha1));
+	return 0;
+}
diff --git a/mktag.c b/mktag.c
deleted file mode 100644
index 6204001..0000000
--- a/mktag.c
+++ /dev/null
@@ -1,183 +0,0 @@
-#include "cache.h"
-#include "tag.h"
-
-/*
- * A signature file has a very simple fixed format: four lines
- * of "object <sha1>" + "type <typename>" + "tag <tagname>" +
- * "tagger <committer>", followed by a blank line, a free-form tag
- * message and a signature block that git itself doesn't care about,
- * but that can be verified with gpg or similar.
- *
- * The first four lines are guaranteed to be at least 83 bytes:
- * "object <sha1>\n" is 48 bytes, "type tag\n" at 9 bytes is the
- * shortest possible type-line, "tag .\n" at 6 bytes is the shortest
- * single-character-tag line, and "tagger . <> 0 +0000\n" at 20 bytes is
- * the shortest possible tagger-line.
- */
-
-/*
- * We refuse to tag something we can't verify. Just because.
- */
-static int verify_object(unsigned char *sha1, const char *expected_type)
-{
-	int ret = -1;
-	enum object_type type;
-	unsigned long size;
-	void *buffer = read_sha1_file(sha1, &type, &size);
-
-	if (buffer) {
-		if (type == type_from_string(expected_type))
-			ret = check_sha1_signature(sha1, buffer, size, expected_type);
-		free(buffer);
-	}
-	return ret;
-}
-
-#ifdef NO_C99_FORMAT
-#define PD_FMT "%d"
-#else
-#define PD_FMT "%td"
-#endif
-
-static int verify_tag_buffer(const char *buffer, size_t size)
-{
-	int typelen;
-	char type[20];
-	unsigned char sha1[20];
-	const char *object, *type_line, *tag_line, *tagger_line, *lb, *rb;
-	size_t len;
-
-	if (size < 84)
-		return error("wanna fool me ? you obviously got the size wrong !");
-
-	/* Verify object line */
-	object = buffer;
-	if (memcmp(object, "object ", 7))
-		return error("char%d: does not start with \"object \"", 0);
-
-	if (get_sha1_hex(object + 7, sha1))
-		return error("char%d: could not get SHA1 hash", 7);
-
-	/* Verify type line */
-	type_line = object + 48;
-	if (memcmp(type_line - 1, "\ntype ", 6))
-		return error("char%d: could not find \"\\ntype \"", 47);
-
-	/* Verify tag-line */
-	tag_line = strchr(type_line, '\n');
-	if (!tag_line)
-		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - buffer);
-	tag_line++;
-	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
-		return error("char" PD_FMT ": no \"tag \" found", tag_line - buffer);
-
-	/* Get the actual type */
-	typelen = tag_line - type_line - strlen("type \n");
-	if (typelen >= sizeof(type))
-		return error("char" PD_FMT ": type too long", type_line+5 - buffer);
-
-	memcpy(type, type_line+5, typelen);
-	type[typelen] = 0;
-
-	/* Verify that the object matches */
-	if (verify_object(sha1, type))
-		return error("char%d: could not verify object %s", 7, sha1_to_hex(sha1));
-
-	/* Verify the tag-name: we don't allow control characters or spaces in it */
-	tag_line += 4;
-	for (;;) {
-		unsigned char c = *tag_line++;
-		if (c == '\n')
-			break;
-		if (c > ' ')
-			continue;
-		return error("char" PD_FMT ": could not verify tag name", tag_line - buffer);
-	}
-
-	/* Verify the tagger line */
-	tagger_line = tag_line;
-
-	if (memcmp(tagger_line, "tagger ", 7))
-		return error("char" PD_FMT ": could not find \"tagger \"",
-			tagger_line - buffer);
-
-	/*
-	 * Check for correct form for name and email
-	 * i.e. " <" followed by "> " on _this_ line
-	 * No angle brackets within the name or email address fields.
-	 * No spaces within the email address field.
-	 */
-	tagger_line += 7;
-	if (!(lb = strstr(tagger_line, " <")) || !(rb = strstr(lb+2, "> ")) ||
-		strpbrk(tagger_line, "<>\n") != lb+1 ||
-		strpbrk(lb+2, "><\n ") != rb)
-		return error("char" PD_FMT ": malformed tagger field",
-			tagger_line - buffer);
-
-	/* Check for author name, at least one character, space is acceptable */
-	if (lb == tagger_line)
-		return error("char" PD_FMT ": missing tagger name",
-			tagger_line - buffer);
-
-	/* timestamp, 1 or more digits followed by space */
-	tagger_line = rb + 2;
-	if (!(len = strspn(tagger_line, "0123456789")))
-		return error("char" PD_FMT ": missing tag timestamp",
-			tagger_line - buffer);
-	tagger_line += len;
-	if (*tagger_line != ' ')
-		return error("char" PD_FMT ": malformed tag timestamp",
-			tagger_line - buffer);
-	tagger_line++;
-
-	/* timezone, 5 digits [+-]hhmm, max. 1400 */
-	if (!((tagger_line[0] == '+' || tagger_line[0] == '-') &&
-	      strspn(tagger_line+1, "0123456789") == 4 &&
-	      tagger_line[5] == '\n' && atoi(tagger_line+1) <= 1400))
-		return error("char" PD_FMT ": malformed tag timezone",
-			tagger_line - buffer);
-	tagger_line += 6;
-
-	/* Verify the blank line separating the header from the body */
-	if (*tagger_line != '\n')
-		return error("char" PD_FMT ": trailing garbage in tag header",
-			tagger_line - buffer);
-
-	/*
-	 * Make sure we haven't advanced past what the caller said the
-	 * buffer size was.
-	 */
-	if (tagger_line - buffer >= size)
-		return error("char" PD_FMT ": tag truncated", size);
-
-	/* The actual stuff afterwards we don't care about.. */
-	return 0;
-}
-
-#undef PD_FMT
-
-int cmd_mktag(int argc, const char **argv, const char *prefix)
-{
-	struct strbuf buf;
-	unsigned char result_sha1[20];
-
-	if (argc != 1)
-		usage("git-mktag < signaturefile");
-
-	strbuf_init(&buf, 0);
-	if (strbuf_read(&buf, 0, 4096) < 0) {
-		die("could not read from stdin");
-	}
-
-	/* Verify it for some basic sanity: it needs to start with
-	   "object <sha1>\ntype\ntagger " */
-	if (verify_tag_buffer(buf.buf, buf.len) < 0)
-		die("invalid tag signature file");
-
-	if (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)
-		die("unable to write tag file");
-
-	strbuf_release(&buf);
-	printf("%s\n", sha1_to_hex(result_sha1));
-	return 0;
-}
-- 
1.5.5.67.g9a49
drafnel@gmail.com· May 9, 2008, 02:19 UTC · lore
From: Brandon Casey <drafnel@gmail.com>

This will ensure that both git-tag and git-mktag "produce valid tags, with the same definition of validity."

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 builtin-tag.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Show changes to builtin-tag.c +2 −0
diff --git a/builtin-tag.c b/builtin-tag.c
index 85ec2f3..0f79d47 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -508,6 +508,8 @@ static void create_tag(const unsigned char *object, const char *tag,
 
 	if (sign && do_sign(buf) < 0)
 		die("unable to sign the tag");
+	if (verify_tag_buffer(buf->buf, buf->len) < 0)
+		die("invalid tag object");
 	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
 		die("unable to write tag file");
 }
-- 
1.5.5.67.g9a49

← back to recent threads