From: Junio C Hamano Date: Fri, 22 May 2015 19:41:52 +0000 (-0700) Subject: Merge branch 'mh/write-refs-sooner-2.4' X-Git-Tag: v2.5.0-rc0~90 X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/91c90876defc60b444d8dcfcff057eafc37991b5?ds=inline;hp=-c Merge branch 'mh/write-refs-sooner-2.4' Multi-ref transaction support we merged a few releases ago unnecessarily kept many file descriptors open, risking to fail with resource exhaustion. This is for 2.4.x track. * mh/write-refs-sooner-2.4: ref_transaction_commit(): fix atomicity and avoid fd exhaustion ref_transaction_commit(): remove the local flags variable ref_transaction_commit(): inline call to write_ref_sha1() rename_ref(): inline calls to write_ref_sha1() from this function commit_ref_update(): new function, extracted from write_ref_sha1() write_ref_to_lockfile(): new function, extracted from write_ref_sha1() t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE update-ref: test handling large transactions properly ref_transaction_commit(): fix atomicity and avoid fd exhaustion ref_transaction_commit(): remove the local flags variable ref_transaction_commit(): inline call to write_ref_sha1() rename_ref(): inline calls to write_ref_sha1() from this function commit_ref_update(): new function, extracted from write_ref_sha1() write_ref_to_lockfile(): new function, extracted from write_ref_sha1() t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE update-ref: test handling large transactions properly --- 91c90876defc60b444d8dcfcff057eafc37991b5 diff --combined refs.c index b5189f4a57,96f5046712..825a1f6847 --- a/refs.c +++ b/refs.c @@@ -11,6 -11,7 +11,6 @@@ struct ref_lock char *orig_ref_name; struct lock_file *lk; unsigned char old_sha1[20]; - int lock_fd; }; /* @@@ -56,6 -57,12 +56,12 @@@ static unsigned char refname_dispositio */ #define REF_HAVE_OLD 0x10 + /* + * Used as a flag in ref_update::flags when the lockfile needs to be + * committed. + */ + #define REF_NEEDS_COMMIT 0x20 + /* * Try to read one refname component from the front of refname. * Return the length of the component found, or -1 if the component is @@@ -343,6 -350,8 +349,6 @@@ static struct ref_entry *create_ref_ent if (check_name && check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) die("Reference has invalid format: '%s'", refname); - if (!check_name && !refname_is_safe(refname)) - die("Reference has invalid name: '%s'", refname); len = strlen(refname) + 1; ref = xmalloc(sizeof(struct ref_entry) + len); hashcpy(ref->u.value.sha1, sha1); @@@ -1175,8 -1184,6 +1181,8 @@@ static void read_packed_refs(FILE *f, s int flag = REF_ISPACKED; if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { + if (!refname_is_safe(refname)) + die("packed refname is dangerous: %s", refname); hashclr(sha1); flag |= REF_BAD_NAME | REF_ISBROKEN; } @@@ -1322,8 -1329,6 +1328,8 @@@ static void read_loose_refs(const char } if (check_refname_format(refname.buf, REFNAME_ALLOW_ONELEVEL)) { + if (!refname_is_safe(refname.buf)) + die("loose refname is dangerous: %s", refname.buf); hashclr(sha1); flag |= REF_BAD_NAME | REF_ISBROKEN; } @@@ -1383,7 -1388,7 +1389,7 @@@ static int resolve_gitlink_ref_recursiv { int fd, len; char buffer[128], *p; - char *path; + const char *path; if (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN) return -1; @@@ -1476,11 -1481,7 +1482,11 @@@ static int resolve_missing_loose_ref(co } /* This function needs to return a meaningful errno on failure */ -const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned char *sha1, int *flags) +static const char *resolve_ref_unsafe_1(const char *refname, + int resolve_flags, + unsigned char *sha1, + int *flags, + struct strbuf *sb_path) { int depth = MAXDEPTH; ssize_t len; @@@ -1511,7 -1512,7 +1517,7 @@@ bad_name = 1; } for (;;) { - char path[PATH_MAX]; + const char *path; struct stat st; char *buf; int fd; @@@ -1521,9 -1522,7 +1527,9 @@@ return NULL; } - git_snpath(path, sizeof(path), "%s", refname); + strbuf_reset(sb_path); + strbuf_git_path(sb_path, "%s", refname); + path = sb_path->buf; /* * We might have to loop back here to avoid a race @@@ -1650,16 -1649,6 +1656,16 @@@ } } +const char *resolve_ref_unsafe(const char *refname, int resolve_flags, + unsigned char *sha1, int *flags) +{ + struct strbuf sb_path = STRBUF_INIT; + const char *ret = resolve_ref_unsafe_1(refname, resolve_flags, + sha1, flags, &sb_path); + strbuf_release(&sb_path); + return ret; +} + char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags) { return xstrdup_or_null(resolve_ref_unsafe(ref, resolve_flags, sha1, flags)); @@@ -2291,7 -2280,7 +2297,7 @@@ static struct ref_lock *lock_ref_sha1_b const struct string_list *skip, unsigned int flags, int *type_p) { - char *ref_file; + const char *ref_file; const char *orig_refname = refname; struct ref_lock *lock; int last_errno = 0; @@@ -2301,6 -2290,7 +2307,6 @@@ int attempts_remaining = 3; lock = xcalloc(1, sizeof(struct ref_lock)); - lock->lock_fd = -1; if (mustexist) resolve_flags |= RESOLVE_REF_READING; @@@ -2359,7 -2349,7 +2365,7 @@@ ref_file = git_path("%s", refname); retry: - switch (safe_create_leading_directories(ref_file)) { + switch (safe_create_leading_directories_const(ref_file)) { case SCLD_OK: break; /* success */ case SCLD_VANISHED: @@@ -2372,7 -2362,8 +2378,7 @@@ goto error_return; } - lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags); - if (lock->lock_fd < 0) { + if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) { last_errno = errno; if (errno == ENOENT && --attempts_remaining > 0) /* @@@ -2736,7 -2727,7 +2742,7 @@@ static int rename_tmp_log(const char *n int attempts_remaining = 4; retry: - switch (safe_create_leading_directories(git_path("logs/%s", newrefname))) { + switch (safe_create_leading_directories_const(git_path("logs/%s", newrefname))) { case SCLD_OK: break; /* success */ case SCLD_VANISHED: @@@ -2788,8 -2779,9 +2794,9 @@@ static int rename_ref_available(const c return ret; } - static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, - const char *logmsg); + static int write_ref_to_lockfile(struct ref_lock *lock, const unsigned char *sha1); + static int commit_ref_update(struct ref_lock *lock, + const unsigned char *sha1, const char *logmsg); int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg) { @@@ -2847,7 -2839,9 +2854,9 @@@ goto rollback; } hashcpy(lock->old_sha1, orig_sha1); - if (write_ref_sha1(lock, orig_sha1, logmsg)) { + + if (write_ref_to_lockfile(lock, orig_sha1) || + commit_ref_update(lock, orig_sha1, logmsg)) { error("unable to write current sha1 into %s", newrefname); goto rollback; } @@@ -2863,7 -2857,8 +2872,8 @@@ flag = log_all_ref_updates; log_all_ref_updates = 0; - if (write_ref_sha1(lock, orig_sha1, NULL)) + if (write_ref_to_lockfile(lock, orig_sha1) || + commit_ref_update(lock, orig_sha1, NULL)) error("unable to write current sha1 into %s", oldrefname); log_all_ref_updates = flag; @@@ -2883,6 -2878,7 +2893,6 @@@ static int close_ref(struct ref_lock *l { if (close_lock_file(lock->lk)) return -1; - lock->lock_fd = -1; return 0; } @@@ -2890,6 -2886,7 +2900,6 @@@ static int commit_ref(struct ref_lock * { if (commit_lock_file(lock->lk)) return -1; - lock->lock_fd = -1; return 0; } @@@ -2920,15 -2917,11 +2930,15 @@@ static int copy_msg(char *buf, const ch } /* This function must set a meaningful errno on failure */ -int log_ref_setup(const char *refname, char *logfile, int bufsize) +int log_ref_setup(const char *refname, struct strbuf *sb_logfile) { int logfd, oflags = O_APPEND | O_WRONLY; + char *logfile; - git_snpath(logfile, bufsize, "logs/%s", refname); + strbuf_git_path(sb_logfile, "logs/%s", refname); + logfile = sb_logfile->buf; + /* make sure the rest of the function can't change "logfile" */ + sb_logfile = NULL; if (log_all_ref_updates && (starts_with(refname, "refs/heads/") || starts_with(refname, "refs/remotes/") || @@@ -2999,22 -2992,18 +3009,22 @@@ static int log_ref_write_fd(int fd, con return 0; } -static int log_ref_write(const char *refname, const unsigned char *old_sha1, - const unsigned char *new_sha1, const char *msg) +static int log_ref_write_1(const char *refname, const unsigned char *old_sha1, + const unsigned char *new_sha1, const char *msg, + struct strbuf *sb_log_file) { int logfd, result, oflags = O_APPEND | O_WRONLY; - char log_file[PATH_MAX]; + char *log_file; if (log_all_ref_updates < 0) log_all_ref_updates = !is_bare_repository(); - result = log_ref_setup(refname, log_file, sizeof(log_file)); + result = log_ref_setup(refname, sb_log_file); if (result) return result; + log_file = sb_log_file->buf; + /* make sure the rest of the function can't change "log_file" */ + sb_log_file = NULL; logfd = open(log_file, oflags); if (logfd < 0) @@@ -3037,26 -3026,17 +3047,26 @@@ return 0; } +static int log_ref_write(const char *refname, const unsigned char *old_sha1, + const unsigned char *new_sha1, const char *msg) +{ + struct strbuf sb = STRBUF_INIT; + int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb); + strbuf_release(&sb); + return ret; +} + int is_branch(const char *refname) { return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/"); } /* - * Write sha1 into the ref specified by the lock. Make sure that errno - * is sane on error. + * Write sha1 into the open lockfile, then close the lockfile. On + * errors, rollback the lockfile and set errno to reflect the problem. */ - static int write_ref_sha1(struct ref_lock *lock, - const unsigned char *sha1, const char *logmsg) + static int write_ref_to_lockfile(struct ref_lock *lock, + const unsigned char *sha1) { static char term = '\n'; struct object *o; @@@ -3076,8 -3056,8 +3086,8 @@@ errno = EINVAL; return -1; } - if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 || - write_in_full(lock->lock_fd, &term, 1) != 1 || + if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 || + write_in_full(lock->lk->fd, &term, 1) != 1 || close_ref(lock) < 0) { int save_errno = errno; error("Couldn't write %s", lock->lk->filename.buf); @@@ -3085,6 -3065,17 +3095,17 @@@ errno = save_errno; return -1; } + return 0; + } + + /* + * Commit a change to a loose reference that has already been written + * to the loose reference lockfile. Also update the reflogs if + * necessary, using the specified lockmsg (which can be NULL). + */ + static int commit_ref_update(struct ref_lock *lock, + const unsigned char *sha1, const char *logmsg) + { clear_loose_ref_cache(&ref_cache); if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 || (strcmp(lock->ref_name, lock->orig_ref_name) && @@@ -3775,19 -3766,24 +3796,24 @@@ int ref_transaction_commit(struct ref_t goto cleanup; } - /* Acquire all locks while verifying old values */ + /* + * Acquire all locks, verify old values if provided, check + * that new values are valid, and write new values to the + * lockfiles, ready to be activated. Only keep one lockfile + * open at a time to avoid running out of file descriptors. + */ for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; - unsigned int flags = update->flags; - if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) - flags |= REF_DELETING; + if ((update->flags & REF_HAVE_NEW) && + is_null_sha1(update->new_sha1)) + update->flags |= REF_DELETING; update->lock = lock_ref_sha1_basic( update->refname, ((update->flags & REF_HAVE_OLD) ? update->old_sha1 : NULL), NULL, - flags, + update->flags, &update->type); if (!update->lock) { ret = (errno == ENOTDIR) @@@ -3797,34 -3793,60 +3823,60 @@@ update->refname); goto cleanup; } - } - - /* Perform updates first so live commits remain referenced */ - for (i = 0; i < n; i++) { - struct ref_update *update = updates[i]; - int flags = update->flags; - - if ((flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) { + if ((update->flags & REF_HAVE_NEW) && + !(update->flags & REF_DELETING)) { int overwriting_symref = ((update->type & REF_ISSYMREF) && (update->flags & REF_NODEREF)); - if (!overwriting_symref - && !hashcmp(update->lock->old_sha1, update->new_sha1)) { + if (!overwriting_symref && + !hashcmp(update->lock->old_sha1, update->new_sha1)) { /* * The reference already has the desired * value, so we don't need to write it. */ - unlock_ref(update->lock); + } else if (write_ref_to_lockfile(update->lock, + update->new_sha1)) { + /* + * The lock was freed upon failure of + * write_ref_to_lockfile(): + */ + update->lock = NULL; + strbuf_addf(err, "Cannot update the ref '%s'.", + update->refname); + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; + } else { + update->flags |= REF_NEEDS_COMMIT; + } + } + if (!(update->flags & REF_NEEDS_COMMIT)) { + /* + * We didn't have to write anything to the lockfile. + * Close it to free up the file descriptor: + */ + if (close_ref(update->lock)) { + strbuf_addf(err, "Couldn't close %s.lock", + update->refname); + goto cleanup; + } + } + } + + /* Perform updates first so live commits remain referenced */ + for (i = 0; i < n; i++) { + struct ref_update *update = updates[i]; + + if (update->flags & REF_NEEDS_COMMIT) { + if (commit_ref_update(update->lock, + update->new_sha1, update->msg)) { + /* freed by commit_ref_update(): */ update->lock = NULL; - } else if (write_ref_sha1(update->lock, update->new_sha1, - update->msg)) { - update->lock = NULL; /* freed by write_ref_sha1 */ strbuf_addf(err, "Cannot update the ref '%s'.", update->refname); ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } else { - /* freed by write_ref_sha1(): */ + /* freed by commit_ref_update(): */ update->lock = NULL; } } @@@ -3833,15 -3855,14 +3885,14 @@@ /* Perform deletes now that updates are safely completed */ for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; - int flags = update->flags; - if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) { + if (update->flags & REF_DELETING) { if (delete_ref_loose(update->lock, update->type, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } - if (!(flags & REF_ISPRUNING)) + if (!(update->flags & REF_ISPRUNING)) string_list_append(&refs_to_delete, update->lock->ref_name); } @@@ -4114,9 -4135,9 +4165,9 @@@ int reflog_expire(const char *refname, status |= error("couldn't write %s: %s", log_file, strerror(errno)); } else if (update && - (write_in_full(lock->lock_fd, + (write_in_full(lock->lk->fd, sha1_to_hex(cb.last_kept_sha1), 40) != 40 || - write_str_in_full(lock->lock_fd, "\n") != 1 || + write_str_in_full(lock->lk->fd, "\n") != 1 || close_ref(lock) < 0)) { status |= error("couldn't write %s", lock->lk->filename.buf);