[RFC/PATCH,12/12] mktag: bring back some of the verify_object() logic

Commit Message

Ævar Arnfjörð Bjarmason Nov. 26, 2020, 1:28 a.m. UTC

When working on this series I saw too late that I'd removed the mktag
check for validating the object the tag points to. The fsck_tag() code
doesn't do this because it's meant for the context of fsck, where
we're validating reachability anyway.

We'd need to either refactor fsck_tag() so that it can pass us back
its "tagged_oid" and the "type_from_string_gently()" value it throws
away to get rid of the re-parsing of stdin here, or just duplicate the
logic as I'm doing here.

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---
 builtin/mktag.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 t/t3800-mktag.sh |  4 ++--
 2 files changed, 50 insertions(+), 2 deletions(-)

Comments

Jeff King Nov. 26, 2020, 8:32 a.m. UTC | #1

On Thu, Nov 26, 2020 at 02:28:54AM +0100, Ævar Arnfjörð Bjarmason wrote:

> When working on this series I saw too late that I'd removed the mktag
> check for validating the object the tag points to. The fsck_tag() code
> doesn't do this because it's meant for the context of fsck, where
> we're validating reachability anyway.
> 
> We'd need to either refactor fsck_tag() so that it can pass us back
> its "tagged_oid" and the "type_from_string_gently()" value it throws
> away to get rid of the re-parsing of stdin here, or just duplicate the
> logic as I'm doing here.

We have yet another tag parser (because of course there's more): the one
in parse_tag_buffer() that we use for reading tags. I think your new
function here could just be:

  enum object_type real_type;
  struct tag t; /* yuck! another fake object */

  memset(&t, 0, sizeof(t));
  if (parse_tag_buffer(r, &t, buf, len) < 0)
	die("unable to parse");

  real_type = oid_object_info(r, &t->tagged->oid, NULL);
  if (real_type < 0)
	die("tagged object does not exist");
  if (real_type != t->tagged->type)
	die("tagged object's type does not match tag type field");

I almost wonder if we could simply rely on parse_tag_buffer() instead of
fsck_tag(), but it is not nearly as picky about finding potential
problems (its goal is the opposite: to return something usable if it's
at all possible to do so).

The "yuck" above isn't great. We could use pretend_object_file() for
that, though it's a bit heavy-weight (it computes the actual oid!). And
also it's a weird one-off that we've talked about getting rid of.

-Peff

Patch

diff --git a/builtin/mktag.c b/builtin/mktag.c
index e9a0954dcb..f1f1cf04ff 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -1,5 +1,6 @@ 
 #include "builtin.h"
 #include "tag.h"
+#include "replace-object.h"
 #include "object-store.h"
 #include "fsck.h"
 
@@ -25,6 +26,50 @@  static int mktag_fsck_error_func(struct fsck_options *o,
 	}
 }
 
+static int verify_object_in_tag(const char *stdin)
+{
+	struct object_id oid;
+	char *eol;
+	const char *p;
+	int expected_type_id;
+	const char *expected_type;
+	int ret = -1;
+	enum object_type type;
+	unsigned long size;
+	void *buffer;
+	const struct object_id *repl;
+
+	if (!skip_prefix(stdin, "object ", &stdin))
+		goto bug;
+	if (parse_oid_hex(stdin, &oid, &p) || *p != '\n')
+		goto bug;
+	stdin = p + 1;
+	if (!skip_prefix(stdin, "type ", &stdin))
+		goto bug;
+	eol = strchr(stdin, '\n');
+	expected_type_id = type_from_string_gently(stdin, eol - stdin, 1);
+	if (expected_type_id < 0)
+		goto bug;
+	expected_type = type_name(expected_type_id);
+
+	buffer = read_object_file(&oid, &type, &size);
+	repl = lookup_replace_object(the_repository, &oid);
+
+	if (buffer) {
+		if (type == type_from_string(expected_type)) {
+			ret = check_object_signature(the_repository, repl,
+						     buffer, size,
+						     expected_type);
+		}
+		free(buffer);
+	}
+	goto ok;
+bug:
+	BUG("fsck_object() should have ensured a sane tag format already!");
+ok:
+	return ret;
+}
+
 int cmd_mktag(int argc, const char **argv, const char *prefix)
 {
 	struct object obj;
@@ -49,6 +94,9 @@  int cmd_mktag(int argc, const char **argv, const char *prefix)
 	if (fsck_object(&obj, buf.buf, buf.len, &fsck_options))
 		die("tag on stdin did not pass our strict fsck check");
 
+	if (verify_object_in_tag(buf.buf))
+		die("tag on stdin did not refer to a valid object");
+
 	if (write_object_file(buf.buf, buf.len, tag_type, &result) < 0)
 		die("unable to write annotated tag object");
 
diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index bc57ee85c9..74cd2eb141 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -137,7 +137,7 @@  tagger . <> 0 +0000
 EOF
 
 check_verify_failure 'verify object (SHA1/type) check' \
-	'^error: char7: could not verify object.*$'
+	'^fatal: tag on stdin did not refer to a valid object'
 
 cat >tag.sig <<EOF
 object $head
@@ -148,7 +148,7 @@  tagger . <> 0 +0000
 EOF
 
 check_verify_failure 'verify object (SHA1/type) check' \
-	'^fatal: invalid object type'
+	'^error: badType:'
 
 cat >tag.sig <<EOF
 object $(test_oid deadbeef)