Re: [PATCH v3 06/20] builtin/receive-pack: convert portions to struct object_id

2017-07-15 Thread René Scharfe
Am 31.03.2017 um 03:39 schrieb brian m. carlson:
> @@ -1081,10 +1081,10 @@ static const char *update(struct command *cmd, struct 
> shallow_info *si)
>   return "hook declined";
>   }
>   
> - if (is_null_sha1(new_sha1)) {
> + if (is_null_oid(new_oid)) {
>   struct strbuf err = STRBUF_INIT;
> - if (!parse_object(old_sha1)) {
> - old_sha1 = NULL;
> + if (!parse_object(old_oid->hash)) {
> + old_oid = NULL;

So old_oid can become NULL...

>   if (ref_exists(name)) {
>   rp_warning("Allowing deletion of corrupt ref.");
>   } else {
> @@ -1094,7 +1094,7 @@ static const char *update(struct command *cmd, struct 
> shallow_info *si)
>   }
>   if (ref_transaction_delete(transaction,
>  namespaced_name,
> -old_sha1,
> +old_oid->hash,

... and here we dereference it.

-- >8 --
Subject: [PATCH] receive-pack: don't access hash of NULL object_id pointer

We set old_oid to NULL if we found out that it's a corrupt reference.
In that case don't try to access the hash member and pass NULL to
ref_transaction_delete() instead.

Found with Clang's UBSan.

Signed-off-by: Rene Scharfe 
---
That's the last bug of this kind which "make SANITIZE=undefined test"
turned up.

 builtin/receive-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index cabdc55e09..946cf55138 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1138,7 +1138,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
}
if (ref_transaction_delete(transaction,
   namespaced_name,
-  old_oid->hash,
+  old_oid ? old_oid->hash : NULL,
   0, "push", &err)) {
rp_error("%s", err.buf);
strbuf_release(&err);
-- 
2.13.3


[PATCH v3 06/20] builtin/receive-pack: convert portions to struct object_id

2017-03-30 Thread brian m. carlson
Convert some hardcoded constants into uses of parse_oid_hex.
Additionally, convert all uses of struct command, and miscellaneous
other functions necessary for that.  This work is necessary to be able
to convert sha1_array_append later on.

To avoid needing to specify a constant, reject shallow lines with the
wrong length instead of simply ignoring them.

Note that in queue_command we are guaranteed to have a NUL-terminated
buffer or at least one byte of overflow that we can safely read, so the
linelen check can be elided.  We would die in such a case, but not read
invalid memory.

Signed-off-by: brian m. carlson 
---
 builtin/receive-pack.c | 98 +-
 1 file changed, 49 insertions(+), 49 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index feafb076a4..62b7c5e25c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -309,8 +309,8 @@ struct command {
unsigned int skip_update:1,
 did_not_exist:1;
int index;
-   unsigned char old_sha1[20];
-   unsigned char new_sha1[20];
+   struct object_id old_oid;
+   struct object_id new_oid;
char ref_name[FLEX_ARRAY]; /* more */
 };
 
@@ -723,7 +723,7 @@ static int feed_receive_hook(void *state_, const char 
**bufp, size_t *sizep)
return -1; /* EOF */
strbuf_reset(&state->buf);
strbuf_addf(&state->buf, "%s %s %s\n",
-   sha1_to_hex(cmd->old_sha1), sha1_to_hex(cmd->new_sha1),
+   oid_to_hex(&cmd->old_oid), oid_to_hex(&cmd->new_oid),
cmd->ref_name);
state->cmd = cmd->next;
if (bufp) {
@@ -764,8 +764,8 @@ static int run_update_hook(struct command *cmd)
return 0;
 
argv[1] = cmd->ref_name;
-   argv[2] = sha1_to_hex(cmd->old_sha1);
-   argv[3] = sha1_to_hex(cmd->new_sha1);
+   argv[2] = oid_to_hex(&cmd->old_oid);
+   argv[3] = oid_to_hex(&cmd->new_oid);
argv[4] = NULL;
 
proc.no_stdin = 1;
@@ -988,8 +988,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;
-   unsigned char *old_sha1 = cmd->old_sha1;
-   unsigned char *new_sha1 = cmd->new_sha1;
+   struct object_id *old_oid = &cmd->old_oid;
+   struct object_id *new_oid = &cmd->new_oid;
 
/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -1014,20 +1014,20 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
refuse_unconfigured_deny();
return "branch is currently checked out";
case DENY_UPDATE_INSTEAD:
-   ret = update_worktree(new_sha1);
+   ret = update_worktree(new_oid->hash);
if (ret)
return ret;
break;
}
}
 
-   if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
+   if (!is_null_oid(new_oid) && !has_object_file(new_oid)) {
error("unpack should have generated %s, "
- "but I can't find it!", sha1_to_hex(new_sha1));
+ "but I can't find it!", oid_to_hex(new_oid));
return "bad pack";
}
 
-   if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
+   if (!is_null_oid(old_oid) && is_null_oid(new_oid)) {
if (deny_deletes && starts_with(name, "refs/heads/")) {
rp_error("denying ref deletion for %s", name);
return "deletion prohibited";
@@ -1053,14 +1053,14 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
}
}
 
-   if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
-   !is_null_sha1(old_sha1) &&
+   if (deny_non_fast_forwards && !is_null_oid(new_oid) &&
+   !is_null_oid(old_oid) &&
starts_with(name, "refs/heads/")) {
struct object *old_object, *new_object;
struct commit *old_commit, *new_commit;
 
-   old_object = parse_object(old_sha1);
-   new_object = parse_object(new_sha1);
+   old_object = parse_object(old_oid->hash);
+   new_object = parse_object(new_oid->hash);
 
if (!old_object || !new_object ||
old_object->type != OBJ_COMMIT ||
@@ -1081,10 +1081,10 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
return "hook declined";
}
 
-   if (is_null_sha1(new_sha1)) {
+   if (is_null_oid(new_oid)) {
struct strbuf err = STRBUF_INIT;
-   if (!parse_object(old_sha1)) {
-   old_