git/config.c, branch v2.29.2

Merge branch 'jk/leakfix'

2020-08-27T21:04:49Z

Code clean-up. * jk/leakfix: submodule--helper: fix leak of core.worktree value config: fix leak in git_config_get_expiry_in_days() config: drop git_config_get_string_const() config: fix leaks from git_config_get_string_const() checkout: fix leak of non-existent branch names submodule--helper: use strbuf_release() to free strbufs clear_pattern_list(): clear embedded hashmaps

config: fix leak in git_config_get_expiry_in_days()

2020-08-17T22:35:47Z

We use git_config_get_string() to retrieve the expiry value in a newly allocated string. But after parsing it, we never free it, leaking the memory. We could fix this with a free() obviously, but there's an even better solution: we can use the non-allocating "tmp" variant of the function; we only need it to be valid for the lifetime of our parse function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

config: drop git_config_get_string_const()

2020-08-17T22:35:47Z

As evidenced by the leak fixes in the previous commit, the "const" in git_config_get_string_const() clearly misleads people into thinking that it does not allocate a copy of the string. We can fix this by renaming it, but it's easier still to just drop it. Of the four remaining callers: - The one in git_config_parse_expiry() still needs to allocate, since that's what its callers expect. We can just use the non-const version and cast our pointer. Slightly ugly, but the damage is contained in one spot. - The two in apply are writing to global "const char *" variables, and need to continue allocating. We often mark these as const because we assign default string literals to them. But in this case we don't do that, so we can just declare them as real "char *" pointers and use the non-const version. - The call in checkout doesn't actually need a copy; it can just use the non-allocating "tmp" version of the function. The function is also mentioned in the MyFirstContribution document. We can swap that call out for the non-allocating "tmp" variant, which fits well in the example given. We'll drop the "configset" and "repo" variants, as well (which are unused). Note that this frees up the "const" name, so we could rename the "tmp" variant back to that. But let's give some time for topics in flight to adapt to the new code before doing so (if we do it too soon, the function semantics will change but the compiler won't alert us). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

config: fix leaks from git_config_get_string_const()

2020-08-14T17:52:04Z

There are two functions to get a single config string: - git_config_get_string() - git_config_get_string_const() One might naively think that the first one allocates a new string and the second one just points us to the internal configset storage. But in fact they both allocate a new copy; the second one exists only to avoid having to cast when using it with a const global which we never intend to free. The documentation for the function explains that clearly, but it seems I'm not alone in being surprised by this. Of 17 calls to the function, 13 of them leak the resulting value. We could obviously fix these by adding the appropriate free(). But it would be simpler still if we actually had a non-allocating way to get the string. There's git_config_get_value() but that doesn't quite do what we want. If the config key is present but is a boolean with no value (e.g., "[foo]bar" in the file), then we'll get NULL (whereas the string versions will print an error and die). So let's introduce a new variant, git_config_get_string_tmp(), that behaves as these callers expect. We need a new name because we have new semantics but the same function signature (so even if we converted the four remaining callers, topics in flight might be surprised). The "tmp" is because this value should only be held onto for a short time. In practice it's rare for us to clear and refresh the configset, invalidating the pointer, but hopefully the "tmp" makes callers think about the lifetime. In each of the converted cases here the value only needs to last within the local function or its immediate caller. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

config: work around gcc-10 -Wstringop-overflow warning

2020-08-04T16:31:01Z

Compiling with gcc-10, -O2, and -fsanitize=undefined results in a compiler warning: config.c: In function ‘git_config_copy_or_rename_section_in_file’: config.c:3170:17: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=] 3170 | output[0] = '\t'; | ~~~~~~~~~~^~~~~~ config.c:3076:7: note: at offset -1 to object ‘buf’ with size 1024 declared here 3076 | char buf[1024]; | ^~~ This is a false positive. The interesting lines of code are: int i; char *output = buf; ... for (i = 0; buf[i] && isspace(buf[i]); i++) ; /* do nothing */ ... int offset; offset = section_name_match(&buf[i], old_name); if (offset > 0) { ... output += offset + i; if (strlen(output) > 0) { /* * More content means there's * a declaration to put on the * next line; indent with a * tab */ output -= 1; output[0] = '\t'; } } So we do assign output to buf initially. Later we increment it based on "offset" and "i" and then subtract "1" from it. That latter step is what the compiler is complaining about; it could lead to going off the left side of the array if "output == buf" at the moment of the subtraction. For that to be the case, then "offset + i" would have to be 0. But that can't happen: - we know that "offset" is at least 1, since we're in a conditional block that checks that - we know that "i" is not negative, since it started at 0 and only incremented over whitespace So the sum must be at least 1, and therefore it's OK to subtract one from "output". But that's not quite the whole story. Since "i" is an int, it could in theory be possible to overflow to negative (when counting whitespace on a very large string). But we know that's impossible because we're counting the 1024-byte buffer we just fed to fgets(), so it can never be larger than that. Switching the type of "i" to "unsigned" makes the warning go away, so let's do that. Arguably size_t is an even better type (for this and for the other length fields), but switching to it produces a similar but distinct warning: config.c: In function ‘git_config_copy_or_rename_section_in_file’: config.c:3170:13: error: array subscript -1 is outside array bounds of ‘char[1024]’ [-Werror=array-bounds] 3170 | output[0] = '\t'; | ~~~~~~^~~ config.c:3076:7: note: while referencing ‘buf’ 3076 | char buf[1024]; | ^~~ If we were to ever switch off of fgets() to strbuf_getline() or similar, we'd probably need to use size_t to avoid other overflow problems. But for now we know we're safe because of the small fixed size of our buffer. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

