{"thread":{"id":"61283","subject":"[PATCH] config: do not leak excludes_file","startedAt":"2024-04-06T18:11:17Z","lastAt":"2024-04-11T23:36:49Z","messageCount":23,"participants":["Junio C Hamano","Jeff King","Eric Sunshine","Rubén Justo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492403","messageId":"xmqqttkeicov.fsf@gitster.g","threadId":"61283","inReplyTo":null,"subject":"[PATCH] config: do not leak excludes_file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T18:11:12Z","receivedAt":"2024-04-06T18:11:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The excludes_file variable is marked \"const char *\", but all the\nassignments to it are made with a piece of memory allocated just\nfor it, and the variable is responsible for owning it.\n\nWhen \"core.excludesfile\" is read, the code just lost the previous\nvalue, leaking memory.  Plug it.\n\nThe real problem is that the variable is mistyped; our convention\nis to never make a variable that owns the piece of memory pointed\nby it as \"const\".  Fixing that would reduce the chance of this kind\nof bug happening, and also would make it unnecessary to cast the\nconstness away while free()ing it, but that would be a much larger\nfollow-up effort.\n\nReported-by: Rubén Justo <rjusto@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n config.c         | 4 +++-\n t/t7300-clean.sh | 1 +\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex eebce8c7e0..ae3652b08f 100644\n--- a/config.c\n+++ b/config.c\n@@ -1584,8 +1584,10 @@ static int git_default_core_config(const char *var, const char *value,\n \tif (!strcmp(var, \"core.askpass\"))\n \t\treturn git_config_string(&askpass_program, var, value);\n \n-\tif (!strcmp(var, \"core.excludesfile\"))\n+\tif (!strcmp(var, \"core.excludesfile\")) {\n+\t\tfree((char *)excludes_file);\n \t\treturn git_config_pathname(&excludes_file, var, value);\n+\t}\n \n \tif (!strcmp(var, \"core.whitespace\")) {\n \t\tif (!value)\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 1f7201eb60..0aae0dee67 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -5,6 +5,7 @@\n \n test_description='git clean basic tests'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n git config clean.requireForce no\n-- \n2.44.0-501-g19981daefd\n\n"},{"id":"492404","messageId":"xmqqa5m6i9n3.fsf@gitster.g","threadId":"61283","inReplyTo":"xmqqttkeicov.fsf@gitster.g","subject":"[WIP] git_config_pathname() leakfix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T19:17:04Z","receivedAt":"2024-04-06T19:17:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The git_config_pathname() function takes a pointer to \"const char *\"\nthat specifies where to store the resulting pathname string, but\nalways assigns a newly assigned value to it.  The caller has to cast\nthe constness away if it wants to call free() on it, e.g.,\n\n\tconst char *str;\n\tint ret = git_config_pathname(&str, var, value);\n\tuse(str)\n\tfree((const char *)str);\n\nHere is a not-working WIP just to show the scale of fallout from an\nexercise to plug leaks of returned value from git_config_pathname().\nIt revealed a few more issues around memory ownership rules.\n\n * OPTION_FILENAME of parse-options API assigns the string from the\n   command line argument directly to the supplied variable.  The\n   variable obviously does not own the value and cannot and does not\n   have to free() it.  It however is very common to use the same\n   variable to also hold a string read from the config API, via\n   git_config_pathname(), which wants the caller to own the string.\n\n   In this sketch, I attempted to introduce OPTION_FILENAME_DUP and\n   have the parse-options API to treat the destination variable as\n   owning the string (i.e. free the existing contents before\n   assigning the value from the command line, and xstrdup() the\n   string from the command line option), but I am sure I missed\n   some (which will lead to segfault when a caller later tries\n   to free() it before calling git_config_pathname()) and/or \n   converted too many (which will lead to leaks when a caller\n   later does not free() it).\n\n * http.c has a private set_from_env() helper function, which lets\n   the callers to borrow memory from the environment.  But some\n   variables that receive value from this mechanism also receive\n   pathnames from configuration via git_config_pathname().\n\n   In this sketch, I attempted to solve it by introducing\n   dup_from_env() and changed callers that wants to overwrite the\n   variable with a value obtained from git_config_pathname().\n\nSome readers might wonder if it is easier to go in the other\ndirection, i.e. allowing cllaers of git_config_pathname() to\nborrow and not worry about freeing, but this is fundamentally\nimpossible as expanding ~/.gitconfig into /home/jch/.gitconfig\nand the like is the central part of its function.\n\nIn any case, I am not going to finish this soonish, but I think I\nfound some existing leaks that can be fixed _without_ going through\nthe whole nine yards, so perhaps I'll see if I can salvage small\nchanges like that as \"preliminary clean-up\".\n\n builtin/blame.c        |  3 ++-\n builtin/commit.c       |  4 ++--\n builtin/config.c       |  4 ++--\n builtin/log.c          |  4 ++--\n builtin/receive-pack.c |  4 ++--\n config.c               |  8 ++++----\n config.h               |  8 ++++----\n diff.c                 |  2 +-\n environment.c          |  6 +++---\n environment.h          |  6 +++---\n fetch-pack.c           |  4 ++--\n fsck.c                 |  4 ++--\n fsmonitor-settings.c   |  8 ++++++--\n gpg-interface.c        |  3 ++-\n http.c                 | 29 +++++++++++++++++++----------\n mailmap.c              |  2 +-\n mailmap.h              |  2 +-\n parse-options.c        | 14 ++++++++++++--\n parse-options.h        | 11 ++++++++++-\n setup.c                |  8 ++++----\n 20 files changed, 84 insertions(+), 50 deletions(-)\n\ndiff --git c/builtin/blame.c w/builtin/blame.c\nindex db1f56de61..23810323e6 100644\n--- c/builtin/blame.c\n+++ w/builtin/blame.c\n@@ -718,13 +718,14 @@ static int git_blame_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"blame.ignorerevsfile\")) {\n-\t\tconst char *str;\n+\t\tchar *str;\n \t\tint ret;\n \n \t\tret = git_config_pathname(&str, var, value);\n \t\tif (ret)\n \t\t\treturn ret;\n \t\tstring_list_insert(&ignore_revs_file_list, str);\n+\t\tfree(str);\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"blame.markunblamablelines\")) {\ndiff --git c/builtin/commit.c w/builtin/commit.c\nindex 7ba7201cfb..a35c524458 100644\n--- c/builtin/commit.c\n+++ w/builtin/commit.c\n@@ -107,7 +107,7 @@ static enum {\n } commit_style;\n \n static const char *logfile, *force_author;\n-static const char *template_file;\n+static char *template_file;\n /*\n  * The _message variables are commit names from which to take\n  * the commit message and/or authorship.\n@@ -1319,7 +1319,7 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\t\t\t  !!use_message, \"-C\",\n \t\t\t\t  !!logfile, \"-F\");\n \tif (use_message || edit_message || logfile ||fixup_message || have_option_m)\n-\t\ttemplate_file = NULL;\n+\t\tFREE_AND_NULL(template_file);\n \tif (edit_message)\n \t\tuse_message = edit_message;\n \tif (amend && !use_message && !fixup_message)\ndiff --git c/builtin/config.c w/builtin/config.c\nindex 0015620dde..739d3beb57 100644\n--- c/builtin/config.c\n+++ w/builtin/config.c\n@@ -282,11 +282,11 @@ static int format_config(struct strbuf *buf, const char *key_,\n \t\t\telse\n \t\t\t\tstrbuf_addstr(buf, v ? \"true\" : \"false\");\n \t\t} else if (type == TYPE_PATH) {\n-\t\t\tconst char *v;\n+\t\t\tchar *v;\n \t\t\tif (git_config_pathname(&v, key_, value_) < 0)\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addstr(buf, v);\n-\t\t\tfree((char *)v);\n+\t\t\tfree(v);\n \t\t} else if (type == TYPE_EXPIRY_DATE) {\n \t\t\ttimestamp_t t;\n \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\ndiff --git c/builtin/log.c w/builtin/log.c\nindex c0a8bb95e9..e90d3f13e7 100644\n--- c/builtin/log.c\n+++ w/builtin/log.c\n@@ -957,7 +957,7 @@ static int do_signoff;\n static enum auto_base_setting auto_base;\n static char *from;\n static const char *signature = git_version_string;\n-static const char *signature_file;\n+static char *signature_file;\n static enum cover_setting config_cover_letter;\n static const char *config_output_directory;\n static enum cover_from_description cover_from_description_mode = COVER_FROM_MESSAGE;\n@@ -1981,7 +1981,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK_F(0, \"base\", &base_commit, N_(\"base-commit\"),\n \t\t\t       N_(\"add prerequisite tree info to the patch series\"),\n \t\t\t       0, base_callback),\n-\t\tOPT_FILENAME(0, \"signature-file\", &signature_file,\n+\t\tOPT_FILENAME_DUP(0, \"signature-file\", &signature_file,\n \t\t\t\tN_(\"add a signature from a file\")),\n \t\tOPT__QUIET(&quiet, N_(\"don't print the patch filenames\")),\n \t\tOPT_BOOL(0, \"progress\", &show_progress,\ndiff --git c/builtin/receive-pack.c w/builtin/receive-pack.c\nindex 56d8a77ed7..f5e55bad51 100644\n--- c/builtin/receive-pack.c\n+++ w/builtin/receive-pack.c\n@@ -168,13 +168,13 @@ static int receive_pack_config(const char *var, const char *value,\n \t}\n \n \tif (strcmp(var, \"receive.fsck.skiplist\") == 0) {\n-\t\tconst char *path;\n+\t\tchar *path;\n \n \t\tif (git_config_pathname(&path, var, value))\n \t\t\treturn 1;\n \t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n \t\t\tfsck_msg_types.len ? ',' : '=', path);\n-\t\tfree((char *)path);\n+\t\tfree(path);\n \t\treturn 0;\n \t}\n \ndiff --git c/config.c w/config.c\nindex ae3652b08f..683acbe235 100644\n--- c/config.c\n+++ w/config.c\n@@ -1345,7 +1345,7 @@ int git_config_string(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n-int git_config_pathname(const char **dest, const char *var, const char *value)\n+int git_config_pathname(char **dest, const char *var, const char *value)\n {\n \tif (!value)\n \t\treturn config_error_nonbool(var);\n@@ -2482,7 +2482,7 @@ int git_configset_get_maybe_bool(struct config_set *set, const char *key, int *d\n \t\treturn 1;\n }\n \n-int git_configset_get_pathname(struct config_set *set, const char *key, const char **dest)\n+int git_configset_get_pathname(struct config_set *set, const char *key, char **dest)\n {\n \tconst char *value;\n \tif (!git_configset_get_value(set, key, &value, NULL))\n@@ -2627,7 +2627,7 @@ int repo_config_get_maybe_bool(struct repository *repo,\n }\n \n int repo_config_get_pathname(struct repository *repo,\n-\t\t\t     const char *key, const char **dest)\n+\t\t\t     const char *key, char **dest)\n {\n \tint ret;\n \tgit_config_check_init(repo);\n@@ -2726,7 +2726,7 @@ int git_config_get_maybe_bool(const char *key, int *dest)\n \treturn repo_config_get_maybe_bool(the_repository, key, dest);\n }\n \n-int git_config_get_pathname(const char *key, const char **dest)\n+int git_config_get_pathname(const char *key, char **dest)\n {\n \treturn repo_config_get_pathname(the_repository, key, dest);\n }\ndiff --git c/config.h w/config.h\nindex f4966e3749..868057c77f 100644\n--- c/config.h\n+++ w/config.h\n@@ -286,7 +286,7 @@ int git_config_string(const char **, const char *, const char *);\n  * Similar to `git_config_string`, but expands `~` or `~user` into the\n  * user's home directory when found at the beginning of the path.\n  */\n-int git_config_pathname(const char **, const char *, const char *);\n+int git_config_pathname(char **, const char *, const char *);\n \n int git_config_expiry_date(timestamp_t *, const char *, const char *);\n int git_config_color(char *, const char *, const char *);\n@@ -541,7 +541,7 @@ int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned lon\n int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);\n int git_configset_get_bool_or_int(struct config_set *cs, const char *key, int *is_bool, int *dest);\n int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest);\n-int git_configset_get_pathname(struct config_set *cs, const char *key, const char **dest);\n+int git_configset_get_pathname(struct config_set *cs, const char *key, char **dest);\n \n /* Functions for reading a repository's config */\n struct repository;\n@@ -577,7 +577,7 @@ int repo_config_get_bool_or_int(struct repository *repo,\n int repo_config_get_maybe_bool(struct repository *repo,\n \t\t\t       const char *key, int *dest);\n int repo_config_get_pathname(struct repository *repo,\n-\t\t\t     const char *key, const char **dest);\n+\t\t\t     const char *key, char **dest);\n \n /*\n  * Functions for reading protected config. By definition, protected\n@@ -687,7 +687,7 @@ int git_config_get_maybe_bool(const char *key, int *dest);\n  * Similar to `git_config_get_string`, but expands `~` or `~user` into\n  * the user's home directory when found at the beginning of the path.\n  */\n-int git_config_get_pathname(const char *key, const char **dest);\n+int git_config_get_pathname(const char *key, char **dest);\n \n int git_config_get_index_threads(int *dest);\n int git_config_get_split_index(void);\ndiff --git c/diff.c w/diff.c\nindex 108c187577..e8754888f4 100644\n--- c/diff.c\n+++ w/diff.c\n@@ -58,7 +58,7 @@ static int diff_context_default = 3;\n static int diff_interhunk_context_default;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n-static const char *diff_order_file_cfg;\n+static char *diff_order_file_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n static int diff_no_prefix;\ndiff --git c/environment.c w/environment.c\nindex a73ba9c12c..279ea3fd5e 100644\n--- c/environment.c\n+++ w/environment.c\n@@ -46,8 +46,8 @@ const char *git_commit_encoding;\n const char *git_log_output_encoding;\n char *apply_default_whitespace;\n char *apply_default_ignorewhitespace;\n-const char *git_attributes_file;\n-const char *git_hooks_path;\n+char *git_attributes_file;\n+char *git_hooks_path;\n int zlib_compression_level = Z_BEST_SPEED;\n int pack_compression_level = Z_DEFAULT_COMPRESSION;\n int fsync_object_files = -1;\n@@ -60,7 +60,7 @@ size_t delta_base_cache_limit = 96 * 1024 * 1024;\n unsigned long big_file_threshold = 512 * 1024 * 1024;\n const char *editor_program;\n const char *askpass_program;\n-const char *excludes_file;\n+char *excludes_file;\n enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;\n enum eol core_eol = EOL_UNSET;\n int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;\ndiff --git c/environment.h w/environment.h\nindex 05fd94d7be..c0e0c8b1f9 100644\n--- c/environment.h\n+++ w/environment.h\n@@ -124,8 +124,8 @@ extern int warn_ambiguous_refs;\n extern int warn_on_object_refname_ambiguity;\n extern char *apply_default_whitespace;\n extern char *apply_default_ignorewhitespace;\n-extern const char *git_attributes_file;\n-extern const char *git_hooks_path;\n+extern char *git_attributes_file;\n+extern char *git_hooks_path;\n extern int zlib_compression_level;\n extern int pack_compression_level;\n extern size_t packed_git_window_size;\n@@ -222,7 +222,7 @@ extern const char *git_log_output_encoding;\n \n extern const char *editor_program;\n extern const char *askpass_program;\n-extern const char *excludes_file;\n+extern char *excludes_file;\n \n /*\n  * Should we print an ellipsis after an abbreviated SHA-1 value\ndiff --git c/fetch-pack.c w/fetch-pack.c\nindex 091f9a80a9..f7579cb3bc 100644\n--- c/fetch-pack.c\n+++ w/fetch-pack.c\n@@ -1863,13 +1863,13 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n \tconst char *msg_id;\n \n \tif (strcmp(var, \"fetch.fsck.skiplist\") == 0) {\n-\t\tconst char *path;\n+\t\tchar *path;\n \n \t\tif (git_config_pathname(&path, var, value))\n \t\t\treturn 1;\n \t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n \t\t\tfsck_msg_types.len ? ',' : '=', path);\n-\t\tfree((char *)path);\n+\t\tfree(path);\n \t\treturn 0;\n \t}\n \ndiff --git c/fsck.c w/fsck.c\nindex 78af29d264..dd0a33028e 100644\n--- c/fsck.c\n+++ w/fsck.c\n@@ -1274,13 +1274,13 @@ int git_fsck_config(const char *var, const char *value,\n \tconst char *msg_id;\n \n \tif (strcmp(var, \"fsck.skiplist\") == 0) {\n-\t\tconst char *path;\n+\t\tchar *path;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tif (git_config_pathname(&path, var, value))\n \t\t\treturn 1;\n \t\tstrbuf_addf(&sb, \"skiplist=%s\", path);\n-\t\tfree((char *)path);\n+\t\tfree(path);\n \t\tfsck_set_msg_types(options, sb.buf);\n \t\tstrbuf_release(&sb);\n \t\treturn 0;\ndiff --git c/fsmonitor-settings.c w/fsmonitor-settings.c\nindex a6a9e6bc19..20e9347907 100644\n--- c/fsmonitor-settings.c\n+++ w/fsmonitor-settings.c\n@@ -129,8 +129,12 @@ static void lookup_fsmonitor_settings(struct repository *r)\n \t\tbreak;\n \n \tcase -1: /* config value set to an arbitrary string */\n-\t\tif (repo_config_get_pathname(r, \"core.fsmonitor\", &const_str))\n-\t\t\treturn; /* should not happen */\n+\t\t{\n+\t\t\tchar *str;\n+\t\t\tif (repo_config_get_pathname(r, \"core.fsmonitor\", &str))\n+\t\t\t\treturn; /* should not happen */\n+\t\t\tconst_str = str;\n+\t\t}\n \t\tbreak;\n \n \tdefault: /* should not happen */\ndiff --git c/gpg-interface.c w/gpg-interface.c\nindex b5993385ff..87e214c76e 100644\n--- c/gpg-interface.c\n+++ w/gpg-interface.c\n@@ -27,7 +27,8 @@ static void gpg_interface_lazy_init(void)\n }\n \n static char *configured_signing_key;\n-static const char *ssh_default_key_command, *ssh_allowed_signers, *ssh_revocation_file;\n+static const char *ssh_default_key_command;\n+static char *ssh_allowed_signers, *ssh_revocation_file;\n static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;\n \n struct gpg_format {\ndiff --git c/http.c w/http.c\nindex e73b136e58..b03a9a169d 100644\n--- c/http.c\n+++ w/http.c\n@@ -39,7 +39,7 @@ char curl_errorstr[CURL_ERROR_SIZE];\n static int curl_ssl_verify = -1;\n static int curl_ssl_try;\n static const char *curl_http_version = NULL;\n-static const char *ssl_cert;\n+static char *ssl_cert;\n static const char *ssl_cert_type;\n static const char *ssl_cipherlist;\n static const char *ssl_version;\n@@ -59,14 +59,14 @@ static struct {\n \t{ \"tlsv1.3\", CURL_SSLVERSION_TLSv1_3 },\n #endif\n };\n-static const char *ssl_key;\n+static char *ssl_key;\n static const char *ssl_key_type;\n-static const char *ssl_capath;\n+static char *ssl_capath;\n static const char *curl_no_proxy;\n #ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY\n-static const char *ssl_pinnedkey;\n+static char *ssl_pinnedkey;\n #endif\n-static const char *ssl_cainfo;\n+static char *ssl_cainfo;\n static long curl_low_speed_limit = -1;\n static long curl_low_speed_time = -1;\n static int curl_ftp_no_epsv;\n@@ -108,7 +108,7 @@ static struct {\n \n static struct credential proxy_auth = CREDENTIAL_INIT;\n static const char *curl_proxyuserpwd;\n-static const char *curl_cookie_file;\n+static char *curl_cookie_file;\n static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n static int http_proactive_auth;\n@@ -1215,6 +1215,15 @@ static void set_from_env(const char **var, const char *envname)\n \t\t*var = val;\n }\n \n+static void dup_from_env(char **var, const char *envname)\n+{\n+\tconst char *val = getenv(envname);\n+\tif (val) {\n+\t\tfree(*var);\n+\t\t*var = strdup(val);\n+\t}\n+}\n+\n void http_init(struct remote *remote, const char *url, int proactive_auth)\n {\n \tchar *low_speed_limit;\n@@ -1291,12 +1300,12 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \tif (getenv(\"GIT_SSL_NO_VERIFY\"))\n \t\tcurl_ssl_verify = 0;\n \n-\tset_from_env(&ssl_cert, \"GIT_SSL_CERT\");\n+\tdup_from_env(&ssl_cert, \"GIT_SSL_CERT\");\n \tset_from_env(&ssl_cert_type, \"GIT_SSL_CERT_TYPE\");\n-\tset_from_env(&ssl_key, \"GIT_SSL_KEY\");\n+\tdup_from_env(&ssl_key, \"GIT_SSL_KEY\");\n \tset_from_env(&ssl_key_type, \"GIT_SSL_KEY_TYPE\");\n-\tset_from_env(&ssl_capath, \"GIT_SSL_CAPATH\");\n-\tset_from_env(&ssl_cainfo, \"GIT_SSL_CAINFO\");\n+\tdup_from_env(&ssl_capath, \"GIT_SSL_CAPATH\");\n+\tdup_from_env(&ssl_cainfo, \"GIT_SSL_CAINFO\");\n \n \tset_from_env(&user_agent, \"GIT_HTTP_USER_AGENT\");\n \ndiff --git c/mailmap.c w/mailmap.c\nindex 3d6a5e9400..044466b043 100644\n--- c/mailmap.c\n+++ w/mailmap.c\n@@ -6,7 +6,7 @@\n #include \"object-store-ll.h\"\n #include \"setup.h\"\n \n-const char *git_mailmap_file;\n+char *git_mailmap_file;\n const char *git_mailmap_blob;\n \n struct mailmap_info {\ndiff --git c/mailmap.h w/mailmap.h\nindex 0f8fd2c586..429a760945 100644\n--- c/mailmap.h\n+++ w/mailmap.h\n@@ -3,7 +3,7 @@\n \n struct string_list;\n \n-extern const char *git_mailmap_file;\n+extern char *git_mailmap_file;\n extern const char *git_mailmap_blob;\n \n int read_mailmap(struct string_list *map);\ndiff --git c/parse-options.c w/parse-options.c\nindex 30b9e68f8a..94c1e52b60 100644\n--- c/parse-options.c\n+++ w/parse-options.c\n@@ -64,8 +64,11 @@ static void fix_filename(const char *prefix, char **file)\n {\n \tif (!file || !*file)\n \t\t; /* leave as NULL */\n-\telse\n-\t\t*file = prefix_filename_except_for_dash(prefix, *file);\n+\telse {\n+\t\tchar *tmp = prefix_filename_except_for_dash(prefix, *file);\n+\t\tfree(*file);\n+\t\t*file = tmp;\n+\t}\n }\n \n static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n@@ -128,6 +131,9 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\t\treturn get_arg(p, opt, flags, (const char **)opt->value);\n \t\treturn 0;\n \n+\tcase OPTION_FILENAME_DUP:\n+\t\tFREE_AND_NULL(*(char **)opt->value);\n+\t\t/* fallthru */\n \tcase OPTION_FILENAME:\n \t\terr = 0;\n \t\tif (unset)\n@@ -137,6 +143,8 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\telse\n \t\t\terr = get_arg(p, opt, flags, (const char **)opt->value);\n \n+\t\tif (opt->type == OPTION_FILENAME_DUP)\n+\t\t\t*(char **)opt->value = xstrdup_or_null(*(const char **)opt->value);\n \t\tif (!err)\n \t\t\tfix_filename(p->prefix, (char **)opt->value);\n \t\treturn err;\n@@ -649,6 +657,7 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,\n \t\tswitch (opts->type) {\n \t\tcase OPTION_STRING:\n \t\tcase OPTION_FILENAME:\n+\t\tcase OPTION_FILENAME_DUP:\n \t\tcase OPTION_INTEGER:\n \t\tcase OPTION_MAGNITUDE:\n \t\tcase OPTION_CALLBACK:\n@@ -701,6 +710,7 @@ static int show_gitcomp(const struct option *opts, int show_all)\n \t\t\tcontinue;\n \t\tcase OPTION_STRING:\n \t\tcase OPTION_FILENAME:\n+\t\tcase OPTION_FILENAME_DUP:\n \t\tcase OPTION_INTEGER:\n \t\tcase OPTION_MAGNITUDE:\n \t\tcase OPTION_CALLBACK:\ndiff --git c/parse-options.h w/parse-options.h\nindex bd62e20268..e78f72f986 100644\n--- c/parse-options.h\n+++ w/parse-options.h\n@@ -26,7 +26,8 @@ enum parse_opt_type {\n \tOPTION_MAGNITUDE,\n \tOPTION_CALLBACK,\n \tOPTION_LOWLEVEL_CALLBACK,\n-\tOPTION_FILENAME\n+\tOPTION_FILENAME,\n+\tOPTION_FILENAME_DUP\n };\n \n enum parse_opt_flags {\n@@ -330,6 +331,14 @@ struct option {\n \t.argh = N_(\"file\"), \\\n \t.help = (h), \\\n }\n+#define OPT_FILENAME_DUP(s, l, v, h) { \\\n+\t.type = OPTION_FILENAME_DUP, \\\n+\t.short_name = (s), \\\n+\t.long_name = (l), \\\n+\t.value = (v), \\\n+\t.argh = N_(\"file\"), \\\n+\t.help = (h), \\\n+}\n #define OPT_COLOR_FLAG(s, l, v, h) { \\\n \t.type = OPTION_CALLBACK, \\\n \t.short_name = (s), \\\ndiff --git c/setup.c w/setup.c\nindex f4b32f76e3..d99856ac81 100644\n--- c/setup.c\n+++ w/setup.c\n@@ -1176,13 +1176,13 @@ static int safe_directory_cb(const char *key, const char *value,\n \t} else if (!strcmp(value, \"*\")) {\n \t\tdata->is_safe = 1;\n \t} else {\n-\t\tconst char *interpolated = NULL;\n+\t\tchar *interpolated = NULL;\n \n \t\tif (!git_config_pathname(&interpolated, key, value) &&\n \t\t    !fspathcmp(data->path, interpolated ? interpolated : value))\n \t\t\tdata->is_safe = 1;\n \n-\t\tfree((char *)interpolated);\n+\t\tfree(interpolated);\n \t}\n \n \treturn 0;\n@@ -2023,7 +2023,7 @@ static int create_default_files(const char *template_path,\n \tchar *path;\n \tint reinit;\n \tint filemode;\n-\tconst char *init_template_dir = NULL;\n+\tchar *init_template_dir = NULL;\n \tconst char *work_tree = get_git_work_tree();\n \n \t/*\n@@ -2037,7 +2037,7 @@ static int create_default_files(const char *template_path,\n \t */\n \tgit_config_get_pathname(\"init.templatedir\", &init_template_dir);\n \tcopy_templates(template_path, init_template_dir);\n-\tfree((char *)init_template_dir);\n+\tfree(init_template_dir);\n \tgit_config_clear();\n \treset_shared_repository();\n \tgit_config(git_default_config, NULL);\n"},{"id":"492409","messageId":"20240407005656.GA436890@coredump.intra.peff.net","threadId":"61283","inReplyTo":"xmqqttkeicov.fsf@gitster.g","subject":"[PATCH 0/12] git_config_string() considered harmful","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T00:56:56Z","receivedAt":"2024-04-07T00:57:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 06, 2024 at 11:11:12AM -0700, Junio C Hamano wrote:\n\n> The excludes_file variable is marked \"const char *\", but all the\n> assignments to it are made with a piece of memory allocated just\n> for it, and the variable is responsible for owning it.\n> \n> When \"core.excludesfile\" is read, the code just lost the previous\n> value, leaking memory.  Plug it.\n> \n> The real problem is that the variable is mistyped; our convention\n> is to never make a variable that owns the piece of memory pointed\n> by it as \"const\".  Fixing that would reduce the chance of this kind\n> of bug happening, and also would make it unnecessary to cast the\n> constness away while free()ing it, but that would be a much larger\n> follow-up effort.\n\nAs you noticed in your follow-up, this is just the tip of the iceberg.\nAnd it's not just git_config_pathname(), but really git_config_string(),\nand it is a potential problem for almost every call.\n\nI have a series that I started a few months ago to try to improve this,\nbut I never sent it in because I didn't have a good solution for the\nlong tail of variables where we assign a string literal as the default.\n\nBut that doesn't mean we can't incrementally make things better. So I\npolished it up a bit, and will send the result in a minute.\n\nLooking at your sketch, I think I glossed over the parse-options\nOPT_FILENAME_DUP() issue. In practice it's OK because we wouldn't\ngenerally re-read the config after parsing the options. But leaving it\ndoes seem rather ugly, and your solution looks reasonable. I'm not sure\nif there's an easy way to get the compiler to point to spots which need\nit; the type information is all lost when parse-options passes\neverything through a void pointer.\n\n(I remember a while ago looking at retaining type annotations for\nparse-options; this could be another use case for that).\n\nI think it would also be useful if we could enable -Wwrite-strings to\ncatch cases where string literals are assigned to non-const pointers.\nBut there's some cleanup/refactoring to get that to compile cleanly.\n\n  [01/11]: config: make sequencer.c's git_config_string_dup() public\n  [02/11]: config: add git_config_pathname_dup()\n  [03/11]: config: prefer git_config_string_dup() for temp variables\n  [04/11]: config: use git_config_string_dup() for open-coded equivalents\n  [05/11]: config: use git_config_string_dup() to fix leaky open coding\n  [06/11]: config: use git_config_string() in easy cases\n  [07/11]: config: use git_config_pathname_dup() in easy cases\n  [08/11]: http: use git_config_string_dup()\n  [09/11]: merge: use git_config_string_dup() for pull strategies\n  [10/11]: userdiff: use git_config_string_dup() when we can\n  [11/11]: blame: use \"dup\" string_list for ignore-revs files\n\n alias.c                |   3 +-\n archive-tar.c          |  10 ++--\n attr.c                 |   2 +-\n attr.h                 |   2 +-\n builtin/blame.c        |   7 +--\n builtin/commit.c       |   8 ++--\n builtin/config.c       |   6 +--\n builtin/help.c         |   7 +--\n builtin/log.c          |  16 +++----\n builtin/merge.c        |  12 ++---\n builtin/receive-pack.c |  10 ++--\n builtin/repack.c       |  16 +++----\n compat/mingw.c         |   7 +--\n config.c               |  48 ++++++++++++-------\n config.h               |  19 ++++++++\n convert.c              |  12 ++---\n delta-islands.c        |   4 +-\n diff.c                 |  12 ++---\n environment.c          |  14 +++---\n environment.h          |  14 +++---\n fetch-pack.c           |   6 +--\n fsck.c                 |   6 +--\n gpg-interface.c        |   9 ++--\n http.c                 | 105 +++++++++++++++++++----------------------\n imap-send.c            |  20 ++++----\n mailmap.c              |   4 +-\n mailmap.h              |   4 +-\n merge-ll.c             |  17 +++----\n pager.c                |   4 +-\n promisor-remote.c      |   2 +-\n promisor-remote.h      |   2 +-\n remote.c               |  45 +++++++++---------\n remote.h               |   8 ++--\n sequencer.c            |  12 +----\n setup.c                |  11 ++---\n upload-pack.c          |   4 +-\n userdiff.c             |   6 +--\n userdiff.h             |   6 +--\n 38 files changed, 251 insertions(+), 249 deletions(-)\n\n-Peff\n"},{"id":"492410","messageId":"20240407005831.GA868358@coredump.intra.peff.net","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"[PATCH 01/11] config: make sequencer.c's git_config_string_dup() public","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T00:58:31Z","receivedAt":"2024-04-07T00:58:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Just about every caller of git_config_string() has a possible leak in\nit: if we parse a config variable twice, it will overwrite the pointer\nthat was allocated the first time, leaving the memory unreferenced.\n\nUnfortunately we can't just fix this directly in git_config_string().\nSome callers do something like:\n\n   const char *foo = \"default_value\";\n   ...\n   git_config_string(&foo, var, value);\n\nAnd we must _not_ free that initial value, as it's a string literal. We\ncan't help those cases easily, as there's no way to distinguish a\nheap-allocated variable from the default one. But let's start by at\nleast providing a helper that avoids the leak. That will let us convert\nsome cases right away, and give us one potential path forward for the\nmore complex ones.\n\nIt turns out we already have such a helper, courtesy of 03a4e260e2\n(sequencer: plug memory leaks for the option values, 2016-10-21). The\nproblem is more acute in sequencer.c, which may load config multiple\ntimes. Hence the solution was limited to that file back then. But this\nreally is a more general problem within our config callbacks.\n\nNote that the new helper takes a \"char *\" rather than a const pointer.\nThis is more appropriate, since we tend to use \"const\" as a signal for\na lack of memory ownership (and this function is most certainly\nasserting ownership over the pointed-to memory).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n config.c    |  9 +++++++++\n config.h    | 12 ++++++++++++\n sequencer.c | 10 ----------\n 3 files changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex eebce8c7e0..2194fb078a 100644\n--- a/config.c\n+++ b/config.c\n@@ -1345,6 +1345,15 @@ int git_config_string(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+int git_config_string_dup(char **dest, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tfree(*dest);\n+\t*dest = xstrdup(value);\n+\treturn 0;\n+}\n+\n int git_config_pathname(const char **dest, const char *var, const char *value)\n {\n \tif (!value)\ndiff --git a/config.h b/config.h\nindex f4966e3749..cdffc14ccf 100644\n--- a/config.h\n+++ b/config.h\n@@ -279,9 +279,21 @@ int git_config_bool(const char *, const char *);\n /**\n  * Allocates and copies the value string into the `dest` parameter; if no\n  * string is given, prints an error message and returns -1.\n+ *\n+ * Note that this function does _not_ free the memory referenced by the\n+ * destination pointer. This makes it safe to use on a variable that initially\n+ * points to a string literal, but it also means that it leaks if the config\n+ * option is seen multiple times.\n  */\n int git_config_string(const char **, const char *, const char *);\n \n+/**\n+ * Like git_config_string(), but frees any previously-allocated\n+ * string at the destination pointer, avoiding a leak when a\n+ * config variable is seen multiple times.\n+ */\n+int git_config_string_dup(char **, const char *, const char *);\n+\n /**\n  * Similar to `git_config_string`, but expands `~` or `~user` into the\n  * user's home directory when found at the beginning of the path.\ndiff --git a/sequencer.c b/sequencer.c\nindex 2c19846385..3e5d82e0e5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2920,16 +2920,6 @@ static int read_populate_todo(struct repository *r,\n \treturn 0;\n }\n \n-static int git_config_string_dup(char **dest,\n-\t\t\t\t const char *var, const char *value)\n-{\n-\tif (!value)\n-\t\treturn config_error_nonbool(var);\n-\tfree(*dest);\n-\t*dest = xstrdup(value);\n-\treturn 0;\n-}\n-\n static int populate_opts_cb(const char *key, const char *value,\n \t\t\t    const struct config_context *ctx,\n \t\t\t    void *data)\n-- \n2.44.0.872.g288abe5b5b\n\n"},{"id":"492411","messageId":"20240407010008.GB868358@coredump.intra.peff.net","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"[PATCH 02/11] config: add git_config_pathname_dup()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:00:08Z","receivedAt":"2024-04-07T01:00:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The git_config_pathname() function suffers the same potential leak issue\nas git_config_string(), since it is basically the same thing but with\nthe added twist of interpolating the path rather than just duplicating\nthe value.\n\nLet's provide a similar \"dup()\" variant to help call sites transition to\nusing the leak-free variant.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n config.c | 11 +++++++++++\n config.h |  7 +++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 2194fb078a..a0aa45abd5 100644\n--- a/config.c\n+++ b/config.c\n@@ -1364,6 +1364,17 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+int git_config_pathname_dup(char **dest, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tfree(*dest);\n+\t*dest = interpolate_path(value, 0);\n+\tif (!*dest)\n+\t\tdie(_(\"failed to expand user dir in: '%s'\"), value);\n+\treturn 0;\n+}\n+\n int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *value)\n {\n \tif (!value)\ndiff --git a/config.h b/config.h\nindex cdffc14ccf..fed21d3144 100644\n--- a/config.h\n+++ b/config.h\n@@ -300,6 +300,13 @@ int git_config_string_dup(char **, const char *, const char *);\n  */\n int git_config_pathname(const char **, const char *, const char *);\n \n+/**\n+ * Like git_config_pathname(), but frees any previously-allocated\n+ * string at the destination pointer, avoiding a leak when a\n+ * config variable is seen multiple times.\n+ */\n+int git_config_pathname_dup(char **, const char *, const char *);\n+\n int git_config_expiry_date(timestamp_t *, const char *, const char *);\n int git_config_color(char *, const char *, const char *);\n int git_config_set_in_file_gently(const char *, const char *, const char *, const char *);\n-- \n2.44.0.872.g288abe5b5b\n\n"},{"id":"492412","messageId":"20240407010037.GC868358@coredump.intra.peff.net","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"[PATCH 03/11] config: prefer git_config_string_dup() for temp variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:00:37Z","receivedAt":"2024-04-07T01:00:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In some cases we may use git_config_string() or git_config_pathname() to\nread a value into a temporary variable, and then either hand off memory\nownership of the new variable or free it. These are not potential leaks,\nsince we know that there is no previous value we are overwriting.\n\nHowever, it's worth converting these to use git_config_string_dup() and\ngit_config_pathname_dup(). It makes it easier to audit for leaky cases,\nand possibly we can get rid of the leak-prone functions in the future.\nPlus it lets the const-ness of our variables match their expected memory\nownership, which avoids some casts when calling free().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c        |  4 ++--\n builtin/config.c       |  6 +++---\n builtin/receive-pack.c |  6 +++---\n fetch-pack.c           |  6 +++---\n fsck.c                 |  6 +++---\n remote.c               | 28 ++++++++++++++--------------\n setup.c                |  6 +++---\n 7 files changed, 31 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex db1f56de61..0b07ceb110 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -718,10 +718,10 @@ static int git_blame_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"blame.ignorerevsfile\")) {\n-\t\tconst char *str;\n+\t\tchar *str = NULL;\n \t\tint ret;\n \n-\t\tret = git_config_pathname(&str, var, value);\n+\t\tret = git_config_pathname_dup(&str, var, value);\n \t\tif (ret)\n \t\t\treturn ret;\n \t\tstring_list_insert(&ignore_revs_file_list, str);\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 0015620dde..fc5d96bb4c 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -282,11 +282,11 @@ static int format_config(struct strbuf *buf, const char *key_,\n \t\t\telse\n \t\t\t\tstrbuf_addstr(buf, v ? \"true\" : \"false\");\n \t\t} else if (type == TYPE_PATH) {\n-\t\t\tconst char *v;\n-\t\t\tif (git_config_pathname(&v, key_, value_) < 0)\n+\t\t\tchar *v = NULL;\n+\t\t\tif (git_config_pathname_dup(&v, key_, value_) < 0)\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addstr(buf, v);\n-\t\t\tfree((char *)v);\n+\t\t\tfree(v);\n \t\t} else if (type == TYPE_EXPIRY_DATE) {\n \t\t\ttimestamp_t t;\n \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 56d8a77ed7..15ed81a3f6 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -168,13 +168,13 @@ static int receive_pack_config(const char *var, const char *value,\n \t}\n \n \tif (strcmp(var, \"receive.fsck.skiplist\") == 0) {\n-\t\tconst char *path;\n+\t\tchar *path = NULL;\n \n-\t\tif (git_config_pathname(&path, var, value))\n+\t\tif (git_config_pathname_dup(&path, var, value))\n \t\t\treturn 1;\n \t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n \t\t\tfsck_msg_types.len ? ',' : '=', path);\n-\t\tfree((char *)path);\n+\t\tfree(path);\n \t\treturn 0;\n \t}\n \ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 091f9a80a9..fd59c497b4 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1863,13 +1863,13 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n \tconst char *msg_id;\n \n \tif (strcmp(var, \"fetch.fsck.skiplist\") == 0) {\n-\t\tconst char *path;\n+\t\tchar *path = NULL;\n \n-\t\tif (git_config_pathname(&path, var, value))\n+\t\tif (git_config_pathname_dup(&path, var, value))\n \t\t\treturn 1;\n \t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n \t\t\tfsck_msg_types.len ? ',' : '=', path);\n-\t\tfree((char *)path);\n+\t\tfree(path);\n \t\treturn 0;\n \t}\n \ndiff --git a/fsck.c b/fsck.c\nindex 78af29d264..a9d27a660f 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1274,13 +1274,13 @@ int git_fsck_config(const char *var, const char *value,\n \tconst char *msg_id;\n \n \tif (strcmp(var, \"fsck.skiplist\") == 0) {\n-\t\tconst char *path;\n+\t\tchar *path = NULL;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n-\t\tif (git_config_pathname(&path, var, value))\n+\t\tif (git_config_pathname_dup(&path, var, value))\n \t\t\treturn 1;\n \t\tstrbuf_addf(&sb, \"skiplist=%s\", path);\n-\t\tfree((char *)path);\n+\t\tfree(path);\n \t\tfsck_set_msg_types(options, sb.buf);\n \t\tstrbuf_release(&sb);\n \t\treturn 0;\ndiff --git a/remote.c b/remote.c\nindex 2b650b813b..09912bebf1 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -428,38 +428,38 @@ static int handle_config(const char *key, const char *value,\n \telse if (!strcmp(subkey, \"prunetags\"))\n \t\tremote->prune_tags = git_config_bool(key, value);\n \telse if (!strcmp(subkey, \"url\")) {\n-\t\tconst char *v;\n-\t\tif (git_config_string(&v, key, value))\n+\t\tchar *v = NULL;\n+\t\tif (git_config_string_dup(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_url(remote, v);\n \t} else if (!strcmp(subkey, \"pushurl\")) {\n-\t\tconst char *v;\n-\t\tif (git_config_string(&v, key, value))\n+\t\tchar *v = NULL;\n+\t\tif (git_config_string_dup(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_pushurl(remote, v);\n \t} else if (!strcmp(subkey, \"push\")) {\n-\t\tconst char *v;\n-\t\tif (git_config_string(&v, key, value))\n+\t\tchar *v = NULL;\n+\t\tif (git_config_string_dup(&v, key, value))\n \t\t\treturn -1;\n \t\trefspec_append(&remote->push, v);\n-\t\tfree((char *)v);\n+\t\tfree(v);\n \t} else if (!strcmp(subkey, \"fetch\")) {\n-\t\tconst char *v;\n-\t\tif (git_config_string(&v, key, value))\n+\t\tchar *v = NULL;\n+\t\tif (git_config_string_dup(&v, key, value))\n \t\t\treturn -1;\n \t\trefspec_append(&remote->fetch, v);\n-\t\tfree((char *)v);\n+\t\tfree(v);\n \t} else if (!strcmp(subkey, \"receivepack\")) {\n-\t\tconst char *v;\n-\t\tif (git_config_string(&v, key, value))\n+\t\tchar *v = NULL;\n+\t\tif (git_config_string_dup(&v, key, value))\n \t\t\treturn -1;\n \t\tif (!remote->receivepack)\n \t\t\tremote->receivepack = v;\n \t\telse\n \t\t\terror(_(\"more than one receivepack given, using the first\"));\n \t} else if (!strcmp(subkey, \"uploadpack\")) {\n-\t\tconst char *v;\n-\t\tif (git_config_string(&v, key, value))\n+\t\tchar *v = NULL;\n+\t\tif (git_config_string_dup(&v, key, value))\n \t\t\treturn -1;\n \t\tif (!remote->uploadpack)\n \t\t\tremote->uploadpack = v;\ndiff --git a/setup.c b/setup.c\nindex f4b32f76e3..9f35a27978 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1176,13 +1176,13 @@ static int safe_directory_cb(const char *key, const char *value,\n \t} else if (!strcmp(value, \"*\")) {\n \t\tdata->is_safe = 1;\n \t} else {\n-\t\tconst char *interpolated = NULL;\n+\t\tchar *interpolated = NULL;\n \n-\t\tif (!git_config_pathname(&interpolated, key, value) &&\n+\t\tif (!git_config_pathname_dup(&interpolated, key, value) &&\n \t\t    !fspathcmp(data->path, interpolated ? interpolated : value))\n \t\t\tdata->is_safe = 1;\n \n-\t\tfree((char *)interpolated);\n+\t\tfree(interpolated);\n \t}\n \n \treturn 0;\n-- \n2.44.0.872.g288abe5b5b\n\n"},{"id":"492413","messageId":"20240407010100.GD868358@coredump.intra.peff.net","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"[PATCH 04/11] config: use git_config_string_dup() for open-coded equivalents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:01:00Z","receivedAt":"2024-04-07T01:01:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These are cases where the calling code does the exact same thing\ngit_config_string_dup() would do. We can shorten the code a bit by using\nit.\n\nNote in the final case that we rely on leaving the if-else chain to\nreturn \"0\" for success, and now we'll return more directly. The two are\nequivalent.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n archive-tar.c  | 10 +++-------\n compat/mingw.c |  7 ++-----\n setup.c        |  5 +----\n 3 files changed, 6 insertions(+), 16 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 8ae30125f8..6da7101553 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -393,13 +393,9 @@ static int tar_filter_config(const char *var, const char *value,\n \t\ttar_filters[nr_tar_filters++] = ar;\n \t}\n \n-\tif (!strcmp(type, \"command\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\tfree(ar->filter_command);\n-\t\tar->filter_command = xstrdup(value);\n-\t\treturn 0;\n-\t}\n+\tif (!strcmp(type, \"command\"))\n+\t\treturn git_config_string_dup(&ar->filter_command, var, value);\n+\n \tif (!strcmp(type, \"remote\")) {\n \t\tif (git_config_bool(var, value))\n \t\t\tar->flags |= ARCHIVER_REMOTE;\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 320fb99a90..aeccb3957f 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -255,11 +255,8 @@ int mingw_core_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"core.unsetenvvars\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\tfree(unset_environment_variables);\n-\t\tunset_environment_variables = xstrdup(value);\n-\t\treturn 0;\n+\t\treturn git_config_string_dup(&unset_environment_variables, var,\n+\t\t\t\t\t     value);\n \t}\n \n \tif (!strcmp(var, \"core.restrictinheritedhandles\")) {\ndiff --git a/setup.c b/setup.c\nindex 9f35a27978..7204fd2815 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -529,10 +529,7 @@ static int read_worktree_config(const char *var, const char *value,\n \tif (strcmp(var, \"core.bare\") == 0) {\n \t\tdata->is_bare = git_config_bool(var, value);\n \t} else if (strcmp(var, \"core.worktree\") == 0) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\tfree(data->work_tree);\n-\t\tdata->work_tree = xstrdup(value);\n+\t\treturn git_config_string_dup(&data->work_tree, var, value);\n \t}\n \treturn 0;\n }\n-- \n2.44.0.872.g288abe5b5b\n\n"},{"id":"492414","messageId":"20240407010143.GE868358@coredump.intra.peff.net","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"[PATCH 05/11] config: use git_config_string_dup() to fix leaky open coding","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:01:43Z","receivedAt":"2024-04-07T01:01:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These are cases which open-code the equivalent of git_config_string(),\nbut end up leaking the resulting value if the config value is seen\nmultiple times (because they never free an existing value). Using\ngit_config_string_dup() plug these potential leaks.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/help.c | 7 ++-----\n config.c       | 8 ++------\n merge-ll.c     | 5 +----\n 3 files changed, 5 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex dc1fbe2b98..1bdd2faee0 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -52,7 +52,7 @@ static enum help_action {\n \tHELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,\n } cmd_mode;\n \n-static const char *html_path;\n+static char *html_path;\n static int verbose = 1;\n static enum help_format help_format = HELP_FORMAT_NONE;\n static int exclude_guides;\n@@ -407,10 +407,7 @@ static int git_help_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"help.htmlpath\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\thtml_path = xstrdup(value);\n-\t\treturn 0;\n+\t\treturn git_config_string_dup(&html_path, var, value);\n \t}\n \tif (!strcmp(var, \"man.viewer\")) {\n \t\tif (!value)\ndiff --git a/config.c b/config.c\nindex a0aa45abd5..c115e6d8c9 100644\n--- a/config.c\n+++ b/config.c\n@@ -1575,12 +1575,8 @@ static int git_default_core_config(const char *var, const char *value,\n \tif (!strcmp(var, \"core.checkroundtripencoding\"))\n \t\treturn git_config_string(&check_roundtrip_encoding, var, value);\n \n-\tif (!strcmp(var, \"core.notesref\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\tnotes_ref_name = xstrdup(value);\n-\t\treturn 0;\n-\t}\n+\tif (!strcmp(var, \"core.notesref\"))\n+\t\treturn git_config_string_dup(&notes_ref_name, var, value);\n \n \tif (!strcmp(var, \"core.editor\"))\n \t\treturn git_config_string(&editor_program, var, value);\ndiff --git a/merge-ll.c b/merge-ll.c\nindex bf1077ae09..660d9a3bd6 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -308,8 +308,6 @@ static int read_merge_config(const char *var, const char *value,\n \t\treturn git_config_string(&fn->description, var, value);\n \n \tif (!strcmp(\"driver\", key)) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n \t\t/*\n \t\t * merge.<name>.driver specifies the command line:\n \t\t *\n@@ -333,8 +331,7 @@ static int read_merge_config(const char *var, const char *value,\n \t\t * file named by %A, and signal that it has done with zero exit\n \t\t * status.\n \t\t */\n-\t\tfn->cmdline = xstrdup(value);\n-\t\treturn 0;\n+\t\treturn git_config_string_dup(&fn->cmdline, var, value);\n \t}\n \n \tif (!strcmp(\"recursive\", key))\n-- \n2.44.0.872.g288abe5b5b\n\n"},{"id":"492415","messageId":"20240407010228.GF868358@coredump.intra.peff.net","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"[PATCH 06/11] config: use git_config_string() in easy cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:02:28Z","receivedAt":"2024-04-07T01:02:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The git_config_string() interface is very prone to leaking; it\noverwrites the contents of the pointer without freeing any existing\nvalue, so:\n\n  const char *foo;\n  ...\n  if (!strcmp(var, \"foo.bar\"))\n\tgit_config_string(&foo, var, value);\n\nis always going to leak if we see the \"foo.bar\" config more than once.\nWe don't tend to notice in practice because people don't have a lot of\nredundant config, but basically every use of git_config_string() has\nthis problem.\n\nFor simple cases where the initial value of the \"foo\" pointer is NULL\n(like the one above), we can easily convert them to use a variant of the\nhelper that frees the existing value before writing over it. That works\nbecause free(NULL) is a noop. What we can't change, though, is cases\nwhere the pointer is initialized to a string literal, like:\n\n  const char *foo = \"some string literal\";\n\nIn that case it would be incorrect to free() the original value. Those\ncases will take more refactoring to make them leak-free, and this patch\npunts on that for now.\n\nIn each case we switch the underlying variable from \"const char *\" to\n\"char *\" to indicate that it will always point to allocated memory (and\nour git_config_string_dup() interface enforces this).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n alias.c                |  3 +--\n attr.c                 |  2 +-\n attr.h                 |  2 +-\n builtin/commit.c       |  4 ++--\n builtin/log.c          | 12 ++++++------\n builtin/merge.c        |  4 ++--\n builtin/receive-pack.c |  4 ++--\n builtin/repack.c       | 16 ++++++++--------\n config.c               | 12 ++++++------\n convert.c              | 12 ++++++------\n delta-islands.c        |  4 ++--\n diff.c                 |  8 ++++----\n environment.c          |  8 ++++----\n environment.h          |  8 ++++----\n gpg-interface.c        |  5 +++--\n imap-send.c            | 20 ++++++++++----------\n mailmap.c              |  2 +-\n mailmap.h              |  2 +-\n merge-ll.c             | 12 ++++++------\n pager.c                |  4 ++--\n promisor-remote.c      |  2 +-\n promisor-remote.h      |  2 +-\n remote.c               | 17 ++++++++---------\n remote.h               |  8 ++++----\n sequencer.c            |  2 +-\n upload-pack.c          |  4 ++--\n 26 files changed, 89 insertions(+), 90 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex 5a238f2e30..7f8c1c3e7d 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -22,8 +22,7 @@ static int config_alias_cb(const char *key, const char *value,\n \n \tif (data->alias) {\n \t\tif (!strcasecmp(p, data->alias))\n-\t\t\treturn git_config_string((const char **)&data->v,\n-\t\t\t\t\t\t key, value);\n+\t\t\treturn git_config_string_dup(&data->v, key, value);\n \t} else if (data->list) {\n \t\tstring_list_append(data->list, p);\n \t}\ndiff --git a/attr.c b/attr.c\nindex 679e42258c..4eda36c865 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -25,7 +25,7 @@\n #include \"tree-walk.h\"\n #include \"object-name.h\"\n \n-const char *git_attr_tree;\n+char *git_attr_tree;\n \n const char git_attr__true[] = \"(builtin)true\";\n const char git_attr__false[] = \"\\0(builtin)false\";\ndiff --git a/attr.h b/attr.h\nindex 127998ae01..e7cc318b0c 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -236,6 +236,6 @@ const char *git_attr_global_file(void);\n /* Return whether the system gitattributes file is enabled and should be used. */\n int git_attr_system_is_enabled(void);\n \n-extern const char *git_attr_tree;\n+extern char *git_attr_tree;\n \n #endif /* ATTR_H */\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 7ba7201cfb..9b6546d401 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -133,7 +133,7 @@ static struct strvec trailer_args = STRVEC_INIT;\n  * is specified explicitly.\n  */\n static enum commit_msg_cleanup_mode cleanup_mode;\n-static const char *cleanup_arg;\n+static char *cleanup_arg;\n \n static enum commit_whence whence;\n static int use_editor = 1, include_status = 1;\n@@ -1632,7 +1632,7 @@ static int git_commit_config(const char *k, const char *v,\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"commit.cleanup\"))\n-\t\treturn git_config_string(&cleanup_arg, k, v);\n+\t\treturn git_config_string_dup(&cleanup_arg, k, v);\n \tif (!strcmp(k, \"commit.gpgsign\")) {\n \t\tsign_commit = git_config_bool(k, v) ? \"\" : NULL;\n \t\treturn 0;\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c0a8bb95e9..eb336f7efb 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -49,7 +49,7 @@\n #define FORMAT_PATCH_NAME_MAX_DEFAULT 64\n \n /* Set a default date-time format for git log (\"log.date\" config variable) */\n-static const char *default_date_mode = NULL;\n+static char *default_date_mode;\n \n static int default_abbrev_commit;\n static int default_show_root = 1;\n@@ -63,7 +63,7 @@ static unsigned int force_in_body_from;\n static int stdout_mboxrd;\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;\n-static const char *fmt_pretty;\n+static char *fmt_pretty;\n static int format_no_prefix;\n \n static const char * const builtin_log_usage[] = {\n@@ -569,7 +569,7 @@ static int git_log_config(const char *var, const char *value,\n \tconst char *slot_name;\n \n \tif (!strcmp(var, \"format.pretty\"))\n-\t\treturn git_config_string(&fmt_pretty, var, value);\n+\t\treturn git_config_string_dup(&fmt_pretty, var, value);\n \tif (!strcmp(var, \"format.subjectprefix\"))\n \t\treturn git_config_string(&fmt_patch_subject_prefix, var, value);\n \tif (!strcmp(var, \"format.filenamemaxlength\")) {\n@@ -585,7 +585,7 @@ static int git_log_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"log.date\"))\n-\t\treturn git_config_string(&default_date_mode, var, value);\n+\t\treturn git_config_string_dup(&default_date_mode, var, value);\n \tif (!strcmp(var, \"log.decorate\")) {\n \t\tdecoration_style = parse_decoration_style(value);\n \t\tif (decoration_style < 0)\n@@ -959,7 +959,7 @@ static char *from;\n static const char *signature = git_version_string;\n static const char *signature_file;\n static enum cover_setting config_cover_letter;\n-static const char *config_output_directory;\n+static char *config_output_directory;\n static enum cover_from_description cover_from_description_mode = COVER_FROM_MESSAGE;\n static int show_notes;\n static struct display_notes_opt notes_opt;\n@@ -1054,7 +1054,7 @@ static int git_format_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"format.outputdirectory\"))\n-\t\treturn git_config_string(&config_output_directory, var, value);\n+\t\treturn git_config_string_dup(&config_output_directory, var, value);\n \tif (!strcmp(var, \"format.useautobase\")) {\n \t\tif (value && !strcasecmp(value, \"whenAble\")) {\n \t\t\tauto_base = AUTO_BASE_WHEN_ABLE;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 6f4fec87fc..c2be29ed2f 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -111,7 +111,7 @@ enum ff_type {\n \n static enum ff_type fast_forward = FF_ALLOW;\n \n-static const char *cleanup_arg;\n+static char *cleanup_arg;\n static enum commit_msg_cleanup_mode cleanup_mode;\n \n static int option_parse_message(const struct option *opt,\n@@ -619,7 +619,7 @@ static int git_merge_config(const char *k, const char *v,\n \telse if (!strcmp(k, \"pull.octopus\"))\n \t\treturn git_config_string(&pull_octopus, k, v);\n \telse if (!strcmp(k, \"commit.cleanup\"))\n-\t\treturn git_config_string(&cleanup_arg, k, v);\n+\t\treturn git_config_string_dup(&cleanup_arg, k, v);\n \telse if (!strcmp(k, \"merge.ff\")) {\n \t\tint boolval = git_parse_maybe_bool(v);\n \t\tif (0 <= boolval) {\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 15ed81a3f6..2ba71b6673 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -88,7 +88,7 @@ static struct strbuf push_cert = STRBUF_INIT;\n static struct object_id push_cert_oid;\n static struct signature_check sigcheck;\n static const char *push_cert_nonce;\n-static const char *cert_nonce_seed;\n+static char *cert_nonce_seed;\n static struct strvec hidden_refs = STRVEC_INIT;\n \n static const char *NONCE_UNSOLICITED = \"UNSOLICITED\";\n@@ -230,7 +230,7 @@ static int receive_pack_config(const char *var, const char *value,\n \t}\n \n \tif (strcmp(var, \"receive.certnonceseed\") == 0)\n-\t\treturn git_config_string(&cert_nonce_seed, var, value);\n+\t\treturn git_config_string_dup(&cert_nonce_seed, var, value);\n \n \tif (strcmp(var, \"receive.certnonceslop\") == 0) {\n \t\tnonce_stamp_slop_limit = git_config_ulong(var, value, ctx->kvi);\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 15e4cccc45..c144e18d9f 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -48,10 +48,10 @@ static const char incremental_bitmap_conflict_error[] = N_(\n );\n \n struct pack_objects_args {\n-\tconst char *window;\n-\tconst char *window_memory;\n-\tconst char *depth;\n-\tconst char *threads;\n+\tchar *window;\n+\tchar *window_memory;\n+\tchar *depth;\n+\tchar *threads;\n \tunsigned long max_pack_size;\n \tint no_reuse_delta;\n \tint no_reuse_object;\n@@ -86,13 +86,13 @@ static int repack_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"repack.cruftwindow\"))\n-\t\treturn git_config_string(&cruft_po_args->window, var, value);\n+\t\treturn git_config_string_dup(&cruft_po_args->window, var, value);\n \tif (!strcmp(var, \"repack.cruftwindowmemory\"))\n-\t\treturn git_config_string(&cruft_po_args->window_memory, var, value);\n+\t\treturn git_config_string_dup(&cruft_po_args->window_memory, var, value);\n \tif (!strcmp(var, \"repack.cruftdepth\"))\n-\t\treturn git_config_string(&cruft_po_args->depth, var, value);\n+\t\treturn git_config_string_dup(&cruft_po_args->depth, var, value);\n \tif (!strcmp(var, \"repack.cruftthreads\"))\n-\t\treturn git_config_string(&cruft_po_args->threads, var, value);\n+\t\treturn git_config_string_dup(&cruft_po_args->threads, var, value);\n \treturn git_default_config(var, value, ctx, cb);\n }\n \ndiff --git a/config.c b/config.c\nindex c115e6d8c9..449b8f4f66 100644\n--- a/config.c\n+++ b/config.c\n@@ -1579,7 +1579,7 @@ static int git_default_core_config(const char *var, const char *value,\n \t\treturn git_config_string_dup(&notes_ref_name, var, value);\n \n \tif (!strcmp(var, \"core.editor\"))\n-\t\treturn git_config_string(&editor_program, var, value);\n+\t\treturn git_config_string_dup(&editor_program, var, value);\n \n \tif (!strcmp(var, \"core.commentchar\") ||\n \t    !strcmp(var, \"core.commentstring\")) {\n@@ -1598,7 +1598,7 @@ static int git_default_core_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"core.askpass\"))\n-\t\treturn git_config_string(&askpass_program, var, value);\n+\t\treturn git_config_string_dup(&askpass_program, var, value);\n \n \tif (!strcmp(var, \"core.excludesfile\"))\n \t\treturn git_config_pathname(&excludes_file, var, value);\n@@ -1703,10 +1703,10 @@ static int git_default_sparse_config(const char *var, const char *value)\n static int git_default_i18n_config(const char *var, const char *value)\n {\n \tif (!strcmp(var, \"i18n.commitencoding\"))\n-\t\treturn git_config_string(&git_commit_encoding, var, value);\n+\t\treturn git_config_string_dup(&git_commit_encoding, var, value);\n \n \tif (!strcmp(var, \"i18n.logoutputencoding\"))\n-\t\treturn git_config_string(&git_log_output_encoding, var, value);\n+\t\treturn git_config_string_dup(&git_log_output_encoding, var, value);\n \n \t/* Add other config variables here and to Documentation/config.txt. */\n \treturn 0;\n@@ -1782,7 +1782,7 @@ static int git_default_mailmap_config(const char *var, const char *value)\n \tif (!strcmp(var, \"mailmap.file\"))\n \t\treturn git_config_pathname(&git_mailmap_file, var, value);\n \tif (!strcmp(var, \"mailmap.blob\"))\n-\t\treturn git_config_string(&git_mailmap_blob, var, value);\n+\t\treturn git_config_string_dup(&git_mailmap_blob, var, value);\n \n \t/* Add other config variables here and to Documentation/config.txt. */\n \treturn 0;\n@@ -1791,7 +1791,7 @@ static int git_default_mailmap_config(const char *var, const char *value)\n static int git_default_attr_config(const char *var, const char *value)\n {\n \tif (!strcmp(var, \"attr.tree\"))\n-\t\treturn git_config_string(&git_attr_tree, var, value);\n+\t\treturn git_config_string_dup(&git_attr_tree, var, value);\n \n \t/*\n \t * Add other attribute related config variables here and to\ndiff --git a/convert.c b/convert.c\nindex 35b25eb3cb..d462485d1f 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -979,9 +979,9 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_p\n static struct convert_driver {\n \tconst char *name;\n \tstruct convert_driver *next;\n-\tconst char *smudge;\n-\tconst char *clean;\n-\tconst char *process;\n+\tchar *smudge;\n+\tchar *clean;\n+\tchar *process;\n \tint required;\n } *user_convert, **user_convert_tail;\n \n@@ -1047,13 +1047,13 @@ static int read_convert_config(const char *var, const char *value,\n \t */\n \n \tif (!strcmp(\"smudge\", key))\n-\t\treturn git_config_string(&drv->smudge, var, value);\n+\t\treturn git_config_string_dup(&drv->smudge, var, value);\n \n \tif (!strcmp(\"clean\", key))\n-\t\treturn git_config_string(&drv->clean, var, value);\n+\t\treturn git_config_string_dup(&drv->clean, var, value);\n \n \tif (!strcmp(\"process\", key))\n-\t\treturn git_config_string(&drv->process, var, value);\n+\t\treturn git_config_string_dup(&drv->process, var, value);\n \n \tif (!strcmp(\"required\", key)) {\n \t\tdrv->required = git_config_bool(var, value);\ndiff --git a/delta-islands.c b/delta-islands.c\nindex f7e079425f..c8ff736a4d 100644\n--- a/delta-islands.c\n+++ b/delta-islands.c\n@@ -313,7 +313,7 @@ struct island_load_data {\n \tsize_t nr;\n \tsize_t alloc;\n };\n-static const char *core_island_name;\n+static char *core_island_name;\n \n static void free_config_regexes(struct island_load_data *ild)\n {\n@@ -362,7 +362,7 @@ static int island_config_callback(const char *k, const char *v,\n \t}\n \n \tif (!strcmp(k, \"pack.islandcore\"))\n-\t\treturn git_config_string(&core_island_name, k, v);\n+\t\treturn git_config_string_dup(&core_island_name, k, v);\n \n \treturn 0;\n }\ndiff --git a/diff.c b/diff.c\nindex 108c187577..bb00bc1110 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -56,8 +56,8 @@ static int diff_color_moved_default;\n static int diff_color_moved_ws_default;\n static int diff_context_default = 3;\n static int diff_interhunk_context_default;\n-static const char *diff_word_regex_cfg;\n-static const char *external_diff_cmd_cfg;\n+static char *diff_word_regex_cfg;\n+static char *external_diff_cmd_cfg;\n static const char *diff_order_file_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n@@ -429,9 +429,9 @@ int git_diff_ui_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"diff.external\"))\n-\t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n+\t\treturn git_config_string_dup(&external_diff_cmd_cfg, var, value);\n \tif (!strcmp(var, \"diff.wordregex\"))\n-\t\treturn git_config_string(&diff_word_regex_cfg, var, value);\n+\t\treturn git_config_string_dup(&diff_word_regex_cfg, var, value);\n \tif (!strcmp(var, \"diff.orderfile\"))\n \t\treturn git_config_pathname(&diff_order_file_cfg, var, value);\n \ndiff --git a/environment.c b/environment.c\nindex a73ba9c12c..5e9fab4d1a 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -42,8 +42,8 @@ int is_bare_repository_cfg = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n int repository_format_precious_objects;\n-const char *git_commit_encoding;\n-const char *git_log_output_encoding;\n+char *git_commit_encoding;\n+char *git_log_output_encoding;\n char *apply_default_whitespace;\n char *apply_default_ignorewhitespace;\n const char *git_attributes_file;\n@@ -58,8 +58,8 @@ size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n size_t delta_base_cache_limit = 96 * 1024 * 1024;\n unsigned long big_file_threshold = 512 * 1024 * 1024;\n-const char *editor_program;\n-const char *askpass_program;\n+char *editor_program;\n+char *askpass_program;\n const char *excludes_file;\n enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;\n enum eol core_eol = EOL_UNSET;\ndiff --git a/environment.h b/environment.h\nindex 05fd94d7be..f06024a457 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -217,11 +217,11 @@ int odb_pack_keep(const char *name);\n const char *get_log_output_encoding(void);\n const char *get_commit_output_encoding(void);\n \n-extern const char *git_commit_encoding;\n-extern const char *git_log_output_encoding;\n+extern char *git_commit_encoding;\n+extern char *git_log_output_encoding;\n \n-extern const char *editor_program;\n-extern const char *askpass_program;\n+extern char *editor_program;\n+extern char *askpass_program;\n extern const char *excludes_file;\n \n /*\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex b5993385ff..4b2f70ef44 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -27,7 +27,8 @@ static void gpg_interface_lazy_init(void)\n }\n \n static char *configured_signing_key;\n-static const char *ssh_default_key_command, *ssh_allowed_signers, *ssh_revocation_file;\n+static char *ssh_default_key_command;\n+static const char *ssh_allowed_signers, *ssh_revocation_file;\n static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;\n \n struct gpg_format {\n@@ -762,7 +763,7 @@ static int git_gpg_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"gpg.ssh.defaultkeycommand\"))\n-\t\treturn git_config_string(&ssh_default_key_command, var, value);\n+\t\treturn git_config_string_dup(&ssh_default_key_command, var, value);\n \n \tif (!strcmp(var, \"gpg.ssh.allowedsignersfile\"))\n \t\treturn git_config_pathname(&ssh_allowed_signers, var, value);\ndiff --git a/imap-send.c b/imap-send.c\nindex f2e1947e63..d4454a3b49 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -87,16 +87,16 @@ static int nfvasprintf(char **strp, const char *fmt, va_list ap)\n \n struct imap_server_conf {\n \tconst char *name;\n-\tconst char *tunnel;\n+\tchar *tunnel;\n \tconst char *host;\n \tint port;\n-\tconst char *folder;\n-\tconst char *user;\n-\tconst char *pass;\n+\tchar *folder;\n+\tchar *user;\n+\tchar *pass;\n \tint use_ssl;\n \tint ssl_verify;\n \tint use_html;\n-\tconst char *auth_method;\n+\tchar *auth_method;\n };\n \n static struct imap_server_conf server = {\n@@ -1332,15 +1332,15 @@ static int git_imap_config(const char *var, const char *val,\n \telse if (!strcmp(\"imap.preformattedhtml\", var))\n \t\tserver.use_html = git_config_bool(var, val);\n \telse if (!strcmp(\"imap.folder\", var))\n-\t\treturn git_config_string(&server.folder, var, val);\n+\t\treturn git_config_string_dup(&server.folder, var, val);\n \telse if (!strcmp(\"imap.user\", var))\n-\t\treturn git_config_string(&server.user, var, val);\n+\t\treturn git_config_string_dup(&server.user, var, val);\n \telse if (!strcmp(\"imap.pass\", var))\n-\t\treturn git_config_string(&server.pass, var, val);\n+\t\treturn git_config_string_dup(&server.pass, var, val);\n \telse if (!strcmp(\"imap.tunnel\", var))\n-\t\treturn git_config_string(&server.tunnel, var, val);\n+\t\treturn git_config_string_dup(&server.tunnel, var, val);\n \telse if (!strcmp(\"imap.authmethod\", var))\n-\t\treturn git_config_string(&server.auth_method, var, val);\n+\t\treturn git_config_string_dup(&server.auth_method, var, val);\n \telse if (!strcmp(\"imap.port\", var))\n \t\tserver.port = git_config_int(var, val, ctx->kvi);\n \telse if (!strcmp(\"imap.host\", var)) {\ndiff --git a/mailmap.c b/mailmap.c\nindex 3d6a5e9400..9bd11c90e6 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -7,7 +7,7 @@\n #include \"setup.h\"\n \n const char *git_mailmap_file;\n-const char *git_mailmap_blob;\n+char *git_mailmap_blob;\n \n struct mailmap_info {\n \tchar *name;\ndiff --git a/mailmap.h b/mailmap.h\nindex 0f8fd2c586..a1bc019b52 100644\n--- a/mailmap.h\n+++ b/mailmap.h\n@@ -4,7 +4,7 @@\n struct string_list;\n \n extern const char *git_mailmap_file;\n-extern const char *git_mailmap_blob;\n+extern char *git_mailmap_blob;\n \n int read_mailmap(struct string_list *map);\n void clear_mailmap(struct string_list *map);\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 660d9a3bd6..5ef96309d8 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -27,9 +27,9 @@ typedef enum ll_merge_result (*ll_merge_fn)(const struct ll_merge_driver *,\n \n struct ll_merge_driver {\n \tconst char *name;\n-\tconst char *description;\n+\tchar *description;\n \tll_merge_fn fn;\n-\tconst char *recursive;\n+\tchar *recursive;\n \tstruct ll_merge_driver *next;\n \tchar *cmdline;\n };\n@@ -268,7 +268,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n  * merge.default and merge.driver configuration items\n  */\n static struct ll_merge_driver *ll_user_merge, **ll_user_merge_tail;\n-static const char *default_ll_merge;\n+static char *default_ll_merge;\n \n static int read_merge_config(const char *var, const char *value,\n \t\t\t     const struct config_context *ctx UNUSED,\n@@ -279,7 +279,7 @@ static int read_merge_config(const char *var, const char *value,\n \tsize_t namelen;\n \n \tif (!strcmp(var, \"merge.default\"))\n-\t\treturn git_config_string(&default_ll_merge, var, value);\n+\t\treturn git_config_string_dup(&default_ll_merge, var, value);\n \n \t/*\n \t * We are not interested in anything but \"merge.<name>.variable\";\n@@ -305,7 +305,7 @@ static int read_merge_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(\"name\", key))\n-\t\treturn git_config_string(&fn->description, var, value);\n+\t\treturn git_config_string_dup(&fn->description, var, value);\n \n \tif (!strcmp(\"driver\", key)) {\n \t\t/*\n@@ -335,7 +335,7 @@ static int read_merge_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(\"recursive\", key))\n-\t\treturn git_config_string(&fn->recursive, var, value);\n+\t\treturn git_config_string_dup(&fn->recursive, var, value);\n \n \treturn 0;\n }\ndiff --git a/pager.c b/pager.c\nindex b8822a9381..6d75528e56 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -13,7 +13,7 @@ int pager_use_color = 1;\n #endif\n \n static struct child_process pager_process;\n-static const char *pager_program;\n+static char *pager_program;\n \n /* Is the value coming back from term_columns() just a guess? */\n static int term_columns_guessed;\n@@ -47,7 +47,7 @@ static int core_pager_config(const char *var, const char *value,\n \t\t\t     void *data UNUSED)\n {\n \tif (!strcmp(var, \"core.pager\"))\n-\t\treturn git_config_string(&pager_program, var, value);\n+\t\treturn git_config_string_dup(&pager_program, var, value);\n \treturn 0;\n }\n \ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex ac3aa1e365..65693e7931 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -139,7 +139,7 @@ static int promisor_remote_config(const char *var, const char *value,\n \t\tif (!r)\n \t\t\treturn 0;\n \n-\t\treturn git_config_string(&r->partial_clone_filter, var, value);\n+\t\treturn git_config_string_dup(&r->partial_clone_filter, var, value);\n \t}\n \n \treturn 0;\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex 2cb9eda9ea..88cb599c39 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -13,7 +13,7 @@ struct object_id;\n  */\n struct promisor_remote {\n \tstruct promisor_remote *next;\n-\tconst char *partial_clone_filter;\n+\tchar *partial_clone_filter;\n \tconst char name[FLEX_ARRAY];\n };\n \ndiff --git a/remote.c b/remote.c\nindex 09912bebf1..761a8b8fe3 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -367,9 +367,9 @@ static int handle_config(const char *key, const char *value,\n \t\t\treturn -1;\n \t\tbranch = make_branch(remote_state, name, namelen);\n \t\tif (!strcmp(subkey, \"remote\")) {\n-\t\t\treturn git_config_string(&branch->remote_name, key, value);\n+\t\t\treturn git_config_string_dup(&branch->remote_name, key, value);\n \t\t} else if (!strcmp(subkey, \"pushremote\")) {\n-\t\t\treturn git_config_string(&branch->pushremote_name, key, value);\n+\t\t\treturn git_config_string_dup(&branch->pushremote_name, key, value);\n \t\t} else if (!strcmp(subkey, \"merge\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n@@ -401,8 +401,8 @@ static int handle_config(const char *key, const char *value,\n \n \t/* Handle remote.* variables */\n \tif (!name && !strcmp(subkey, \"pushdefault\"))\n-\t\treturn git_config_string(&remote_state->pushremote_name, key,\n-\t\t\t\t\t value);\n+\t\treturn git_config_string_dup(&remote_state->pushremote_name, key,\n+\t\t\t\t\t     value);\n \n \tif (!name)\n \t\treturn 0;\n@@ -471,13 +471,12 @@ static int handle_config(const char *key, const char *value,\n \t\telse if (!strcmp(value, \"--tags\"))\n \t\t\tremote->fetch_tags = 2;\n \t} else if (!strcmp(subkey, \"proxy\")) {\n-\t\treturn git_config_string((const char **)&remote->http_proxy,\n-\t\t\t\t\t key, value);\n+\t\treturn git_config_string_dup(&remote->http_proxy, key, value);\n \t} else if (!strcmp(subkey, \"proxyauthmethod\")) {\n-\t\treturn git_config_string((const char **)&remote->http_proxy_authmethod,\n-\t\t\t\t\t key, value);\n+\t\treturn git_config_string_dup(&remote->http_proxy_authmethod,\n+\t\t\t\t\t     key, value);\n \t} else if (!strcmp(subkey, \"vcs\")) {\n-\t\treturn git_config_string(&remote->foreign_vcs, key, value);\n+\t\treturn git_config_string_dup(&remote->foreign_vcs, key, value);\n \t}\n \treturn 0;\n }\ndiff --git a/remote.h b/remote.h\nindex dfd4837e60..e8c6655e42 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -46,7 +46,7 @@ struct remote_state {\n \tstruct hashmap branches_hash;\n \n \tstruct branch *current_branch;\n-\tconst char *pushremote_name;\n+\tchar *pushremote_name;\n \n \tstruct rewrites rewrites;\n \tstruct rewrites rewrites_push;\n@@ -65,7 +65,7 @@ struct remote {\n \n \tint origin, configured_in_repo;\n \n-\tconst char *foreign_vcs;\n+\tchar *foreign_vcs;\n \n \t/* An array of all of the url_nr URLs configured for the remote */\n \tconst char **url;\n@@ -309,9 +309,9 @@ struct branch {\n \tconst char *refname;\n \n \t/* The name of the remote listed in the configuration. */\n-\tconst char *remote_name;\n+\tchar *remote_name;\n \n-\tconst char *pushremote_name;\n+\tchar *pushremote_name;\n \n \t/* An array of the \"merge\" lines in the configuration. */\n \tconst char **merge_name;\ndiff --git a/sequencer.c b/sequencer.c\nindex 3e5d82e0e5..29f019b2a0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -266,7 +266,7 @@ static int git_sequencer_config(const char *k, const char *v,\n \t}\n \n \tif (!opts->default_strategy && !strcmp(k, \"pull.twohead\")) {\n-\t\tint ret = git_config_string((const char**)&opts->default_strategy, k, v);\n+\t\tint ret = git_config_string_dup(&opts->default_strategy, k, v);\n \t\tif (ret == 0) {\n \t\t\t/*\n \t\t\t * pull.twohead is allowed to be multi-valued; we only\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 902144b9d3..5f5943cc19 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -94,7 +94,7 @@ struct upload_pack_data {\n \n \tstruct packet_writer writer;\n \n-\tconst char *pack_objects_hook;\n+\tchar *pack_objects_hook;\n \n \tunsigned stateless_rpc : 1;\t\t\t\t/* v0 only */\n \tunsigned no_done : 1;\t\t\t\t\t/* v0 only */\n@@ -1386,7 +1386,7 @@ static int upload_pack_protected_config(const char *var, const char *value,\n \tstruct upload_pack_data *data = cb_data;\n \n \tif (!strcmp(\"uploadpack.packobjectshook\", var))\n-\t\treturn git_config_string(&data->pack_objects_hook, var, value);\n+\t\treturn git_config_string_dup(&data->pack_objects_hook, var, value);\n \treturn 0;\n }\n \n-- \n2.44.0.872.g288abe5b5b\n\n"},{"id":"492416","messageId":"20240407010301.GG868358@coredump.intra.peff.net","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"[PATCH 07/11] config: use git_config_pathname_dup() in easy cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:03:01Z","receivedAt":"2024-04-07T01:03:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As with the previous commit for git_config_string(), this fixes simple\ncases where calling git_config_pathname() may leak if a config variable\nis seen multiple times.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI mostly split this out because the diff is big, but maybe it would make\nsense squashed to share the rationale?\n\n builtin/commit.c | 4 ++--\n builtin/log.c    | 4 ++--\n config.c         | 8 ++++----\n diff.c           | 4 ++--\n environment.c    | 6 +++---\n environment.h    | 6 +++---\n gpg-interface.c  | 6 +++---\n mailmap.c        | 2 +-\n mailmap.h        | 2 +-\n 9 files changed, 21 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 9b6546d401..693175e5c4 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -107,7 +107,7 @@ static enum {\n } commit_style;\n \n static const char *logfile, *force_author;\n-static const char *template_file;\n+static char *template_file;\n /*\n  * The _message variables are commit names from which to take\n  * the commit message and/or authorship.\n@@ -1626,7 +1626,7 @@ static int git_commit_config(const char *k, const char *v,\n \tstruct wt_status *s = cb;\n \n \tif (!strcmp(k, \"commit.template\"))\n-\t\treturn git_config_pathname(&template_file, k, v);\n+\t\treturn git_config_pathname_dup(&template_file, k, v);\n \tif (!strcmp(k, \"commit.status\")) {\n \t\tinclude_status = git_config_bool(k, v);\n \t\treturn 0;\ndiff --git a/builtin/log.c b/builtin/log.c\nindex eb336f7efb..3df261f94a 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -957,7 +957,7 @@ static int do_signoff;\n static enum auto_base_setting auto_base;\n static char *from;\n static const char *signature = git_version_string;\n-static const char *signature_file;\n+static char *signature_file;\n static enum cover_setting config_cover_letter;\n static char *config_output_directory;\n static enum cover_from_description cover_from_description_mode = COVER_FROM_MESSAGE;\n@@ -1044,7 +1044,7 @@ static int git_format_config(const char *var, const char *value,\n \tif (!strcmp(var, \"format.signature\"))\n \t\treturn git_config_string(&signature, var, value);\n \tif (!strcmp(var, \"format.signaturefile\"))\n-\t\treturn git_config_pathname(&signature_file, var, value);\n+\t\treturn git_config_pathname_dup(&signature_file, var, value);\n \tif (!strcmp(var, \"format.coverletter\")) {\n \t\tif (value && !strcasecmp(value, \"auto\")) {\n \t\t\tconfig_cover_letter = COVER_AUTO;\ndiff --git a/config.c b/config.c\nindex 449b8f4f66..c2863d6514 100644\n--- a/config.c\n+++ b/config.c\n@@ -1434,10 +1434,10 @@ static int git_default_core_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"core.attributesfile\"))\n-\t\treturn git_config_pathname(&git_attributes_file, var, value);\n+\t\treturn git_config_pathname_dup(&git_attributes_file, var, value);\n \n \tif (!strcmp(var, \"core.hookspath\"))\n-\t\treturn git_config_pathname(&git_hooks_path, var, value);\n+\t\treturn git_config_pathname_dup(&git_hooks_path, var, value);\n \n \tif (!strcmp(var, \"core.bare\")) {\n \t\tis_bare_repository_cfg = git_config_bool(var, value);\n@@ -1601,7 +1601,7 @@ static int git_default_core_config(const char *var, const char *value,\n \t\treturn git_config_string_dup(&askpass_program, var, value);\n \n \tif (!strcmp(var, \"core.excludesfile\"))\n-\t\treturn git_config_pathname(&excludes_file, var, value);\n+\t\treturn git_config_pathname_dup(&excludes_file, var, value);\n \n \tif (!strcmp(var, \"core.whitespace\")) {\n \t\tif (!value)\n@@ -1780,7 +1780,7 @@ static int git_default_push_config(const char *var, const char *value)\n static int git_default_mailmap_config(const char *var, const char *value)\n {\n \tif (!strcmp(var, \"mailmap.file\"))\n-\t\treturn git_config_pathname(&git_mailmap_file, var, value);\n+\t\treturn git_config_pathname_dup(&git_mailmap_file, var, value);\n \tif (!strcmp(var, \"mailmap.blob\"))\n \t\treturn git_config_string_dup(&git_mailmap_blob, var, value);\n \ndiff --git a/diff.c b/diff.c\nindex bb00bc1110..6236746920 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -58,7 +58,7 @@ static int diff_context_default = 3;\n static int diff_interhunk_context_default;\n static char *diff_word_regex_cfg;\n static char *external_diff_cmd_cfg;\n-static const char *diff_order_file_cfg;\n+static char *diff_order_file_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n static int diff_no_prefix;\n@@ -433,7 +433,7 @@ int git_diff_ui_config(const char *var, const char *value,\n \tif (!strcmp(var, \"diff.wordregex\"))\n \t\treturn git_config_string_dup(&diff_word_regex_cfg, var, value);\n \tif (!strcmp(var, \"diff.orderfile\"))\n-\t\treturn git_config_pathname(&diff_order_file_cfg, var, value);\n+\t\treturn git_config_pathname_dup(&diff_order_file_cfg, var, value);\n \n \tif (!strcmp(var, \"diff.ignoresubmodules\")) {\n \t\tif (!value)\ndiff --git a/environment.c b/environment.c\nindex 5e9fab4d1a..87123f902c 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -46,8 +46,8 @@ char *git_commit_encoding;\n char *git_log_output_encoding;\n char *apply_default_whitespace;\n char *apply_default_ignorewhitespace;\n-const char *git_attributes_file;\n-const char *git_hooks_path;\n+char *git_attributes_file;\n+char *git_hooks_path;\n int zlib_compression_level = Z_BEST_SPEED;\n int pack_compression_level = Z_DEFAULT_COMPRESSION;\n int fsync_object_files = -1;\n@@ -60,7 +60,7 @@ size_t delta_base_cache_limit = 96 * 1024 * 1024;\n unsigned long big_file_threshold = 512 * 1024 * 1024;\n char *editor_program;\n char *askpass_program;\n-const char *excludes_file;\n+char *excludes_file;\n enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;\n enum eol core_eol = EOL_UNSET;\n int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;\ndiff --git a/environment.h b/environment.h\nindex f06024a457..c33f101b24 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -124,8 +124,8 @@ extern int warn_ambiguous_refs;\n extern int warn_on_object_refname_ambiguity;\n extern char *apply_default_whitespace;\n extern char *apply_default_ignorewhitespace;\n-extern const char *git_attributes_file;\n-extern const char *git_hooks_path;\n+extern char *git_attributes_file;\n+extern char *git_hooks_path;\n extern int zlib_compression_level;\n extern int pack_compression_level;\n extern size_t packed_git_window_size;\n@@ -222,7 +222,7 @@ extern char *git_log_output_encoding;\n \n extern char *editor_program;\n extern char *askpass_program;\n-extern const char *excludes_file;\n+extern char *excludes_file;\n \n /*\n  * Should we print an ellipsis after an abbreviated SHA-1 value\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 4b2f70ef44..d6e45ff09c 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -28,7 +28,7 @@ static void gpg_interface_lazy_init(void)\n \n static char *configured_signing_key;\n static char *ssh_default_key_command;\n-static const char *ssh_allowed_signers, *ssh_revocation_file;\n+static char *ssh_allowed_signers, *ssh_revocation_file;\n static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;\n \n struct gpg_format {\n@@ -766,10 +766,10 @@ static int git_gpg_config(const char *var, const char *value,\n \t\treturn git_config_string_dup(&ssh_default_key_command, var, value);\n \n \tif (!strcmp(var, \"gpg.ssh.allowedsignersfile\"))\n-\t\treturn git_config_pathname(&ssh_allowed_signers, var, value);\n+\t\treturn git_config_pathname_dup(&ssh_allowed_signers, var, value);\n \n \tif (!strcmp(var, \"gpg.ssh.revocationfile\"))\n-\t\treturn git_config_pathname(&ssh_revocation_file, var, value);\n+\t\treturn git_config_pathname_dup(&ssh_revocation_file, var, value);\n \n \tif (!strcmp(var, \"gpg.program\") || !strcmp(var, \"gpg.openpgp.program\"))\n \t\tfmtname = \"openpgp\";\ndiff --git a/mailmap.c b/mailmap.c\nindex 9bd11c90e6..b2efe29b3d 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -6,7 +6,7 @@\n #include \"object-store-ll.h\"\n #include \"setup.h\"\n \n-const char *git_mailmap_file;\n+char *git_mailmap_file;\n char *git_mailmap_blob;\n \n struct mailmap_info {\ndiff --git a/mailmap.h b/mailmap.h\nindex a1bc019b52..cbda9bc5e0 100644\n--- a/mailmap.h\n+++ b/mailmap.h\n@@ -3,7 +3,7 @@\n \n struct string_list;\n \n-extern const char *git_mailmap_file;\n+extern char *git_mailmap_file;\n extern char *git_mailmap_blob;\n \n int read_mailmap(struct string_list *map);\n-- \n2.44.0.872.g288abe5b5b\n\n"},{"id":"492417","messageId":"20240407010354.GH868358@coredump.intra.peff.net","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"[PATCH 08/11] http: use git_config_string_dup()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:03:54Z","receivedAt":"2024-04-07T01:03:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are many calls to git_config_string() in the HTTP code, leading to\npossible leaks if a config variable is seen multiple times. I split\nthese out from the other \"easy\" cases because the change to non-const\npointers has some follow-on effects:\n\n  1. When these variables are passed to var_override(), that should now\n     take a non-const pointer (and consequently can drop the cast it\n     passes to free).\n\n  2. That file also has the somewhat redundant set_from_env() helper,\n     which did _not_ free the existing value, and so has basically the\n     same leak as git_config_string(). Rather than adjust it to take a\n     non-const pointer, we can just swap it out for var_override(),\n     fixing the leak and reducing the number of helpers at the same\n     time.\n\nThis makes for a fairly big patch, but we have to do it all at once to\nkeep the compiler happy (or alternatively, insert a bunch of temporary\ncasts into http.c).\n\nNote that some of these are git_config_pathname_dup(); it seemed better\nto convert them all together, since the root of the problem (and the\nsolution) are essentially the same.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c | 105 +++++++++++++++++++++++++++------------------------------\n 1 file changed, 49 insertions(+), 56 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex e73b136e58..f2bcc9083e 100644\n--- a/http.c\n+++ b/http.c\n@@ -38,11 +38,11 @@ char curl_errorstr[CURL_ERROR_SIZE];\n \n static int curl_ssl_verify = -1;\n static int curl_ssl_try;\n-static const char *curl_http_version = NULL;\n-static const char *ssl_cert;\n-static const char *ssl_cert_type;\n-static const char *ssl_cipherlist;\n-static const char *ssl_version;\n+static char *curl_http_version;\n+static char *ssl_cert;\n+static char *ssl_cert_type;\n+static char *ssl_cipherlist;\n+static char *ssl_version;\n static struct {\n \tconst char *name;\n \tlong ssl_version;\n@@ -59,23 +59,23 @@ static struct {\n \t{ \"tlsv1.3\", CURL_SSLVERSION_TLSv1_3 },\n #endif\n };\n-static const char *ssl_key;\n-static const char *ssl_key_type;\n-static const char *ssl_capath;\n-static const char *curl_no_proxy;\n+static char *ssl_key;\n+static char *ssl_key_type;\n+static char *ssl_capath;\n+static char *curl_no_proxy;\n #ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY\n-static const char *ssl_pinnedkey;\n+static char *ssl_pinnedkey;\n #endif\n-static const char *ssl_cainfo;\n+static char *ssl_cainfo;\n static long curl_low_speed_limit = -1;\n static long curl_low_speed_time = -1;\n static int curl_ftp_no_epsv;\n-static const char *curl_http_proxy;\n-static const char *http_proxy_authmethod;\n+static char *curl_http_proxy;\n+static char *http_proxy_authmethod;\n \n-static const char *http_proxy_ssl_cert;\n-static const char *http_proxy_ssl_key;\n-static const char *http_proxy_ssl_ca_info;\n+static char *http_proxy_ssl_cert;\n+static char *http_proxy_ssl_key;\n+static char *http_proxy_ssl_ca_info;\n static struct credential proxy_cert_auth = CREDENTIAL_INIT;\n static int proxy_ssl_cert_password_required;\n \n@@ -95,7 +95,7 @@ static struct {\n \t */\n };\n #ifdef CURLGSSAPI_DELEGATION_FLAG\n-static const char *curl_deleg;\n+static char *curl_deleg;\n static struct {\n \tconst char *name;\n \tlong curl_deleg_param;\n@@ -108,11 +108,11 @@ static struct {\n \n static struct credential proxy_auth = CREDENTIAL_INIT;\n static const char *curl_proxyuserpwd;\n-static const char *curl_cookie_file;\n+static char *curl_cookie_file;\n static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n static int http_proactive_auth;\n-static const char *user_agent;\n+static char *user_agent;\n static int curl_empty_auth = -1;\n \n enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;\n@@ -366,28 +366,28 @@ static int http_options(const char *var, const char *value,\n \t\t\tconst struct config_context *ctx, void *data)\n {\n \tif (!strcmp(\"http.version\", var)) {\n-\t\treturn git_config_string(&curl_http_version, var, value);\n+\t\treturn git_config_string_dup(&curl_http_version, var, value);\n \t}\n \tif (!strcmp(\"http.sslverify\", var)) {\n \t\tcurl_ssl_verify = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \tif (!strcmp(\"http.sslcipherlist\", var))\n-\t\treturn git_config_string(&ssl_cipherlist, var, value);\n+\t\treturn git_config_string_dup(&ssl_cipherlist, var, value);\n \tif (!strcmp(\"http.sslversion\", var))\n-\t\treturn git_config_string(&ssl_version, var, value);\n+\t\treturn git_config_string_dup(&ssl_version, var, value);\n \tif (!strcmp(\"http.sslcert\", var))\n-\t\treturn git_config_pathname(&ssl_cert, var, value);\n+\t\treturn git_config_pathname_dup(&ssl_cert, var, value);\n \tif (!strcmp(\"http.sslcerttype\", var))\n-\t\treturn git_config_string(&ssl_cert_type, var, value);\n+\t\treturn git_config_string_dup(&ssl_cert_type, var, value);\n \tif (!strcmp(\"http.sslkey\", var))\n-\t\treturn git_config_pathname(&ssl_key, var, value);\n+\t\treturn git_config_pathname_dup(&ssl_key, var, value);\n \tif (!strcmp(\"http.sslkeytype\", var))\n-\t\treturn git_config_string(&ssl_key_type, var, value);\n+\t\treturn git_config_string_dup(&ssl_key_type, var, value);\n \tif (!strcmp(\"http.sslcapath\", var))\n-\t\treturn git_config_pathname(&ssl_capath, var, value);\n+\t\treturn git_config_pathname_dup(&ssl_capath, var, value);\n \tif (!strcmp(\"http.sslcainfo\", var))\n-\t\treturn git_config_pathname(&ssl_cainfo, var, value);\n+\t\treturn git_config_pathname_dup(&ssl_cainfo, var, value);\n \tif (!strcmp(\"http.sslcertpasswordprotected\", var)) {\n \t\tssl_cert_password_required = git_config_bool(var, value);\n \t\treturn 0;\n@@ -436,27 +436,27 @@ static int http_options(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(\"http.proxy\", var))\n-\t\treturn git_config_string(&curl_http_proxy, var, value);\n+\t\treturn git_config_string_dup(&curl_http_proxy, var, value);\n \n \tif (!strcmp(\"http.proxyauthmethod\", var))\n-\t\treturn git_config_string(&http_proxy_authmethod, var, value);\n+\t\treturn git_config_string_dup(&http_proxy_authmethod, var, value);\n \n \tif (!strcmp(\"http.proxysslcert\", var))\n-\t\treturn git_config_string(&http_proxy_ssl_cert, var, value);\n+\t\treturn git_config_string_dup(&http_proxy_ssl_cert, var, value);\n \n \tif (!strcmp(\"http.proxysslkey\", var))\n-\t\treturn git_config_string(&http_proxy_ssl_key, var, value);\n+\t\treturn git_config_string_dup(&http_proxy_ssl_key, var, value);\n \n \tif (!strcmp(\"http.proxysslcainfo\", var))\n-\t\treturn git_config_string(&http_proxy_ssl_ca_info, var, value);\n+\t\treturn git_config_string_dup(&http_proxy_ssl_ca_info, var, value);\n \n \tif (!strcmp(\"http.proxysslcertpasswordprotected\", var)) {\n \t\tproxy_ssl_cert_password_required = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n \tif (!strcmp(\"http.cookiefile\", var))\n-\t\treturn git_config_pathname(&curl_cookie_file, var, value);\n+\t\treturn git_config_pathname_dup(&curl_cookie_file, var, value);\n \tif (!strcmp(\"http.savecookies\", var)) {\n \t\tcurl_save_cookies = git_config_bool(var, value);\n \t\treturn 0;\n@@ -472,7 +472,7 @@ static int http_options(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(\"http.useragent\", var))\n-\t\treturn git_config_string(&user_agent, var, value);\n+\t\treturn git_config_string_dup(&user_agent, var, value);\n \n \tif (!strcmp(\"http.emptyauth\", var)) {\n \t\tif (value && !strcmp(\"auto\", value))\n@@ -484,7 +484,7 @@ static int http_options(const char *var, const char *value,\n \n \tif (!strcmp(\"http.delegation\", var)) {\n #ifdef CURLGSSAPI_DELEGATION_FLAG\n-\t\treturn git_config_string(&curl_deleg, var, value);\n+\t\treturn git_config_string_dup(&curl_deleg, var, value);\n #else\n \t\twarning(_(\"Delegation control is not supported with cURL < 7.22.0\"));\n \t\treturn 0;\n@@ -493,7 +493,7 @@ static int http_options(const char *var, const char *value,\n \n \tif (!strcmp(\"http.pinnedpubkey\", var)) {\n #ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY\n-\t\treturn git_config_pathname(&ssl_pinnedkey, var, value);\n+\t\treturn git_config_pathname_dup(&ssl_pinnedkey, var, value);\n #else\n \t\twarning(_(\"Public key pinning not supported with cURL < 7.39.0\"));\n \t\treturn 0;\n@@ -572,10 +572,10 @@ static void init_curl_http_auth(CURL *result)\n }\n \n /* *var must be free-able */\n-static void var_override(const char **var, char *value)\n+static void var_override(char **var, char *value)\n {\n \tif (value) {\n-\t\tfree((void *)*var);\n+\t\tfree(*var);\n \t\t*var = xstrdup(value);\n \t}\n }\n@@ -1208,13 +1208,6 @@ static CURL *get_curl_handle(void)\n \treturn result;\n }\n \n-static void set_from_env(const char **var, const char *envname)\n-{\n-\tconst char *val = getenv(envname);\n-\tif (val)\n-\t\t*var = val;\n-}\n-\n void http_init(struct remote *remote, const char *url, int proactive_auth)\n {\n \tchar *low_speed_limit;\n@@ -1291,14 +1284,14 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \tif (getenv(\"GIT_SSL_NO_VERIFY\"))\n \t\tcurl_ssl_verify = 0;\n \n-\tset_from_env(&ssl_cert, \"GIT_SSL_CERT\");\n-\tset_from_env(&ssl_cert_type, \"GIT_SSL_CERT_TYPE\");\n-\tset_from_env(&ssl_key, \"GIT_SSL_KEY\");\n-\tset_from_env(&ssl_key_type, \"GIT_SSL_KEY_TYPE\");\n-\tset_from_env(&ssl_capath, \"GIT_SSL_CAPATH\");\n-\tset_from_env(&ssl_cainfo, \"GIT_SSL_CAINFO\");\n+\tvar_override(&ssl_cert, getenv(\"GIT_SSL_CERT\"));\n+\tvar_override(&ssl_cert_type, getenv(\"GIT_SSL_CERT_TYPE\"));\n+\tvar_override(&ssl_key, getenv(\"GIT_SSL_KEY\"));\n+\tvar_override(&ssl_key_type, getenv(\"GIT_SSL_KEY_TYPE\"));\n+\tvar_override(&ssl_capath, getenv(\"GIT_SSL_CAPATH\"));\n+\tvar_override(&ssl_cainfo, getenv(\"GIT_SSL_CAINFO\"));\n \n-\tset_from_env(&user_agent, \"GIT_HTTP_USER_AGENT\");\n+\tvar_override(&user_agent, getenv(\"GIT_HTTP_USER_AGENT\"));\n \n \tlow_speed_limit = getenv(\"GIT_HTTP_LOW_SPEED_LIMIT\");\n \tif (low_speed_limit)\n@@ -1314,9 +1307,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \tif (max_requests < 1)\n \t\tmax_requests = DEFAULT_MAX_REQUESTS;\n \n-\tset_from_env(&http_proxy_ssl_cert, \"GIT_PROXY_SSL_CERT\");\n-\tset_from_env(&http_proxy_ssl_key, \"GIT_PROXY_SSL_KEY\");\n-\tset_from_env(&http_proxy_ssl_ca_info, \"GIT_PROXY_SSL_CAINFO\");\n+\tvar_override(&http_proxy_ssl_cert, getenv(\"GIT_PROXY_SSL_CERT\"));\n+\tvar_override(&http_proxy_ssl_key, getenv(\"GIT_PROXY_SSL_KEY\"));\n+\tvar_override(&http_proxy_ssl_ca_info, getenv(\"GIT_PROXY_SSL_CAINFO\"));\n \n \tif (getenv(\"GIT_PROXY_SSL_CERT_PASSWORD_PROTECTED\"))\n \t\tproxy_ssl_cert_password_required = 1;\n-- \n2.44.0.872.g288abe5b5b\n\n"},{"id":"492418","messageId":"20240407010429.GI868358@coredump.intra.peff.net","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"[PATCH 09/11] merge: use git_config_string_dup() for pull strategies","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:04:29Z","receivedAt":"2024-04-07T01:04:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Converting pull.twohead and pull.octopus to use git_config_string_dup()\nfixes possible leaks if we see those config variables defined multiple\ntimes. Doing so is mostly an \"easy\" case, except that we later may\nassign a string literal to pull_twohead (which is now a non-const\npointer).\n\nThat's actually not _too_ bad in practice, as it happens after we've\ndone all of our config parsing (so nobody would try to free it). And the\ncompiler won't complain unless -Wwrite-strings is used (and turning that\non creates a host of other warnings, some useful and some not). But\nwhile we're here let's future proof it as best we can.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/merge.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex c2be29ed2f..a9e5100e70 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -101,7 +101,7 @@ static struct strategy all_strategy[] = {\n \t{ \"subtree\",    NO_FAST_FORWARD | NO_TRIVIAL },\n };\n \n-static const char *pull_twohead, *pull_octopus;\n+static char *pull_twohead, *pull_octopus;\n \n enum ff_type {\n \tFF_NO,\n@@ -615,9 +615,9 @@ static int git_merge_config(const char *k, const char *v,\n \telse if (!strcmp(k, \"merge.verifysignatures\"))\n \t\tverify_signatures = git_config_bool(k, v);\n \telse if (!strcmp(k, \"pull.twohead\"))\n-\t\treturn git_config_string(&pull_twohead, k, v);\n+\t\treturn git_config_string_dup(&pull_twohead, k, v);\n \telse if (!strcmp(k, \"pull.octopus\"))\n-\t\treturn git_config_string(&pull_octopus, k, v);\n+\t\treturn git_config_string_dup(&pull_octopus, k, v);\n \telse if (!strcmp(k, \"commit.cleanup\"))\n \t\treturn git_config_string_dup(&cleanup_arg, k, v);\n \telse if (!strcmp(k, \"merge.ff\")) {\n@@ -1291,7 +1291,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (!pull_twohead) {\n \t\tchar *default_strategy = getenv(\"GIT_TEST_MERGE_ALGORITHM\");\n \t\tif (default_strategy && !strcmp(default_strategy, \"ort\"))\n-\t\t\tpull_twohead = \"ort\";\n+\t\t\tpull_twohead = xstrdup(\"ort\");\n \t}\n \n \tinit_diff_ui_defaults();\n-- \n2.44.0.872.g288abe5b5b\n\n"},{"id":"492419","messageId":"20240407010450.GJ868358@coredump.intra.peff.net","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"[PATCH 10/11] userdiff: use git_config_string_dup() when we can","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:04:50Z","receivedAt":"2024-04-07T01:04:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are many uses of git_config_string() in the userdiff code which\nhave the usual \"leak when we see the variable multiple times\" problem.\nWe can fix many of these in the usual way by switching to the _dup()\nvariant.\n\nBut note there are some adjacent ones we cannot fix right now: funcname\npatterns and word regexes default to string literals, and we can't use\nthose with our dup variant (which would try to free the literals). For\nnow let's convert what we can.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n userdiff.c | 6 +++---\n userdiff.h | 6 +++---\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/userdiff.c b/userdiff.c\nindex 92ef649c99..ba168f50b5 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -409,15 +409,15 @@ int userdiff_config(const char *k, const char *v)\n \tif (!strcmp(type, \"binary\"))\n \t\treturn parse_tristate(&drv->binary, k, v);\n \tif (!strcmp(type, \"command\"))\n-\t\treturn git_config_string(&drv->external, k, v);\n+\t\treturn git_config_string_dup(&drv->external, k, v);\n \tif (!strcmp(type, \"textconv\"))\n-\t\treturn git_config_string(&drv->textconv, k, v);\n+\t\treturn git_config_string_dup(&drv->textconv, k, v);\n \tif (!strcmp(type, \"cachetextconv\"))\n \t\treturn parse_bool(&drv->textconv_want_cache, k, v);\n \tif (!strcmp(type, \"wordregex\"))\n \t\treturn git_config_string(&drv->word_regex, k, v);\n \tif (!strcmp(type, \"algorithm\"))\n-\t\treturn git_config_string(&drv->algorithm, k, v);\n+\t\treturn git_config_string_dup(&drv->algorithm, k, v);\n \n \treturn 0;\n }\ndiff --git a/userdiff.h b/userdiff.h\nindex d726804c3e..7cae1f4da0 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -13,13 +13,13 @@ struct userdiff_funcname {\n \n struct userdiff_driver {\n \tconst char *name;\n-\tconst char *external;\n-\tconst char *algorithm;\n+\tchar *external;\n+\tchar *algorithm;\n \tint binary;\n \tstruct userdiff_funcname funcname;\n \tconst char *word_regex;\n \tconst char *word_regex_multi_byte;\n-\tconst char *textconv;\n+\tchar *textconv;\n \tstruct notes_cache *textconv_cache;\n \tint textconv_want_cache;\n };\n-- \n2.44.0.872.g288abe5b5b\n\n"},{"id":"492420","messageId":"20240407010704.GK868358@coredump.intra.peff.net","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"[PATCH 11/11] blame: use \"dup\" string_list for ignore-revs files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:07:04Z","receivedAt":"2024-04-07T01:07:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We feed items into ignore_revs_file_list in two ways: from config and\nfrom the command-line. Items from the command-line point to argv entries\nthat we don't own, but items from config are hea-allocated strings whose\nownership is handed to the string list when we insert them.\n\nBecause the string_list is declared with \"NODUP\", our string_list_clear()\ncall will not free the entries themselves. This is the right thing for\nthe argv entries, but means that we leak the allocated config entries.\n\nLet's declare the string list as owning its own strings, which means\nthat the argv entries will be copied.\n\nFor the config entries we would ideally use string_list_append_nodup()\nto just hand off ownership of our strings. But we insert them into the\nsorted list with string_list_insert(), which doesn't have a nodup\nvariant. Since this isn't a hot path, we can accept that the path\ninterpolation will produce a temporary string which is then freed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNot strictly related to this series, but something I noticed while\nconverting this spot in an earlier patch. I had thought originally we\ncould switch to avoid the extra copy altogether:\n\n  if (!value)\n\treturn config_error_nonbool();\n  string_list_insert(&ignore_revs_file_list, value);\n\nbut of course that is missing the call to interpolate_path().\n\nI imagine that it could also be further refactored to append while\nreading the config, and then sort after. That's more efficient overall,\nand would let us use append_nodup().\n\n builtin/blame.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 0b07ceb110..fa7f8fce09 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -67,7 +67,7 @@ static int no_whole_file_rename;\n static int show_progress;\n static char repeated_meta_color[COLOR_MAXLEN];\n static int coloring_mode;\n-static struct string_list ignore_revs_file_list = STRING_LIST_INIT_NODUP;\n+static struct string_list ignore_revs_file_list = STRING_LIST_INIT_DUP;\n static int mark_unblamable_lines;\n static int mark_ignored_lines;\n \n@@ -725,6 +725,7 @@ static int git_blame_config(const char *var, const char *value,\n \t\tif (ret)\n \t\t\treturn ret;\n \t\tstring_list_insert(&ignore_revs_file_list, str);\n+\t\tfree(str);\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"blame.markunblamablelines\")) {\n-- \n2.44.0.872.g288abe5b5b\n"},{"id":"492421","messageId":"20240407010858.GA1076556@coredump.intra.peff.net","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"Re: [PATCH 0/12] git_config_string() considered harmful","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:08:58Z","receivedAt":"2024-04-07T01:08:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 06, 2024 at 08:56:56PM -0400, Jeff King wrote:\n\n> Subject: Re: [PATCH 0/12] git_config_string() considered harmful\n> [...]\n>   [01/11]: config: make sequencer.c's git_config_string_dup() public\n>   [02/11]: config: add git_config_pathname_dup()\n>   [03/11]: config: prefer git_config_string_dup() for temp variables\n>   [04/11]: config: use git_config_string_dup() for open-coded equivalents\n>   [05/11]: config: use git_config_string_dup() to fix leaky open coding\n>   [06/11]: config: use git_config_string() in easy cases\n>   [07/11]: config: use git_config_pathname_dup() in easy cases\n>   [08/11]: http: use git_config_string_dup()\n>   [09/11]: merge: use git_config_string_dup() for pull strategies\n>   [10/11]: userdiff: use git_config_string_dup() when we can\n>   [11/11]: blame: use \"dup\" string_list for ignore-revs files\n\nThe cover letter subject is wrong, of course. :) I wrote it manually and\nended up squashing two of the patches at the last minute.\n\n-Peff\n"},{"id":"492431","messageId":"CAPig+cRyya=hUYtdzoqgFLQq0eEnkgH1ayZTLBGtM2gRrc2yTw@mail.gmail.com","threadId":"61283","inReplyTo":"20240407010037.GC868358@coredump.intra.peff.net","subject":"Re: [PATCH 03/11] config: prefer git_config_string_dup() for temp variables","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-07T01:50:23Z","receivedAt":"2024-04-07T01:50:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 6, 2024 at 9:00 PM Jeff King <peff@peff.net> wrote:\n> In some cases we may use git_config_string() or git_config_pathname() to\n> read a value into a temporary variable, and then either hand off memory\n> ownership of the new variable or free it. These are not potential leaks,\n> since we know that there is no previous value we are overwriting.\n>\n> However, it's worth converting these to use git_config_string_dup() and\n> git_config_pathname_dup(). It makes it easier to audit for leaky cases,\n> and possibly we can get rid of the leak-prone functions in the future.\n> Plus it lets the const-ness of our variables match their expected memory\n> ownership, which avoids some casts when calling free().\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/builtin/config.c b/builtin/config.c\n> @@ -282,11 +282,11 @@ static int format_config(struct strbuf *buf, const char *key_,\n> -                       const char *v;\n> -                       if (git_config_pathname(&v, key_, value_) < 0)\n> +                       char *v = NULL;\n> +                       if (git_config_pathname_dup(&v, key_, value_) < 0)\n>                                 return -1;\n>                         strbuf_addstr(buf, v);\n> -                       free((char *)v);\n> +                       free(v);\n\nThis revised code implies that git_config_pathname_dup() doesn't\nassign allocated memory to `v` in the \"failure\" case, thus it is safe\nto `return` immediately without calling free(v). However, the\ndocumentation for git_config_pathname_dup() and cousins doesn't state\nthis explicitly, which means the caller needs to peek into the\nimplementation of git_config_pathname_dup() to verify that it is safe\nto write code such as the above. Hence, should the documentation be\nupdated to explain that `v` won't be modified in the \"failure\" case?\n"},{"id":"492432","messageId":"CAPig+cST40pM5PdGA==o8aGFwv8kStzzC8=28NULUtV8OFJmLg@mail.gmail.com","threadId":"61283","inReplyTo":"20240407010228.GF868358@coredump.intra.peff.net","subject":"Re: [PATCH 06/11] config: use git_config_string() in easy cases","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-07T02:05:00Z","receivedAt":"2024-04-07T02:05:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 6, 2024 at 9:02 PM Jeff King <peff@peff.net> wrote:\n> config: use git_config_string() in easy cases\n\nDid you instead mean?\n\n    config: use git_config_string_dup() in easy cases\n\n> (like the one above), we can easily convert them to use a variant of the\n> helper that frees the existing value before writing over it. That works\n> because free(NULL) is a noop. What we can't change, though, is cases\n> where the pointer is initialized to a string literal, like:\n\nFun historical (historic?) fact: On NeXTSTEP operating system,\nfree(NULL) and realloc(NULL,n) would crash the program, so getting\nnon-native (open-source) code to run correctly often involved more\nthan a little effort..\n\n>   const char *foo = \"some string literal\";\n>\n> In that case it would be incorrect to free() the original value. Those\n> cases will take more refactoring to make them leak-free, and this patch\n> punts on that for now.\n>\n> In each case we switch the underlying variable from \"const char *\" to\n> \"char *\" to indicate that it will always point to allocated memory (and\n> our git_config_string_dup() interface enforces this).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n"},{"id":"492433","messageId":"CAPig+cTj=_1JAvCr-RWtZT8nZtccGYmrPgcAARx5XMGZso4BtQ@mail.gmail.com","threadId":"61283","inReplyTo":"20240407010704.GK868358@coredump.intra.peff.net","subject":"Re: [PATCH 11/11] blame: use \"dup\" string_list for ignore-revs files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-07T02:42:42Z","receivedAt":"2024-04-07T02:42:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 6, 2024 at 9:07 PM Jeff King <peff@peff.net> wrote:\n> We feed items into ignore_revs_file_list in two ways: from config and\n> from the command-line. Items from the command-line point to argv entries\n> that we don't own, but items from config are hea-allocated strings whose\n> ownership is handed to the string list when we insert them.\n\ns/hea/heap/\n\n> Because the string_list is declared with \"NODUP\", our string_list_clear()\n> call will not free the entries themselves. This is the right thing for\n> the argv entries, but means that we leak the allocated config entries.\n>\n> Let's declare the string list as owning its own strings, which means\n> that the argv entries will be copied.\n>\n> For the config entries we would ideally use string_list_append_nodup()\n> to just hand off ownership of our strings. But we insert them into the\n> sorted list with string_list_insert(), which doesn't have a nodup\n> variant. Since this isn't a hot path, we can accept that the path\n> interpolation will produce a temporary string which is then freed.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n"},{"id":"492434","messageId":"20240407024535.GA1585186@coredump.intra.peff.net","threadId":"61283","inReplyTo":"CAPig+cRyya=hUYtdzoqgFLQq0eEnkgH1ayZTLBGtM2gRrc2yTw@mail.gmail.com","subject":"Re: [PATCH 03/11] config: prefer git_config_string_dup() for temp variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T02:45:35Z","receivedAt":"2024-04-07T02:45:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 06, 2024 at 09:50:23PM -0400, Eric Sunshine wrote:\n\n> On Sat, Apr 6, 2024 at 9:00 PM Jeff King <peff@peff.net> wrote:\n> > In some cases we may use git_config_string() or git_config_pathname() to\n> > read a value into a temporary variable, and then either hand off memory\n> > ownership of the new variable or free it. These are not potential leaks,\n> > since we know that there is no previous value we are overwriting.\n> >\n> > However, it's worth converting these to use git_config_string_dup() and\n> > git_config_pathname_dup(). It makes it easier to audit for leaky cases,\n> > and possibly we can get rid of the leak-prone functions in the future.\n> > Plus it lets the const-ness of our variables match their expected memory\n> > ownership, which avoids some casts when calling free().\n> >\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> > diff --git a/builtin/config.c b/builtin/config.c\n> > @@ -282,11 +282,11 @@ static int format_config(struct strbuf *buf, const char *key_,\n> > -                       const char *v;\n> > -                       if (git_config_pathname(&v, key_, value_) < 0)\n> > +                       char *v = NULL;\n> > +                       if (git_config_pathname_dup(&v, key_, value_) < 0)\n> >                                 return -1;\n> >                         strbuf_addstr(buf, v);\n> > -                       free((char *)v);\n> > +                       free(v);\n> \n> This revised code implies that git_config_pathname_dup() doesn't\n> assign allocated memory to `v` in the \"failure\" case, thus it is safe\n> to `return` immediately without calling free(v). However, the\n> documentation for git_config_pathname_dup() and cousins doesn't state\n> this explicitly, which means the caller needs to peek into the\n> implementation of git_config_pathname_dup() to verify that it is safe\n> to write code such as the above. Hence, should the documentation be\n> updated to explain that `v` won't be modified in the \"failure\" case?\n\nThat's nothing new with my patch, though, is it? The same would apply\nfor git_config_pathname(). And git_config_string() for that matter. I'm\nalso struggling to imagine what it _would_ copy into \"v\" on failure\nanyway.\n\n-Peff\n"},{"id":"492435","messageId":"20240407024733.GB1585186@coredump.intra.peff.net","threadId":"61283","inReplyTo":"CAPig+cST40pM5PdGA==o8aGFwv8kStzzC8=28NULUtV8OFJmLg@mail.gmail.com","subject":"Re: [PATCH 06/11] config: use git_config_string() in easy cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T02:47:33Z","receivedAt":"2024-04-07T02:47:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 06, 2024 at 10:05:00PM -0400, Eric Sunshine wrote:\n\n> On Sat, Apr 6, 2024 at 9:02 PM Jeff King <peff@peff.net> wrote:\n> > config: use git_config_string() in easy cases\n> \n> Did you instead mean?\n> \n>     config: use git_config_string_dup() in easy cases\n\nOops, yes.\n\n> > (like the one above), we can easily convert them to use a variant of the\n> > helper that frees the existing value before writing over it. That works\n> > because free(NULL) is a noop. What we can't change, though, is cases\n> > where the pointer is initialized to a string literal, like:\n> \n> Fun historical (historic?) fact: On NeXTSTEP operating system,\n> free(NULL) and realloc(NULL,n) would crash the program, so getting\n> non-native (open-source) code to run correctly often involved more\n> than a little effort..\n\nI'm happy that's no longer something we need to worry about. ;) IIRC we\nhad some discussion long ago about whether free(NULL) needed to be\nprotected, and came to the conclusion that it's OK everywhere.\n\n(There's a discussion in a nearby thread about a possible git_free()\nwrapper. The point there is errno handling, but of course it could also\ncover this case for us if we found a modern platform where it was a\nproblem).\n\n-Peff\n"},{"id":"492471","messageId":"ef8e5f43-5f27-4f0a-acf5-cf4f8281a8f8@gmail.com","threadId":"61283","inReplyTo":"20240407005656.GA436890@coredump.intra.peff.net","subject":"Re: [PATCH 0/12] git_config_string() considered harmful","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-07T17:58:02Z","receivedAt":"2024-04-07T17:58:09Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Sat, Apr 06, 2024 at 08:56:56PM -0400, Jeff King wrote:\n\n> And it's not just git_config_pathname(), but really git_config_string(),\n\nIndeed.\n\nAfter Junio's series and yours, I'm on the fence now, but my envision was\nto introduce:\n\n--- >8 ---\ndiff --git a/config.c b/config.c\nindex eebce8c7e0..7322bdfb94 100644\n--- a/config.c\n+++ b/config.c\n@@ -1345,6 +1345,15 @@ int git_config_string(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+int git_config_strbuf(struct strbuf *dest, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tstrbuf_reset(dest);\n+\tstrbuf_addstr(dest, value);\n+\treturn 0;\n+}\n+\n int git_config_pathname(const char **dest, const char *var, const char *value)\n {\n \tif (!value)\ndiff --git a/config.h b/config.h\nindex f4966e3749..46e3137612 100644\n--- a/config.h\n+++ b/config.h\n@@ -282,6 +282,12 @@ int git_config_bool(const char *, const char *);\n  */\n int git_config_string(const char **, const char *, const char *);\n \n+/**\n+ * Copies the value string into the `dest` parameter; if no\n+ * string is given, prints an error message and returns -1.\n+ */\n+int git_config_strbuf(struct strbuf *, const char *, const char *);\n+\n /**\n  * Similar to `git_config_string`, but expands `~` or `~user` into the\n  * user's home directory when found at the beginning of the path.\n--- 8< ---\n\nTo allow uses like:\n\n--- >8 ---\ndiff --git a/config.c b/config.c\nindex 7322bdfb94..03884fa782 100644\n--- a/config.c\n+++ b/config.c\n@@ -1572,7 +1572,7 @@ static int git_default_core_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"core.editor\"))\n-\t\treturn git_config_string(&editor_program, var, value);\n+\t\treturn git_config_strbuf(&editor_program, var, value);\n \n \tif (!strcmp(var, \"core.commentchar\") ||\n \t    !strcmp(var, \"core.commentstring\")) {\ndiff --git a/editor.c b/editor.c\nindex b67b802ddf..618c193249 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -27,8 +27,8 @@ const char *git_editor(void)\n \tconst char *editor = getenv(\"GIT_EDITOR\");\n \tint terminal_is_dumb = is_terminal_dumb();\n \n-\tif (!editor && editor_program)\n-\t\teditor = editor_program;\n+\tif (!editor && editor_program.len)\n+\t\teditor = editor_program.buf;\n \tif (!editor && !terminal_is_dumb)\n \t\teditor = getenv(\"VISUAL\");\n \tif (!editor)\ndiff --git a/environment.c b/environment.c\nindex a73ba9c12c..b5073ff972 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -58,7 +58,7 @@ size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n size_t delta_base_cache_limit = 96 * 1024 * 1024;\n unsigned long big_file_threshold = 512 * 1024 * 1024;\n-const char *editor_program;\n+struct strbuf editor_program = STRBUF_INIT;\n const char *askpass_program;\n const char *excludes_file;\n enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;\ndiff --git a/environment.h b/environment.h\nindex 05fd94d7be..c20898345e 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -220,7 +220,7 @@ const char *get_commit_output_encoding(void);\n extern const char *git_commit_encoding;\n extern const char *git_log_output_encoding;\n \n-extern const char *editor_program;\n+extern struct strbuf editor_program;\n extern const char *askpass_program;\n extern const char *excludes_file;\n"},{"id":"492581","messageId":"20240408205511.GB1639295@coredump.intra.peff.net","threadId":"61283","inReplyTo":"ef8e5f43-5f27-4f0a-acf5-cf4f8281a8f8@gmail.com","subject":"Re: [PATCH 0/12] git_config_string() considered harmful","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-08T20:55:11Z","receivedAt":"2024-04-08T20:55:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 07, 2024 at 07:58:02PM +0200, Rubén Justo wrote:\n\n> After Junio's series and yours, I'm on the fence now, but my envision was\n> to introduce:\n> \n> --- >8 ---\n> diff --git a/config.c b/config.c\n> index eebce8c7e0..7322bdfb94 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1345,6 +1345,15 @@ int git_config_string(const char **dest, const char *var, const char *value)\n>  \treturn 0;\n>  }\n>  \n> +int git_config_strbuf(struct strbuf *dest, const char *var, const char *value)\n> +{\n> +\tif (!value)\n> +\t\treturn config_error_nonbool(var);\n> +\tstrbuf_reset(dest);\n> +\tstrbuf_addstr(dest, value);\n> +\treturn 0;\n> +}\n> +\n>  int git_config_pathname(const char **dest, const char *var, const char *value)\n>  {\n>  \tif (!value)\n\nHmm. I think that is nice in some ways, because it is a much bigger\nsignal about memory ownership than just dropping \"const\" from the\npointer.\n\nBut it is less nice in other ways. Every _user_ of the value now needs\nto care that it is a strbuf, and use foo.buf consistently (which you\nobviously noticed). Likewise, any downstream writers of the variable\nneed to treat it like a strbuf, too. So the parse-options OPT_FILENAME()\nmacro, for example, needs to be replaced with a strbuf-aware variant\n(though arguably that is an improvement, as using the wrong one would\nfail catastrophically, whereas using a non-const pointer with\nOPT_FILENAME() creates a subtle bug).\n\nI'm also not sure what the solution is for setting default values, like:\n\n  const char *my_config = \"default\";\n\nOf course that is a problem with my solution, too. Perhaps in the long\nrun we need to accept that they should always default to NULL, and\nreaders of the variable need to fill in the default when they look at it\n(possibly with an accessor function).\n\nOr I guess the alternative is to stop using bare pointers, and carry a\nbit which tells us if something is heap-allocated. Which starts to look\nan awful lot like a strbuf. ;)\n\nI think in the past we've talked about being able to initialize a strbuf\nlike:\n\n  struct strbuf foo = STRBUF_INIT_VAL(\"bar\");\n\nThat would use \"bar\" instead of the usual slopbuf, but we can still tell\nit's not a heap buffer by the fact that foo.alloc is 0.\n\n-Peff\n"},{"id":"492824","messageId":"f4dbc3e7-5b8c-46d4-a7d6-122e34e0d922@gmail.com","threadId":"61283","inReplyTo":"20240408205511.GB1639295@coredump.intra.peff.net","subject":"Re: [PATCH 0/12] git_config_string() considered harmful","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-11T23:36:46Z","receivedAt":"2024-04-11T23:36:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Apr 08, 2024 at 04:55:11PM -0400, Jeff King wrote:\n> On Sun, Apr 07, 2024 at 07:58:02PM +0200, Rubén Justo wrote:\n> \n> > After Junio's series and yours, I'm on the fence now, but my envision was\n> > to introduce:\n> > \n> > --- >8 ---\n> > diff --git a/config.c b/config.c\n> > index eebce8c7e0..7322bdfb94 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -1345,6 +1345,15 @@ int git_config_string(const char **dest, const char *var, const char *value)\n> >  \treturn 0;\n> >  }\n> >  \n> > +int git_config_strbuf(struct strbuf *dest, const char *var, const char *value)\n> > +{\n> > +\tif (!value)\n> > +\t\treturn config_error_nonbool(var);\n> > +\tstrbuf_reset(dest);\n> > +\tstrbuf_addstr(dest, value);\n> > +\treturn 0;\n> > +}\n> > +\n> >  int git_config_pathname(const char **dest, const char *var, const char *value)\n> >  {\n> >  \tif (!value)\n> \n> Hmm. I think that is nice in some ways, because it is a much bigger\n> signal about memory ownership than just dropping \"const\" from the\n> pointer.\n> \n> But it is less nice in other ways. Every _user_ of the value now needs\n> to care that it is a strbuf, and use foo.buf consistently (which you\n> obviously noticed). Likewise, any downstream writers of the variable\n> need to treat it like a strbuf, too. So the parse-options OPT_FILENAME()\n> macro, for example, needs to be replaced with a strbuf-aware variant\n> (though arguably that is an improvement, as using the wrong one would\n> fail catastrophically, whereas using a non-const pointer with\n> OPT_FILENAME() creates a subtle bug).\n> \n> I'm also not sure what the solution is for setting default values, like:\n> \n>   const char *my_config = \"default\";\n> \n> Of course that is a problem with my solution, too. Perhaps in the long\n> run we need to accept that they should always default to NULL, and\n> readers of the variable need to fill in the default when they look at it\n> (possibly with an accessor function).\n> \n> Or I guess the alternative is to stop using bare pointers, and carry a\n> bit which tells us if something is heap-allocated. Which starts to look\n> an awful lot like a strbuf. ;)\n> \n> I think in the past we've talked about being able to initialize a strbuf\n> like:\n> \n>   struct strbuf foo = STRBUF_INIT_VAL(\"bar\");\n> \n> That would use \"bar\" instead of the usual slopbuf, but we can still tell\n> it's not a heap buffer by the fact that foo.alloc is 0.\n> \n> -Peff\n\nHi Peff.\n\nThanks for your ideas.\n\nFor the globals we have in environment.h, maybe we can keep them const\nand avoid the other inconveniences, doing something like:\n\ndiff --git a/config.c b/config.c\nindex 146856567a..ead3565c27 100644\n--- a/config.c\n+++ b/config.c\n@@ -1671,8 +1671,13 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n                return 0;\n        }\n\n-       if (!strcmp(var, \"core.editor\"))\n-               return git_config_string(&editor_program, var, value);\n+       if (!strcmp(var, \"core.editor\")) {\n+               static struct strbuf editor_program_ = STRBUF_INIT;\n+               if (git_config_strbuf(&editor_program_, var, value))\n+                       return -1;\n+               editor_program = editor_program_.buf;\n+               return 0;\n+       }\n\n        if (!strcmp(var, \"core.commentchar\")) {\n                if (!value)\n\n"}]}