https://github.com/git/git
Revision a5bb10fd5e74101e7c07da93e7c32bbe60f6173a authored by Taylor Blau on 06 April 2023, 18:07:58 UTC, committed by Johannes Schindelin on 17 April 2023, 19:15:40 UTC
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 <andre@ethiack.com> Reported-by: Vítor Pinho <vitor@ethiack.com> Helped-by: Patrick Steinhardt <ps@pks.im> Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Taylor Blau <me@ttaylorr.com>
1 parent 2919821
Tip revision: a5bb10fd5e74101e7c07da93e7c32bbe60f6173a authored by Taylor Blau on 06 April 2023, 18:07:58 UTC
config: avoid fixed-sized buffer when renaming/deleting a section
config: avoid fixed-sized buffer when renaming/deleting a section
Tip revision: a5bb10f
File | Mode | Size |
---|---|---|
araxis | -rw-r--r-- | 358 bytes |
bc | -rw-r--r-- | 423 bytes |
codecompare | -rw-r--r-- | 353 bytes |
deltawalker | -rw-r--r-- | 663 bytes |
diffmerge | -rw-r--r-- | 309 bytes |
diffuse | -rw-r--r-- | 248 bytes |
ecmerge | -rw-r--r-- | 306 bytes |
emerge | -rw-r--r-- | 438 bytes |
examdiff | -rw-r--r-- | 336 bytes |
guiffy | -rw-r--r-- | 263 bytes |
gvimdiff | -rw-r--r-- | 29 bytes |
kdiff3 | -rw-r--r-- | 522 bytes |
kompare | -rw-r--r-- | 117 bytes |
meld | -rw-r--r-- | 1.9 KB |
nvimdiff | -rw-r--r-- | 29 bytes |
opendiff | -rw-r--r-- | 267 bytes |
p4merge | -rw-r--r-- | 617 bytes |
smerge | -rw-r--r-- | 264 bytes |
tkdiff | -rw-r--r-- | 258 bytes |
tortoisemerge | -rw-r--r-- | 602 bytes |
vimdiff | -rw-r--r-- | 986 bytes |
winmerge | -rw-r--r-- | 361 bytes |
xxdiff | -rw-r--r-- | 584 bytes |
Computing file changes ...