config: reject parsing of files over INT_MAX

2020-04-10T21:58:21Z

While the last few commits have made it possible for the config parser to handle config files up to the limits of size_t, the rest of the code isn't really ready for this. In particular, we often feed the keys as strings into printf "%s" format specifiers. And because the printf family of functions must return an int to specify the result, they complain. Here are two concrete examples (using glibc; we're in uncharted territory here so results may vary): Generate a gigantic .gitmodules file like this: git submodule add /some/other/repo foo { printf '[submodule "' perl -e 'print "a" x 2**31' echo '"]path = foo' } >.gitmodules git commit -m 'huge gitmodule' then try this: $ git show BUG: strbuf.c:397: your vsnprintf is broken (returned -1) The problem is that we end up calling: strbuf_addf(&sb, "submodule.%s.ignore", submodule_name); which relies on vsnprintf(), and that function has no way to report back a size larger than INT_MAX. Taking that same file, try this: git config --file=.gitmodules --list --name-only On my system it produces an output with exactly 4GB of spaces. I confirmed in a debugger that we reach the config callback with the key intact: it's 2147483663 bytes and full of a's. But when we print it with this call: printf("%s%c", key_, term); we just get the spaces. So given the fact that these are insane cases which we have no need to support, the weird behavior from feeding the results to printf even if the code is careful, and the possibility of uncareful code introducing its own integer truncation issues, let's just declare INT_MAX as a limit for parsing config files. We'll enforce the limit in get_next_char(), which generalizes over all sources (blobs, files, etc) and covers any element we're parsing (whether section, key, value, etc). For simplicity, the limit is over the length of the _whole_ file, so you couldn't have two 1GB values in the same file. This should be perfectly fine, as the expected size for config files is generally kilobytes at most. With this patch both cases above will yield: fatal: bad config line 1 in file .gitmodules That's not an amazing error message, but the parser isn't set up to provide specific messages (it just breaks out of the parsing loop and gives that generic error even if see a syntactic issue). And we really wouldn't expect to see this case outside of somebody maliciously probing the limits of the config system. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

config: use size_t to store parsed variable baselen

2020-04-10T21:56:45Z

Most of the config parsing infrastructure is limited in what it can parse only by the size of memory, because it parses character by character, building up strbufs for keys, values, etc. One exception is the "baselen" value we keep in git_parse_source(), which is an int. That stores the length of the section.subsection base, to which we can then append individual key names (by truncating back to the baselen with strbuf_setlen(), and then appending characters for the key name). But because it's an int, if we see an absurdly long section or subsection, we may overflow the integer, wrapping negative. That negative value is then implicitly cast to a size_t when we pass it to strbuf_setlen(), creating a very large value and triggering a BUG. For example: $ { printf '[foo "' perl -e 'print "a" x 2**31' echo '"]bar = value' } >huge $ git config --file=huge --list fatal: BUG: strbuf_setlen() beyond buffer While this is obviously a silly case that we don't care about supporting, it's worth fixing it by switching to a size_t for a few reasons: - we should try to avoid hitting BUG assertions at all - avoiding integer truncation or overflow sets a good example and makes it easier to audit the code for more important issues - the BUG outcome is what happens in _this_ instance, because we wrap negative. If we used a 2**32 subsection, we'd wrap to a small positive value and actually generate wrong output (the subsection of our key would be truncated). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

git_config_parse_key(): return baselen as size_t

2020-04-10T21:52:22Z

As with the recent change to parse_config_key(), the best type to return a string length is a size_t, as it won't cause integer truncation for a gigantic key. And as with that change, this is mostly a clarity / hygiene issue for now, as our config parser would choke on such a large key anyway. There are a few ripple effects within the config code, as callers switch to using size_t. I also adjusted a few related variables that iterate over strings. The most unexpected change is that a call to strbuf_addf() had to switch to strbuf_add(). We can't use a size_t with "%.*s", because printf precisions must have type "int" (we could cast, of course, but that would miss the point of using size_t in the first place). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

config: drop useless length variable in write_pair()

2020-04-10T21:47:36Z

We compute the length of a subset of a string, but then use that length only to feed a "%.*s" printf placeholder for the same string. We can just use "%s" to achieve the same thing. The variable became useless in cb891a5989 (Use a strbuf for building up section header and key/value pair strings., 2007-12-14), which swapped out a write() which _did_ use the length for a strbuf_addf() call. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

parse_config_key(): return subsection len as size_t

2020-04-10T21:44:29Z

We return the length to a subset of a string using an "int *" out-parameter. This is fine most of the time, as we'd expect config keys to be relatively short, but it could behave oddly if we had a gigantic config key. A more appropriate type is size_t. Let's switch over, which lets our callers use size_t as appropriate (they are bound by our type because they must pass the out-parameter as a pointer). This is mostly just a cleanup to make it clear this code handles long strings correctly. In practice, our config parser already chokes on long key names (because of a similar int/size_t mixup!). When doing an int/size_t conversion, we have to be careful that nobody was trying to assign a negative value to the variable. I manually confirmed that for each case here. They tend to just feed the result to xmemdupz() or similar; in a few cases I adjusted the parameter types for helper functions to make sure the size_t is preserved. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano