@matheusbsbs / matheus-bagliano / commits / bfffb48c5d

stop leaking lock structs in some simple cases

Now that it's safe to declare a "struct lock_file" on the stack, we can do so (and avoid an intentional leak). These leaks were found by running t0000 and t0001 under valgrind (though certainly other similar leaks exist and just don't happen to be exercised by those tests). Initializing the lock_file's inner tempfile with NULL is not strictly necessary in these cases, but it's a good practice to model. It means that if we were to call a function like rollback_lock_file() on a lock that was never taken in the first place, it becomes a quiet noop (rather than undefined behavior). Likewise, it's always safe to rollback_lock_file() on a file that has already been committed or deleted, since that operation is a noop on an inactive lockfile (and that's why the case in config.c can drop the "if (lock)" check as we move away from using a pointer). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Sep 5, 2017 at 08:15 UTC bfffb48c5d520a5f181716b0330774a03cd3fa39

4 files changed +18 -37

builtin/reset.c

+3 -3

@@ -367,8 +367,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)

367 die_if_unmerged_cache(reset_type);

368

369 if (reset_type != SOFT) {

370 - struct lock_file *lock = xcalloc(1, sizeof(*lock));

371 - hold_locked_index(lock, LOCK_DIE_ON_ERROR);

370 + struct lock_file lock = LOCK_INIT;

371 + hold_locked_index(&lock, LOCK_DIE_ON_ERROR);

372 if (reset_type == MIXED) {

373 int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;

374 if (read_from_tree(&pathspec, &oid, intent_to_add))

@@ -384,7 +384,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)

384 die(_("Could not reset index file to revision '%s'."), rev);

385 }

386

387 - if (write_locked_index(&the_index, lock, COMMIT_LOCK))

387 + if (write_locked_index(&the_index, &lock, COMMIT_LOCK))

388 die(_("Could not write new index file."));

389 }

390

builtin/update-index.c

+4 -7

@@ -915,7 +915,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)

915 struct refresh_params refresh_args = {0, &has_errors};

916 int lock_error = 0;

917 int split_index = -1;

918 - struct lock_file *lock_file;

918 + struct lock_file lock_file = LOCK_INIT;

919 struct parse_opt_ctx_t ctx;

920 strbuf_getline_fn getline_fn;

921 int parseopt_state = PARSE_OPT_UNKNOWN;

@@ -1014,11 +1014,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)

1014

1015 git_config(git_default_config, NULL);

1016

1017 - /* We can't free this memory, it becomes part of a linked list parsed atexit() */

1018 - lock_file = xcalloc(1, sizeof(struct lock_file));

1019 -

1017 /* we will diagnose later if it turns out that we need to update it */

1021 - newfd = hold_locked_index(lock_file, 0);

1018 + newfd = hold_locked_index(&lock_file, 0);

1019 if (newfd < 0)

1020 lock_error = errno;

1021

@@ -1153,11 +1150,11 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)

1150 exit(128);

1151 unable_to_lock_die(get_index_file(), lock_error);

1152 }

1156 - if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))

1153 + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))

1154 die("Unable to write new index file");

1155 }

1156

1160 - rollback_lock_file(lock_file);

1157 + rollback_lock_file(&lock_file);

1158

1159 return has_errors ? 1 : 0;

1160 }

cache-tree.c

+4 -10

@@ -603,16 +603,10 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat

603 int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix)

604 {

605 int entries, was_valid, newfd;

606 - struct lock_file *lock_file;

606 + struct lock_file lock_file = LOCK_INIT;

607 int ret = 0;

608

609 - /*

610 - * We can't free this memory, it becomes part of a linked list

611 - * parsed atexit()

612 - */

613 - lock_file = xcalloc(1, sizeof(struct lock_file));

614 -

615 - newfd = hold_lock_file_for_update(lock_file, index_path, LOCK_DIE_ON_ERROR);

609 + newfd = hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);

610

611 entries = read_index_from(index_state, index_path);

612 if (entries < 0) {

@@ -632,7 +626,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co

626 goto out;

627 }

628 if (0 <= newfd) {

635 - if (!write_locked_index(index_state, lock_file, COMMIT_LOCK))

629 + if (!write_locked_index(index_state, &lock_file, COMMIT_LOCK))

630 newfd = -1;

631 }

632 /* Not being able to write is fine -- we are only interested

@@ -657,7 +651,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co

651

652 out:

653 if (0 <= newfd)

660 - rollback_lock_file(lock_file);

654 + rollback_lock_file(&lock_file);

655 return ret;

656 }

657

config.c

+7 -17

@@ -2450,7 +2450,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,

2450 {

2451 int fd = -1, in_fd = -1;

2452 int ret;

2453 - struct lock_file *lock = NULL;

2453 + struct lock_file lock = LOCK_INIT;

2454 char *filename_buf = NULL;

2455 char *contents = NULL;

2456 size_t contents_sz;

@@ -2469,8 +2469,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,

2469 * The lock serves a purpose in addition to locking: the new

2470 * contents of .git/config will be written into it.

2471 */

2472 - lock = xcalloc(1, sizeof(struct lock_file));

2473 - fd = hold_lock_file_for_update(lock, config_filename, 0);

2472 + fd = hold_lock_file_for_update(&lock, config_filename, 0);

2473 if (fd < 0) {

2474 error_errno("could not lock config file %s", config_filename);

2475 free(store.key);

@@ -2583,8 +2582,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,

2582 close(in_fd);

2583 in_fd = -1;

2584

2586 - if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {

2587 - error_errno("chmod on %s failed", get_lock_file_path(lock));

2585 + if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {

2586 + error_errno("chmod on %s failed", get_lock_file_path(&lock));

2587 ret = CONFIG_NO_WRITE;

2588 goto out_free;

2589 }

@@ -2639,28 +2638,19 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,

2638 contents = NULL;

2639 }

2640

2642 - if (commit_lock_file(lock) < 0) {

2641 + if (commit_lock_file(&lock) < 0) {

2642 error_errno("could not write config file %s", config_filename);

2643 ret = CONFIG_NO_WRITE;

2645 - lock = NULL;

2644 goto out_free;

2645 }

2646

2649 - /*

2650 - * lock is committed, so don't try to roll it back below.

2651 - * NOTE: Since lockfile.c keeps a linked list of all created

2652 - * lock_file structures, it isn't safe to free(lock). It's

2653 - * better to just leave it hanging around.

2654 - */

2655 - lock = NULL;

2647 ret = 0;

2648

2649 /* Invalidate the config cache */

2650 git_config_clear();

2651

2652 out_free:

2662 - if (lock)

2663 - rollback_lock_file(lock);

2653 + rollback_lock_file(&lock);

2654 free(filename_buf);

2655 if (contents)

2656 munmap(contents, contents_sz);

@@ -2669,7 +2659,7 @@ out_free:

2659 return ret;

2660

2661 write_err_out:

2672 - ret = write_error(get_lock_file_path(lock));

2662 + ret = write_error(get_lock_file_path(&lock));

2663 goto out_free;

2664

2665 }