From a5bb10fd5e74101e7c07da93e7c32bbe60f6173a Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 6 Apr 2023 14:07:58 -0400 Subject: [PATCH] config: avoid fixed-sized buffer when renaming/deleting a section MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit When renaming (or deleting) a section of configuration, Git uses the function `git_config_copy_or_rename_section_in_file()` to rewrite the configuration file after applying the rename or deletion to the given section. To do this, Git repeatedly calls `fgets()` to read the existing configuration data into a fixed size buffer. When the configuration value under `old_name` exceeds the size of the buffer, we will call `fgets()` an additional time even if there is no newline in the configuration file, since our read length is capped at `sizeof(buf)`. If the first character of the buffer (after zero or more characters satisfying `isspace()`) is a '[', Git will incorrectly treat it as beginning a new section when the original section is being removed. In other words, a configuration value satisfying this criteria can incorrectly be considered as a new secftion instead of a variable in the original section. Avoid this issue by using a variable-width buffer in the form of a strbuf rather than a fixed-with region on the stack. A couple of small points worth noting: - Using a strbuf will cause us to allocate arbitrary sizes to match the length of each line. In practice, we don't expect any reasonable configuration files to have lines that long, and a bandaid will be introduced in a later patch to ensure that this is the case. - We are using strbuf_getwholeline() here instead of strbuf_getline() in order to match `fgets()`'s behavior of leaving the trailing LF character on the buffer (as well as a trailing NUL). This could be changed later, but using strbuf_getwholeline() changes the least about this function's implementation, so it is picked as the safest path. - It is temping to want to replace the loop to skip over characters matching isspace() at the beginning of the buffer with a convenience function like `strbuf_ltrim()`. But this is the wrong approach for a couple of reasons: First, it involves a potentially large and expensive `memmove()` which we would like to avoid. Second, and more importantly, we also *do* want to preserve those spaces to avoid changing the output of other sections. In all, this patch is a minimal replacement of the fixed-width buffer in `git_config_copy_or_rename_section_in_file()` to instead use a `struct strbuf`. Reported-by: André Baptista Reported-by: Vítor Pinho Helped-by: Patrick Steinhardt Co-authored-by: Johannes Schindelin Signed-off-by: Johannes Schindelin Signed-off-by: Taylor Blau --- config.c | 13 +++++++------ t/t1300-config.sh | 4 ++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/config.c b/config.c index 1137bd73af..524347676d 100644 --- a/config.c +++ b/config.c @@ -3091,7 +3091,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename char *filename_buf = NULL; struct lock_file lock = LOCK_INIT; int out_fd; - char buf[1024]; + struct strbuf buf = STRBUF_INIT; FILE *config_file = NULL; struct stat st; struct strbuf copystr = STRBUF_INIT; @@ -3132,14 +3132,14 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename goto out; } - while (fgets(buf, sizeof(buf), config_file)) { + while (!strbuf_getwholeline(&buf, config_file, '\n')) { unsigned i; int length; int is_section = 0; - char *output = buf; - for (i = 0; buf[i] && isspace(buf[i]); i++) + char *output = buf.buf; + for (i = 0; buf.buf[i] && isspace(buf.buf[i]); i++) ; /* do nothing */ - if (buf[i] == '[') { + if (buf.buf[i] == '[') { /* it's a section */ int offset; is_section = 1; @@ -3158,7 +3158,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename strbuf_reset(©str); } - offset = section_name_match(&buf[i], old_name); + offset = section_name_match(&buf.buf[i], old_name); if (offset > 0) { ret++; if (new_name == NULL) { @@ -3233,6 +3233,7 @@ out: out_no_rollback: free(filename_buf); config_store_data_clear(&store); + strbuf_release(&buf); return ret; } diff --git a/t/t1300-config.sh b/t/t1300-config.sh index cd8f744160..24c13b91db 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -613,7 +613,7 @@ test_expect_success 'renaming to bogus section is rejected' ' test_must_fail git config --rename-section branch.zwei "bogus name" ' -test_expect_failure 'renaming a section with a long line' ' +test_expect_success 'renaming a section with a long line' ' { printf "[b]\\n" && printf " c = d %1024s [a] e = f\\n" " " && @@ -623,7 +623,7 @@ test_expect_failure 'renaming a section with a long line' ' test_must_fail git config -f y b.e ' -test_expect_failure 'renaming an embedded section with a long line' ' +test_expect_success 'renaming an embedded section with a long line' ' { printf "[b]\\n" && printf " c = d %1024s [a] [foo] e = f\\n" " " && -- 2.11.4.GIT