From 39950fef8bb45e944655e48393ee04c0b33211f5 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 27 Apr 2016 12:39:11 +0200 Subject: refname_is_safe(): use skip_prefix() Signed-off-by: Michael Haggerty --- refs.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 87dc82f1d8..5789152743 100644 --- a/refs.c +++ b/refs.c @@ -120,17 +120,19 @@ int check_refname_format(const char *refname, int flags) int refname_is_safe(const char *refname) { - if (starts_with(refname, "refs/")) { + const char *rest; + + if (skip_prefix(refname, "refs/", &rest)) { char *buf; int result; - buf = xmallocz(strlen(refname)); /* * Does the refname try to escape refs/? * For example: refs/foo/../bar is safe but refs/foo/../../bar * is not. */ - result = !normalize_path_copy(buf, refname + strlen("refs/")); + buf = xmallocz(strlen(rest)); + result = !normalize_path_copy(buf, rest); free(buf); return result; } -- cgit v1.2.3 From 35db25c65f6f77c153ef2b1183ea7821236201c8 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 27 Apr 2016 12:42:27 +0200 Subject: refname_is_safe(): don't allow the empty string Signed-off-by: Michael Haggerty --- refs.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 5789152743..ca0280f7eb 100644 --- a/refs.c +++ b/refs.c @@ -136,11 +136,12 @@ int refname_is_safe(const char *refname) free(buf); return result; } - while (*refname) { + + do { if (!isupper(*refname) && *refname != '_') return 0; refname++; - } + } while (*refname); return 1; } -- cgit v1.2.3 From e40f3557f7e767bd2be2a824bc3bc2379aa69931 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 27 Apr 2016 12:40:39 +0200 Subject: refname_is_safe(): insist that the refname already be normalized The reference name is going to be compared to other reference names, so it should be in its normalized form. Signed-off-by: Michael Haggerty --- refs.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index ca0280f7eb..b18d9959af 100644 --- a/refs.c +++ b/refs.c @@ -125,14 +125,19 @@ int refname_is_safe(const char *refname) if (skip_prefix(refname, "refs/", &rest)) { char *buf; int result; + size_t restlen = strlen(rest); + + /* rest must not be empty, or start or end with "/" */ + if (!restlen || *rest == '/' || rest[restlen - 1] == '/') + return 0; /* * Does the refname try to escape refs/? * For example: refs/foo/../bar is safe but refs/foo/../../bar * is not. */ - buf = xmallocz(strlen(rest)); - result = !normalize_path_copy(buf, rest); + buf = xmallocz(restlen); + result = !normalize_path_copy(buf, rest) && !strcmp(buf, rest); free(buf); return result; } -- cgit v1.2.3 From 0568c8e9dce2aa0dd18f41f23e3465f3639e371e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 27 Apr 2016 15:21:36 +0200 Subject: refs: make error messages more consistent * Always start error messages with a lower-case letter. * Always enclose reference names in single quotes. Signed-off-by: Michael Haggerty --- refs.c | 8 ++++---- refs/files-backend.c | 32 ++++++++++++++++---------------- t/t1400-update-ref.sh | 4 ++-- 3 files changed, 22 insertions(+), 22 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index b18d9959af..ba14105959 100644 --- a/refs.c +++ b/refs.c @@ -504,7 +504,7 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1, filename = git_path("%s", pseudoref); fd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR); if (fd < 0) { - strbuf_addf(err, "Could not open '%s' for writing: %s", + strbuf_addf(err, "could not open '%s' for writing: %s", filename, strerror(errno)); return -1; } @@ -515,14 +515,14 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1, if (read_ref(pseudoref, actual_old_sha1)) die("could not read ref '%s'", pseudoref); if (hashcmp(actual_old_sha1, old_sha1)) { - strbuf_addf(err, "Unexpected sha1 when writing %s", pseudoref); + strbuf_addf(err, "unexpected sha1 when writing '%s'", pseudoref); rollback_lock_file(&lock); goto done; } } if (write_in_full(fd, buf.buf, buf.len) != buf.len) { - strbuf_addf(err, "Could not write to '%s'", filename); + strbuf_addf(err, "could not write to '%s'", filename); rollback_lock_file(&lock); goto done; } @@ -792,7 +792,7 @@ int ref_transaction_update(struct ref_transaction *transaction, if (new_sha1 && !is_null_sha1(new_sha1) && check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { - strbuf_addf(err, "refusing to update ref with bad name %s", + strbuf_addf(err, "refusing to update ref with bad name '%s'", refname); return -1; } diff --git a/refs/files-backend.c b/refs/files-backend.c index dc247e0e7b..c978fe49c9 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1701,7 +1701,7 @@ static int verify_lock(struct ref_lock *lock, lock->old_oid.hash, NULL)) { if (old_sha1) { int save_errno = errno; - strbuf_addf(err, "can't verify ref %s", lock->ref_name); + strbuf_addf(err, "can't verify ref '%s'", lock->ref_name); errno = save_errno; return -1; } else { @@ -1710,7 +1710,7 @@ static int verify_lock(struct ref_lock *lock, } } if (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) { - strbuf_addf(err, "ref %s is at %s but expected %s", + strbuf_addf(err, "ref '%s' is at %s but expected %s", lock->ref_name, sha1_to_hex(lock->old_oid.hash), sha1_to_hex(old_sha1)); @@ -1790,7 +1790,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, if (last_errno != ENOTDIR || !verify_refname_available_dir(orig_refname, extras, skip, get_loose_refs(&ref_cache), err)) - strbuf_addf(err, "unable to resolve reference %s: %s", + strbuf_addf(err, "unable to resolve reference '%s': %s", orig_refname, strerror(last_errno)); goto error_return; @@ -1828,7 +1828,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, /* fall through */ default: last_errno = errno; - strbuf_addf(err, "unable to create directory for %s", + strbuf_addf(err, "unable to create directory for '%s'", ref_file.buf); goto error_return; } @@ -2473,7 +2473,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str strbuf_git_path(logfile, "logs/%s", refname); if (force_create || should_autocreate_reflog(refname)) { if (safe_create_leading_directories(logfile->buf) < 0) { - strbuf_addf(err, "unable to create directory for %s: " + strbuf_addf(err, "unable to create directory for '%s': " "%s", logfile->buf, strerror(errno)); return -1; } @@ -2487,7 +2487,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str if (errno == EISDIR) { if (remove_empty_directories(logfile)) { - strbuf_addf(err, "There are still logs under " + strbuf_addf(err, "there are still logs under " "'%s'", logfile->buf); return -1; } @@ -2495,7 +2495,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str } if (logfd < 0) { - strbuf_addf(err, "unable to append to %s: %s", + strbuf_addf(err, "unable to append to '%s': %s", logfile->buf, strerror(errno)); return -1; } @@ -2564,13 +2564,13 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1, result = log_ref_write_fd(logfd, old_sha1, new_sha1, git_committer_info(0), msg); if (result) { - strbuf_addf(err, "unable to append to %s: %s", logfile->buf, + strbuf_addf(err, "unable to append to '%s': %s", logfile->buf, strerror(errno)); close(logfd); return -1; } if (close(logfd)) { - strbuf_addf(err, "unable to append to %s: %s", logfile->buf, + strbuf_addf(err, "unable to append to '%s': %s", logfile->buf, strerror(errno)); return -1; } @@ -2611,14 +2611,14 @@ static int write_ref_to_lockfile(struct ref_lock *lock, o = parse_object(sha1); if (!o) { strbuf_addf(err, - "Trying to write ref %s with nonexistent object %s", + "trying to write ref '%s' with nonexistent object %s", lock->ref_name, sha1_to_hex(sha1)); unlock_ref(lock); return -1; } if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) { strbuf_addf(err, - "Trying to write non-commit object %s to branch %s", + "trying to write non-commit object %s to branch '%s'", sha1_to_hex(sha1), lock->ref_name); unlock_ref(lock); return -1; @@ -2628,7 +2628,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock, write_in_full(fd, &term, 1) != 1 || close_ref(lock) < 0) { strbuf_addf(err, - "Couldn't write %s", get_lock_file_path(lock->lk)); + "couldn't write '%s'", get_lock_file_path(lock->lk)); unlock_ref(lock); return -1; } @@ -2649,7 +2649,7 @@ static int commit_ref_update(struct ref_lock *lock, (strcmp(lock->ref_name, lock->orig_ref_name) && log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0)) { char *old_msg = strbuf_detach(err, NULL); - strbuf_addf(err, "Cannot update the ref '%s': %s", + strbuf_addf(err, "cannot update the ref '%s': %s", lock->ref_name, old_msg); free(old_msg); unlock_ref(lock); @@ -2684,7 +2684,7 @@ static int commit_ref_update(struct ref_lock *lock, } } if (commit_ref(lock)) { - strbuf_addf(err, "Couldn't set %s", lock->ref_name); + strbuf_addf(err, "couldn't set '%s'", lock->ref_name); unlock_ref(lock); return -1; } @@ -3033,7 +3033,7 @@ static int ref_update_reject_duplicates(struct string_list *refnames, for (i = 1; i < n; i++) if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) { strbuf_addf(err, - "Multiple updates for ref '%s' not allowed.", + "multiple updates for ref '%s' not allowed.", refnames->items[i].string); return 1; } @@ -3137,7 +3137,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, * Close it to free up the file descriptor: */ if (close_ref(update->lock)) { - strbuf_addf(err, "Couldn't close %s.lock", + strbuf_addf(err, "couldn't close '%s.lock'", update->refname); goto cleanup; } diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index af1b20dd5c..40b0ccedfc 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -479,7 +479,7 @@ test_expect_success 'stdin fails with duplicate refs' ' create $a $m EOF test_must_fail git update-ref --stdin err && - grep "fatal: Multiple updates for ref '"'"'$a'"'"' not allowed." err + grep "fatal: multiple updates for ref '"'"'$a'"'"' not allowed." err ' test_expect_success 'stdin create ref works' ' @@ -880,7 +880,7 @@ test_expect_success 'stdin -z fails option with unknown name' ' test_expect_success 'stdin -z fails with duplicate refs' ' printf $F "create $a" "$m" "create $b" "$m" "create $a" "$m" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: Multiple updates for ref '"'"'$a'"'"' not allowed." err + grep "fatal: multiple updates for ref '"'"'$a'"'"' not allowed." err ' test_expect_success 'stdin -z create ref works' ' -- cgit v1.2.3 From c52ce248d63a185eb0a616b361d1fd72c5c66451 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 24 Apr 2016 09:48:26 +0200 Subject: ref_transaction_create(): disallow recursive pruning It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING without REF_NODEREF. Forbid it explicitly. Change the one REF_ISPRUNING caller to pass REF_NODEREF too. Signed-off-by: Michael Haggerty --- refs.c | 3 +++ refs/files-backend.c | 2 +- refs/refs-internal.h | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index ba14105959..5dc2473fbb 100644 --- a/refs.c +++ b/refs.c @@ -790,6 +790,9 @@ int ref_transaction_update(struct ref_transaction *transaction, if (transaction->state != REF_TRANSACTION_OPEN) die("BUG: update called for transaction that is not open"); + if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF)) + die("BUG: REF_ISPRUNING set without REF_NODEREF"); + if (new_sha1 && !is_null_sha1(new_sha1) && check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { strbuf_addf(err, "refusing to update ref with bad name '%s'", diff --git a/refs/files-backend.c b/refs/files-backend.c index c978fe49c9..35d37ce58b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2087,7 +2087,7 @@ static void prune_ref(struct ref_to_prune *r) transaction = ref_transaction_begin(&err); if (!transaction || ref_transaction_delete(transaction, r->name, r->sha1, - REF_ISPRUNING, NULL, &err) || + REF_ISPRUNING | REF_NODEREF, NULL, &err) || ref_transaction_commit(transaction, &err)) { ref_transaction_free(transaction); error("%s", err.buf); diff --git a/refs/refs-internal.h b/refs/refs-internal.h index de7722e323..1f94f7a262 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -15,7 +15,7 @@ /* * Used as a flag in ref_update::flags when a loose ref is being - * pruned. + * pruned. This flag must only be used when REF_NODEREF is set. */ #define REF_ISPRUNING 0x04 -- cgit v1.2.3 From 71564516deccafba0a58129bd7d3851e28fdb4bb Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 25 Apr 2016 11:39:54 +0200 Subject: add_update(): initialize the whole ref_update Change add_update() to initialize all of the fields in the new ref_update object. Rename the function to ref_transaction_add_update(), and increase its visibility to all of the refs-related code. All of this makes the function more useful for other future callers. Signed-off-by: Michael Haggerty --- refs.c | 48 ++++++++++++++++++++++++++---------------------- refs/refs-internal.h | 14 ++++++++++++++ 2 files changed, 40 insertions(+), 22 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 5dc2473fbb..7c4eeb1c99 100644 --- a/refs.c +++ b/refs.c @@ -766,13 +766,33 @@ void ref_transaction_free(struct ref_transaction *transaction) free(transaction); } -static struct ref_update *add_update(struct ref_transaction *transaction, - const char *refname) +struct ref_update *ref_transaction_add_update( + struct ref_transaction *transaction, + const char *refname, unsigned int flags, + const unsigned char *new_sha1, + const unsigned char *old_sha1, + const char *msg) { struct ref_update *update; + + if (transaction->state != REF_TRANSACTION_OPEN) + die("BUG: update called for transaction that is not open"); + + if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF)) + die("BUG: REF_ISPRUNING set without REF_NODEREF"); + FLEX_ALLOC_STR(update, refname, refname); ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc); transaction->updates[transaction->nr++] = update; + + update->flags = flags; + + if (flags & REF_HAVE_NEW) + hashcpy(update->new_sha1, new_sha1); + if (flags & REF_HAVE_OLD) + hashcpy(update->old_sha1, old_sha1); + if (msg) + update->msg = xstrdup(msg); return update; } @@ -783,16 +803,8 @@ int ref_transaction_update(struct ref_transaction *transaction, unsigned int flags, const char *msg, struct strbuf *err) { - struct ref_update *update; - assert(err); - if (transaction->state != REF_TRANSACTION_OPEN) - die("BUG: update called for transaction that is not open"); - - if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF)) - die("BUG: REF_ISPRUNING set without REF_NODEREF"); - if (new_sha1 && !is_null_sha1(new_sha1) && check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { strbuf_addf(err, "refusing to update ref with bad name '%s'", @@ -800,18 +812,10 @@ int ref_transaction_update(struct ref_transaction *transaction, return -1; } - update = add_update(transaction, refname); - if (new_sha1) { - hashcpy(update->new_sha1, new_sha1); - flags |= REF_HAVE_NEW; - } - if (old_sha1) { - hashcpy(update->old_sha1, old_sha1); - flags |= REF_HAVE_OLD; - } - update->flags = flags; - if (msg) - update->msg = xstrdup(msg); + flags |= (new_sha1 ? REF_HAVE_NEW : 0) | (old_sha1 ? REF_HAVE_OLD : 0); + + ref_transaction_add_update(transaction, refname, flags, + new_sha1, old_sha1, msg); return 0; } diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 9686e60942..babdf2769f 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -157,6 +157,20 @@ struct ref_update { const char refname[FLEX_ARRAY]; }; +/* + * Add a ref_update with the specified properties to transaction, and + * return a pointer to the new object. This function does not verify + * that refname is well-formed. new_sha1 and old_sha1 are only + * dereferenced if the REF_HAVE_NEW and REF_HAVE_OLD bits, + * respectively, are set in flags. + */ +struct ref_update *ref_transaction_add_update( + struct ref_transaction *transaction, + const char *refname, unsigned int flags, + const unsigned char *new_sha1, + const unsigned char *old_sha1, + const char *msg); + /* * Transaction states. * OPEN: The transaction is in a valid state and can accept new updates. -- cgit v1.2.3 From 8a679de6f1a4bd077f828273f75eea46947b5b73 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 27 Apr 2016 15:54:45 +0200 Subject: ref_transaction_update(): check refname_is_safe() at a minimum If the user has asked that a new value be set for a reference, we use check_refname_format() to verify that the reference name satisfies all of the rules. But in other cases, at least check that refname_is_safe(). Signed-off-by: Michael Haggerty --- refs.c | 5 +++-- t/t1400-update-ref.sh | 2 +- t/t1430-bad-ref-name.sh | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 7c4eeb1c99..842c5c7b05 100644 --- a/refs.c +++ b/refs.c @@ -805,8 +805,9 @@ int ref_transaction_update(struct ref_transaction *transaction, { assert(err); - if (new_sha1 && !is_null_sha1(new_sha1) && - check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { + if ((new_sha1 && !is_null_sha1(new_sha1)) ? + check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) : + !refname_is_safe(refname)) { strbuf_addf(err, "refusing to update ref with bad name '%s'", refname); return -1; diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 40b0ccedfc..08bd8fd8d6 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -23,7 +23,7 @@ test_expect_success setup ' m=refs/heads/master n_dir=refs/heads/gu n=$n_dir/fixes -outside=foo +outside=refs/foo test_expect_success \ "create $m" \ diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh index 25ddab4e98..8937e25e49 100755 --- a/t/t1430-bad-ref-name.sh +++ b/t/t1430-bad-ref-name.sh @@ -285,7 +285,7 @@ test_expect_success 'update-ref -d cannot delete non-ref in .git dir' ' echo precious >expect && test_must_fail git update-ref -d my-private-file >output 2>error && test_must_be_empty output && - test_i18ngrep -e "cannot lock .*: unable to resolve reference" error && + test_i18ngrep -e "refusing to update ref with bad name" error && test_cmp expect .git/my-private-file ' -- cgit v1.2.3