| Message ID | 20240407010037.GC868358@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | git_config_string() considered harmful | expand |
On Sat, Apr 6, 2024 at 9:00 PM Jeff King <peff@peff.net> wrote: > In some cases we may use git_config_string() or git_config_pathname() to > read a value into a temporary variable, and then either hand off memory > ownership of the new variable or free it. These are not potential leaks, > since we know that there is no previous value we are overwriting. > > However, it's worth converting these to use git_config_string_dup() and > git_config_pathname_dup(). It makes it easier to audit for leaky cases, > and possibly we can get rid of the leak-prone functions in the future. > Plus it lets the const-ness of our variables match their expected memory > ownership, which avoids some casts when calling free(). > > Signed-off-by: Jeff King <peff@peff.net> > --- > diff --git a/builtin/config.c b/builtin/config.c > @@ -282,11 +282,11 @@ static int format_config(struct strbuf *buf, const char *key_, > - const char *v; > - if (git_config_pathname(&v, key_, value_) < 0) > + char *v = NULL; > + if (git_config_pathname_dup(&v, key_, value_) < 0) > return -1; > strbuf_addstr(buf, v); > - free((char *)v); > + free(v); This revised code implies that git_config_pathname_dup() doesn't assign allocated memory to `v` in the "failure" case, thus it is safe to `return` immediately without calling free(v). However, the documentation for git_config_pathname_dup() and cousins doesn't state this explicitly, which means the caller needs to peek into the implementation of git_config_pathname_dup() to verify that it is safe to write code such as the above. Hence, should the documentation be updated to explain that `v` won't be modified in the "failure" case?
On Sat, Apr 06, 2024 at 09:50:23PM -0400, Eric Sunshine wrote: > On Sat, Apr 6, 2024 at 9:00 PM Jeff King <peff@peff.net> wrote: > > In some cases we may use git_config_string() or git_config_pathname() to > > read a value into a temporary variable, and then either hand off memory > > ownership of the new variable or free it. These are not potential leaks, > > since we know that there is no previous value we are overwriting. > > > > However, it's worth converting these to use git_config_string_dup() and > > git_config_pathname_dup(). It makes it easier to audit for leaky cases, > > and possibly we can get rid of the leak-prone functions in the future. > > Plus it lets the const-ness of our variables match their expected memory > > ownership, which avoids some casts when calling free(). > > > > Signed-off-by: Jeff King <peff@peff.net> > > --- > > diff --git a/builtin/config.c b/builtin/config.c > > @@ -282,11 +282,11 @@ static int format_config(struct strbuf *buf, const char *key_, > > - const char *v; > > - if (git_config_pathname(&v, key_, value_) < 0) > > + char *v = NULL; > > + if (git_config_pathname_dup(&v, key_, value_) < 0) > > return -1; > > strbuf_addstr(buf, v); > > - free((char *)v); > > + free(v); > > This revised code implies that git_config_pathname_dup() doesn't > assign allocated memory to `v` in the "failure" case, thus it is safe > to `return` immediately without calling free(v). However, the > documentation for git_config_pathname_dup() and cousins doesn't state > this explicitly, which means the caller needs to peek into the > implementation of git_config_pathname_dup() to verify that it is safe > to write code such as the above. Hence, should the documentation be > updated to explain that `v` won't be modified in the "failure" case? That's nothing new with my patch, though, is it? The same would apply for git_config_pathname(). And git_config_string() for that matter. I'm also struggling to imagine what it _would_ copy into "v" on failure anyway. -Peff
diff --git a/builtin/blame.c b/builtin/blame.c index db1f56de61..0b07ceb110 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -718,10 +718,10 @@ static int git_blame_config(const char *var, const char *value, return 0; } if (!strcmp(var, "blame.ignorerevsfile")) { - const char *str; + char *str = NULL; int ret; - ret = git_config_pathname(&str, var, value); + ret = git_config_pathname_dup(&str, var, value); if (ret) return ret; string_list_insert(&ignore_revs_file_list, str); diff --git a/builtin/config.c b/builtin/config.c index 0015620dde..fc5d96bb4c 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -282,11 +282,11 @@ static int format_config(struct strbuf *buf, const char *key_, else strbuf_addstr(buf, v ? "true" : "false"); } else if (type == TYPE_PATH) { - const char *v; - if (git_config_pathname(&v, key_, value_) < 0) + char *v = NULL; + if (git_config_pathname_dup(&v, key_, value_) < 0) return -1; strbuf_addstr(buf, v); - free((char *)v); + free(v); } else if (type == TYPE_EXPIRY_DATE) { timestamp_t t; if (git_config_expiry_date(&t, key_, value_) < 0) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 56d8a77ed7..15ed81a3f6 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -168,13 +168,13 @@ static int receive_pack_config(const char *var, const char *value, } if (strcmp(var, "receive.fsck.skiplist") == 0) { - const char *path; + char *path = NULL; - if (git_config_pathname(&path, var, value)) + if (git_config_pathname_dup(&path, var, value)) return 1; strbuf_addf(&fsck_msg_types, "%cskiplist=%s", fsck_msg_types.len ? ',' : '=', path); - free((char *)path); + free(path); return 0; } diff --git a/fetch-pack.c b/fetch-pack.c index 091f9a80a9..fd59c497b4 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1863,13 +1863,13 @@ static int fetch_pack_config_cb(const char *var, const char *value, const char *msg_id; if (strcmp(var, "fetch.fsck.skiplist") == 0) { - const char *path; + char *path = NULL; - if (git_config_pathname(&path, var, value)) + if (git_config_pathname_dup(&path, var, value)) return 1; strbuf_addf(&fsck_msg_types, "%cskiplist=%s", fsck_msg_types.len ? ',' : '=', path); - free((char *)path); + free(path); return 0; } diff --git a/fsck.c b/fsck.c index 78af29d264..a9d27a660f 100644 --- a/fsck.c +++ b/fsck.c @@ -1274,13 +1274,13 @@ int git_fsck_config(const char *var, const char *value, const char *msg_id; if (strcmp(var, "fsck.skiplist") == 0) { - const char *path; + char *path = NULL; struct strbuf sb = STRBUF_INIT; - if (git_config_pathname(&path, var, value)) + if (git_config_pathname_dup(&path, var, value)) return 1; strbuf_addf(&sb, "skiplist=%s", path); - free((char *)path); + free(path); fsck_set_msg_types(options, sb.buf); strbuf_release(&sb); return 0; diff --git a/remote.c b/remote.c index 2b650b813b..09912bebf1 100644 --- a/remote.c +++ b/remote.c @@ -428,38 +428,38 @@ static int handle_config(const char *key, const char *value, else if (!strcmp(subkey, "prunetags")) remote->prune_tags = git_config_bool(key, value); else if (!strcmp(subkey, "url")) { - const char *v; - if (git_config_string(&v, key, value)) + char *v = NULL; + if (git_config_string_dup(&v, key, value)) return -1; add_url(remote, v); } else if (!strcmp(subkey, "pushurl")) { - const char *v; - if (git_config_string(&v, key, value)) + char *v = NULL; + if (git_config_string_dup(&v, key, value)) return -1; add_pushurl(remote, v); } else if (!strcmp(subkey, "push")) { - const char *v; - if (git_config_string(&v, key, value)) + char *v = NULL; + if (git_config_string_dup(&v, key, value)) return -1; refspec_append(&remote->push, v); - free((char *)v); + free(v); } else if (!strcmp(subkey, "fetch")) { - const char *v; - if (git_config_string(&v, key, value)) + char *v = NULL; + if (git_config_string_dup(&v, key, value)) return -1; refspec_append(&remote->fetch, v); - free((char *)v); + free(v); } else if (!strcmp(subkey, "receivepack")) { - const char *v; - if (git_config_string(&v, key, value)) + char *v = NULL; + if (git_config_string_dup(&v, key, value)) return -1; if (!remote->receivepack) remote->receivepack = v; else error(_("more than one receivepack given, using the first")); } else if (!strcmp(subkey, "uploadpack")) { - const char *v; - if (git_config_string(&v, key, value)) + char *v = NULL; + if (git_config_string_dup(&v, key, value)) return -1; if (!remote->uploadpack) remote->uploadpack = v; diff --git a/setup.c b/setup.c index f4b32f76e3..9f35a27978 100644 --- a/setup.c +++ b/setup.c @@ -1176,13 +1176,13 @@ static int safe_directory_cb(const char *key, const char *value, } else if (!strcmp(value, "*")) { data->is_safe = 1; } else { - const char *interpolated = NULL; + char *interpolated = NULL; - if (!git_config_pathname(&interpolated, key, value) && + if (!git_config_pathname_dup(&interpolated, key, value) && !fspathcmp(data->path, interpolated ? interpolated : value)) data->is_safe = 1; - free((char *)interpolated); + free(interpolated); } return 0;
In some cases we may use git_config_string() or git_config_pathname() to read a value into a temporary variable, and then either hand off memory ownership of the new variable or free it. These are not potential leaks, since we know that there is no previous value we are overwriting. However, it's worth converting these to use git_config_string_dup() and git_config_pathname_dup(). It makes it easier to audit for leaky cases, and possibly we can get rid of the leak-prone functions in the future. Plus it lets the const-ness of our variables match their expected memory ownership, which avoids some casts when calling free(). Signed-off-by: Jeff King <peff@peff.net> --- builtin/blame.c | 4 ++-- builtin/config.c | 6 +++--- builtin/receive-pack.c | 6 +++--- fetch-pack.c | 6 +++--- fsck.c | 6 +++--- remote.c | 28 ++++++++++++++-------------- setup.c | 6 +++--- 7 files changed, 31 insertions(+), 31 deletions(-)