[PATCH 0/12] git_config_string() considered harmful
- From
Jeff King <peff@peff.net>
- Date
- Apr 7, 2024, 00:56 UTC
- Message-ID
- <20240407005656.GA436890@coredump.intra.peff.net>
- In-Reply-To
- <xmqqttkeicov.fsf@gitster.g>
On Sat, Apr 06, 2024 at 11:11:12AM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> The excludes_file variable is marked "const char *", but all the > assignments to it are made with a piece of memory allocated just > for it, and the variable is responsible for owning it. > > When "core.excludesfile" is read, the code just lost the previous > value, leaking memory. Plug it. > > The real problem is that the variable is mistyped; our convention > is to never make a variable that owns the piece of memory pointed > by it as "const". Fixing that would reduce the chance of this kind > of bug happening, and also would make it unnecessary to cast the > constness away while free()ing it, but that would be a much larger > follow-up effort.
As you noticed in your follow-up, this is just the tip of the iceberg. And it's not just git_config_pathname(), but really git_config_string(), and it is a potential problem for almost every call.
I have a series that I started a few months ago to try to improve this, but I never sent it in because I didn't have a good solution for the long tail of variables where we assign a string literal as the default.
But that doesn't mean we can't incrementally make things better. So I polished it up a bit, and will send the result in a minute.
Looking at your sketch, I think I glossed over the parse-options OPT_FILENAME_DUP() issue. In practice it's OK because we wouldn't generally re-read the config after parsing the options. But leaving it does seem rather ugly, and your solution looks reasonable. I'm not sure if there's an easy way to get the compiler to point to spots which need it; the type information is all lost when parse-options passes everything through a void pointer.
(I remember a while ago looking at retaining type annotations for parse-options; this could be another use case for that).
I think it would also be useful if we could enable -Wwrite-strings to catch cases where string literals are assigned to non-const pointers. But there's some cleanup/refactoring to get that to compile cleanly.
[01/11]: config: make sequencer.c's git_config_string_dup() public [02/11]: config: add git_config_pathname_dup() [03/11]: config: prefer git_config_string_dup() for temp variables [04/11]: config: use git_config_string_dup() for open-coded equivalents [05/11]: config: use git_config_string_dup() to fix leaky open coding [06/11]: config: use git_config_string() in easy cases [07/11]: config: use git_config_pathname_dup() in easy cases [08/11]: http: use git_config_string_dup() [09/11]: merge: use git_config_string_dup() for pull strategies [10/11]: userdiff: use git_config_string_dup() when we can [11/11]: blame: use "dup" string_list for ignore-revs files
alias.c | 3 +- archive-tar.c | 10 ++-- attr.c | 2 +- attr.h | 2 +- builtin/blame.c | 7 +-- builtin/commit.c | 8 ++-- builtin/config.c | 6 +-- builtin/help.c | 7 +-- builtin/log.c | 16 +++---- builtin/merge.c | 12 ++--- builtin/receive-pack.c | 10 ++-- builtin/repack.c | 16 +++---- compat/mingw.c | 7 +-- config.c | 48 ++++++++++++------- config.h | 19 ++++++++ convert.c | 12 ++--- delta-islands.c | 4 +- diff.c | 12 ++--- environment.c | 14 +++--- environment.h | 14 +++--- fetch-pack.c | 6 +-- fsck.c | 6 +-- gpg-interface.c | 9 ++-- http.c | 105 +++++++++++++++++++---------------------- imap-send.c | 20 ++++---- mailmap.c | 4 +- mailmap.h | 4 +- merge-ll.c | 17 +++---- pager.c | 4 +- promisor-remote.c | 2 +- promisor-remote.h | 2 +- remote.c | 45 +++++++++--------- remote.h | 8 ++-- sequencer.c | 12 +---- setup.c | 11 ++--- upload-pack.c | 4 +- userdiff.c | 6 +-- userdiff.h | 6 +-- 38 files changed, 251 insertions(+), 249 deletions(-)
-Peff