From a2f96479abfe17357f666a80d9d0163dd8014fa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 29 Oct 2015 20:31:25 +0100 Subject: [PATCH 1/3] config: add failing test for an external modification We currently use the timestamp in order to decide whether a config file has changed since we last read it. This scheme falls down if the file is written twice within the same second, as we fail to detect the file change after the first read in that second. --- tests/config/stress.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/config/stress.c b/tests/config/stress.c index 503f44f03..6e960425c 100644 --- a/tests/config/stress.c +++ b/tests/config/stress.c @@ -107,3 +107,23 @@ void test_config_stress__complex(void) git_config_free(config); } + +void test_config_stress__quick_write(void) +{ + git_config *config_w, *config_r; + const char *path = "./config-quick-write"; + const char *key = "quick.write"; + int32_t i; + + /* Create an external writer for one instance with the other one */ + cl_git_pass(git_config_open_ondisk(&config_w, path)); + cl_git_pass(git_config_open_ondisk(&config_r, path)); + + /* Write and read in the same second (repeat to increase the chance of it happening) */ + for (i = 0; i < 10; i++) { + int32_t val; + cl_git_pass(git_config_set_int32(config_w, key, i)); + cl_git_pass(git_config_get_int32(&val, config_r, key)); + cl_assert_equal_i(i, val); + } +} From eb5977991a75f8d1630348a529805ba40765a616 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 29 Oct 2015 21:12:37 +0100 Subject: [PATCH 2/3] filebuf: use a checksum to detect file changes Instead of relying on the size and timestamp, which can hide changes performed in the same second, hash the file content's when we care about detecting changes. --- src/config_file.c | 15 +++++---------- src/fileops.c | 49 +++++++++++++++++++++++++---------------------- src/fileops.h | 3 ++- 3 files changed, 33 insertions(+), 34 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index 46f21c0f1..5f5e309e0 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -77,8 +77,7 @@ typedef struct git_config_file_iter { (iter) = (tmp)) struct reader { - time_t file_mtime; - size_t file_size; + git_oid checksum; char *file_path; git_buf buffer; char *read_ptr; @@ -285,7 +284,7 @@ static int config_open(git_config_backend *cfg, git_config_level_t level) git_buf_init(&reader->buffer, 0); res = git_futils_readbuffer_updated( - &reader->buffer, b->file_path, &reader->file_mtime, &reader->file_size, NULL); + &reader->buffer, b->file_path, &reader->checksum, NULL); /* It's fine if the file doesn't exist */ if (res == GIT_ENOTFOUND) @@ -345,7 +344,7 @@ static int config_refresh(git_config_backend *cfg) reader = git_array_get(b->readers, i); error = git_futils_readbuffer_updated( &reader->buffer, reader->file_path, - &reader->file_mtime, &reader->file_size, &updated); + &reader->checksum, &updated); if (error < 0 && error != GIT_ENOTFOUND) return error; @@ -1618,7 +1617,7 @@ static int read_on_variable( git_buf_init(&r->buffer, 0); result = git_futils_readbuffer_updated( - &r->buffer, r->file_path, &r->file_mtime, &r->file_size, NULL); + &r->buffer, r->file_path, &r->checksum, NULL); if (result == 0) { result = config_read(parse_data->values, parse_data->cfg_file, r, parse_data->level, parse_data->depth+1); @@ -1894,7 +1893,7 @@ static int config_write(diskfile_backend *cfg, const char *key, const regex_t *p } else { /* Lock the file */ if ((result = git_filebuf_open( - &file, cfg->file_path, 0, GIT_CONFIG_FILE_MODE)) < 0) { + &file, cfg->file_path, GIT_FILEBUF_HASH_CONTENTS, GIT_CONFIG_FILE_MODE)) < 0) { git_buf_free(&reader->buffer); return result; } @@ -1945,10 +1944,6 @@ static int config_write(diskfile_backend *cfg, const char *key, const regex_t *p git_buf_attach(&cfg->locked_content, git_buf_detach(&buf), len); } else { git_filebuf_write(&file, git_buf_cstr(&buf), git_buf_len(&buf)); - - /* refresh stats - if this errors, then commit will error too */ - (void)git_filebuf_stats(&reader->file_mtime, &reader->file_size, &file); - result = git_filebuf_commit(&file); } diff --git a/src/fileops.c b/src/fileops.c index 57d2ce9c3..9bef02847 100644 --- a/src/fileops.c +++ b/src/fileops.c @@ -153,11 +153,12 @@ int git_futils_readbuffer_fd(git_buf *buf, git_file fd, size_t len) } int git_futils_readbuffer_updated( - git_buf *buf, const char *path, time_t *mtime, size_t *size, int *updated) + git_buf *buf, const char *path, git_oid *checksum, int *updated) { + int error; git_file fd; struct stat st; - bool changed = false; + git_oid checksum_new; assert(buf && path && *path); @@ -178,26 +179,6 @@ int git_futils_readbuffer_updated( return -1; } - /* - * If we were given a time and/or a size, we only want to read the file - * if it has been modified. - */ - if (size && *size != (size_t)st.st_size) - changed = true; - if (mtime && *mtime != (time_t)st.st_mtime) - changed = true; - if (!size && !mtime) - changed = true; - - if (!changed) { - return 0; - } - - if (mtime != NULL) - *mtime = st.st_mtime; - if (size != NULL) - *size = (size_t)st.st_size; - if ((fd = git_futils_open_ro(path)) < 0) return fd; @@ -208,6 +189,28 @@ int git_futils_readbuffer_updated( p_close(fd); + if ((error = git_hash_buf(&checksum_new, buf->ptr, buf->size)) < 0) { + git_buf_free(buf); + return error; + } + + /* + * If we were given a checksum, we only want to use it if it's different + */ + if (checksum && !git_oid__cmp(checksum, &checksum_new)) { + git_buf_free(buf); + if (updated) + *updated = 0; + + return 0; + } + + /* + * If we're here, the file did change, or the user didn't have an old version + */ + if (checksum) + git_oid_cpy(checksum, &checksum_new); + if (updated != NULL) *updated = 1; @@ -216,7 +219,7 @@ int git_futils_readbuffer_updated( int git_futils_readbuffer(git_buf *buf, const char *path) { - return git_futils_readbuffer_updated(buf, path, NULL, NULL, NULL); + return git_futils_readbuffer_updated(buf, path, NULL, NULL); } int git_futils_writebuffer( diff --git a/src/fileops.h b/src/fileops.h index 572ff01a5..c6db1953e 100644 --- a/src/fileops.h +++ b/src/fileops.h @@ -13,6 +13,7 @@ #include "path.h" #include "pool.h" #include "strmap.h" +#include "oid.h" /** * Filebuffer methods @@ -21,7 +22,7 @@ */ extern int git_futils_readbuffer(git_buf *obj, const char *path); extern int git_futils_readbuffer_updated( - git_buf *obj, const char *path, time_t *mtime, size_t *size, int *updated); + git_buf *obj, const char *path, git_oid *checksum, int *updated); extern int git_futils_readbuffer_fd(git_buf *obj, git_file fd, size_t len); extern int git_futils_writebuffer( From 3547b122b57f4aafd173013d7f82e326b9cffb24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 30 Oct 2015 21:36:51 +0100 Subject: [PATCH 3/3] filebuf: use an internal buffer This reduces the chances of a crash in the thread tests. This shouldn't affect general usage too much, since the main usage of these functions are to read into an empty buffer. --- src/fileops.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/fileops.c b/src/fileops.c index 9bef02847..cfc22bb53 100644 --- a/src/fileops.c +++ b/src/fileops.c @@ -153,14 +153,15 @@ int git_futils_readbuffer_fd(git_buf *buf, git_file fd, size_t len) } int git_futils_readbuffer_updated( - git_buf *buf, const char *path, git_oid *checksum, int *updated) + git_buf *out, const char *path, git_oid *checksum, int *updated) { int error; git_file fd; struct stat st; + git_buf buf = GIT_BUF_INIT; git_oid checksum_new; - assert(buf && path && *path); + assert(out && path && *path); if (updated != NULL) *updated = 0; @@ -182,15 +183,15 @@ int git_futils_readbuffer_updated( if ((fd = git_futils_open_ro(path)) < 0) return fd; - if (git_futils_readbuffer_fd(buf, fd, (size_t)st.st_size) < 0) { + if (git_futils_readbuffer_fd(&buf, fd, (size_t)st.st_size) < 0) { p_close(fd); return -1; } p_close(fd); - if ((error = git_hash_buf(&checksum_new, buf->ptr, buf->size)) < 0) { - git_buf_free(buf); + if ((error = git_hash_buf(&checksum_new, buf.ptr, buf.size)) < 0) { + git_buf_free(&buf); return error; } @@ -198,7 +199,7 @@ int git_futils_readbuffer_updated( * If we were given a checksum, we only want to use it if it's different */ if (checksum && !git_oid__cmp(checksum, &checksum_new)) { - git_buf_free(buf); + git_buf_free(&buf); if (updated) *updated = 0; @@ -214,6 +215,9 @@ int git_futils_readbuffer_updated( if (updated != NULL) *updated = 1; + git_buf_swap(out, &buf); + git_buf_free(&buf); + return 0; }