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
Raw File
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
Tip revision: a5bb10f
refspec.h
#ifndef REFSPEC_H
#define REFSPEC_H

#define TAG_REFSPEC "refs/tags/*:refs/tags/*"
extern const struct refspec_item *tag_refspec;

/**
 * A struct refspec_item holds the parsed interpretation of a refspec.  If it
 * will force updates (starts with a '+'), force is true.  If it is a pattern
 * (sides end with '*') pattern is true.  If it is a negative refspec, (starts
 * with '^'), negative is true.  src and dest are the two sides (including '*'
 * characters if present); if there is only one side, it is src, and dst is
 * NULL; if sides exist but are empty (i.e., the refspec either starts or ends
 * with ':'), the corresponding side is "".
 *
 * remote_find_tracking(), given a remote and a struct refspec_item with either src
 * or dst filled out, will fill out the other such that the result is in the
 * "fetch" specification for the remote (note that this evaluates patterns and
 * returns a single result).
 */
struct refspec_item {
	unsigned force : 1;
	unsigned pattern : 1;
	unsigned matching : 1;
	unsigned exact_sha1 : 1;
	unsigned negative : 1;

	char *src;
	char *dst;
};

#define REFSPEC_FETCH 1
#define REFSPEC_PUSH 0

#define REFSPEC_INIT_FETCH { .fetch = REFSPEC_FETCH }
#define REFSPEC_INIT_PUSH { .fetch = REFSPEC_PUSH }

/**
 * An array of strings can be parsed into a struct refspec using
 * parse_fetch_refspec() or parse_push_refspec().
 */
struct refspec {
	struct refspec_item *items;
	int alloc;
	int nr;

	const char **raw;
	int raw_alloc;
	int raw_nr;

	int fetch;
};

int refspec_item_init(struct refspec_item *item, const char *refspec,
		      int fetch);
void refspec_item_init_or_die(struct refspec_item *item, const char *refspec,
			      int fetch);
void refspec_item_clear(struct refspec_item *item);
void refspec_init(struct refspec *rs, int fetch);
void refspec_append(struct refspec *rs, const char *refspec);
__attribute__((format (printf,2,3)))
void refspec_appendf(struct refspec *rs, const char *fmt, ...);
void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
void refspec_clear(struct refspec *rs);

int valid_fetch_refspec(const char *refspec);
int valid_remote_name(const char *name);

struct strvec;
/*
 * Determine what <prefix> values to pass to the peer in ref-prefix lines
 * (see Documentation/technical/protocol-v2.txt).
 */
void refspec_ref_prefixes(const struct refspec *rs,
			  struct strvec *ref_prefixes);

#endif /* REFSPEC_H */
back to top