{"thread":{"id":"50054","subject":"[PATCH 0/3] setup: add `clear_repository_format()`","startedAt":"2018-12-18T07:25:47Z","lastAt":"2019-03-06T04:56:46Z","messageCount":40,"participants":["Martin Ågren","brian m. carlson","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"365517","messageId":"20181218072528.3870492-1-martin.agren@gmail.com","threadId":"50054","inReplyTo":null,"subject":"[PATCH 0/3] setup: add `clear_repository_format()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-12-18T07:25:25Z","receivedAt":"2018-12-18T07:25:47Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Patch 3 adds `clear_repository_format()` to plug a few memory leaks\nrelated to `struct repository_format`. In preparation for that, patch 2\ntightens up some code which uses a possibly-undefined value from the\nstruct. Patch 1 drops a return value that no-one has ever used.\n\nCc-ing Peff, who introduced `struct repository_format`, and brian, who\nintroduced its `hash_algo` field (see patch 2).\n\nMartin Ågren (3):\n setup: drop return value from `read_repository_format()`\n setup: do not use invalid `repository_format`\n setup: add `clear_repository_format()`\n\n cache.h | 13 +++++++++----\n repository.c | 1 +\n setup.c | 19 ++++++++++++++++---\n 3 files changed, 26 insertions(+), 7 deletions(-)\n\n-- \n2.20.1\n\n"},{"id":"365518","messageId":"20181218072528.3870492-2-martin.agren@gmail.com","threadId":"50054","inReplyTo":"20181218072528.3870492-1-martin.agren@gmail.com","subject":"[PATCH 1/3] setup: drop return value from `read_repository_format()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-12-18T07:25:26Z","receivedAt":"2018-12-18T07:25:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"No-one looks at the return value, so we might as well drop it. It's\nstill available as `format->version`.\n\nIn v1 of what became commit 2cc7c2c737 (\"setup: refactor repo format\nreading and verification\", 2016-03-11), this function actually had\nreturn type \"void\", but that was changed in v2. Almost three years\nlater, no-one has used this return value.\n\nSigned-off-by: Martin Ågren \n---\n I only discovered the full history after writing the patch. Had I known\n it from the beginning, maybe I'd have just skipped this step, but I was\n sufficiently disturbed by the redundant and unused return value that I\n dropped it before working on the actual meat of this series.\n\n cache.h | 7 +++----\n setup.c | 3 +--\n 2 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex ca36b44ee0..8b9e592c65 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -974,11 +974,10 @@ struct repository_format {\n \n /*\n * Read the repository format characteristics from the config file \"path\" into\n- * \"format\" struct. Returns the numeric version. On error, -1 is returned,\n- * format->version is set to -1, and all other fields in the struct are\n- * undefined.\n+ * \"format\" struct. On error, format->version is set to -1, and all other\n+ * fields in the struct are undefined.\n */\n-int read_repository_format(struct repository_format *format, const char *path);\n+void read_repository_format(struct repository_format *format, const char *path);\n \n /*\n * Verify that the repository described by repository_format is something we\ndiff --git a/setup.c b/setup.c\nindex 1be5037f12..27747af7a3 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -509,7 +509,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \treturn 0;\n }\n \n-int read_repository_format(struct repository_format *format, const char *path)\n+void read_repository_format(struct repository_format *format, const char *path)\n {\n \tmemset(format, 0, sizeof(*format));\n \tformat->version = -1;\n@@ -517,7 +517,6 @@ int read_repository_format(struct repository_format *format, const char *path)\n \tformat->hash_algo = GIT_HASH_SHA1;\n \tstring_list_init(&format->unknown_extensions, 1);\n \tgit_config_from_file(check_repo_format, path, format);\n-\treturn format->version;\n }\n \n int verify_repository_format(const struct repository_format *format,\n-- \n2.20.1\n\n"},{"id":"365519","messageId":"20181218072528.3870492-3-martin.agren@gmail.com","threadId":"50054","inReplyTo":"20181218072528.3870492-1-martin.agren@gmail.com","subject":"[PATCH 2/3] setup: do not use invalid `repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-12-18T07:25:27Z","receivedAt":"2018-12-18T07:26:01Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If `read_repository_format()` encounters an error, `format->version`\nwill be -1 and all other fields of `format` will be undefined. However,\nin `setup_git_directory_gently()`, we use `repo_fmt.hash_algo`\nregardless of the value of `repo_fmt.version`.\n\nThis can be observed by adding this to the end of\n`read_repository_format()`:\n\n\tif (format->version == -1)\n\t\tformat->hash_algo = 0; /* no-one should peek at this! */\n\nThis causes, e.g., \"git branch -m q q2 without config should succeed\" in\nt3200 to fail with \"fatal: Failed to resolve HEAD as a valid ref.\"\nbecause it has moved .git/config out of the way and is now trying to use\na bad hash algorithm.\n\nCheck that `version` is non-negative before using `hash_algo`.\n\nThis patch adds no tests, but do note that if we skip this patch, the\nnext patch would cause existing tests to fail as outlined above.\n\nSigned-off-by: Martin Ågren \n---\n I fully admit to not understanding all of this setup code, neither in\n its current incarnation, nor in terms of an ideal end game. This check\n seems like a good thing to do though.\n\n setup.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex 27747af7a3..52c3c9d31f 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1138,7 +1138,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\t\tgitdir = DEFAULT_GIT_DIR_ENVIRONMENT;\n \t\t\tsetup_git_env(gitdir);\n \t\t}\n-\t\tif (startup_info->have_repository)\n+\t\tif (startup_info->have_repository && repo_fmt.version > -1)\n \t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n \t}\n \n-- \n2.20.1\n\n"},{"id":"365520","messageId":"20181218072528.3870492-4-martin.agren@gmail.com","threadId":"50054","inReplyTo":"20181218072528.3870492-1-martin.agren@gmail.com","subject":"[PATCH 3/3] setup: add `clear_repository_format()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-12-18T07:25:28Z","receivedAt":"2018-12-18T07:26:07Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After we set up a `struct repository_format`, it owns various pieces of\nallocated memory. We then either use those members, because we decide we\nwant to use the \"candidate\" repository format, or we discard the\ncandidate / scratch space. In the first case, we transfer ownership of\nthe memory to a few global variables. In the latter case, we just\nsilently drop the struct and end up leaking memory.\n\nIntroduce a function `clear_repository_format()` which frees the memory\nthe struct holds on to. Call it in the code paths where we currently\nleak the memory. Also call it in the error path of\n`read_repository_format()` to clean up any partial result.\n\nFor hygiene, we need to at least set the pointers that we free to NULL.\nFor future-proofing, let's zero the entire struct instead. It just means\nthat in the error path of `read_...()` we need to restore the error\nsentinel in the `version` field.\n\nWe could take this opportunity to stop claiming that all fields except\n`version` are undefined in case of an error. On the other hand, having\nthem defined as zero is not much better than having them undefined. We\ncould define them to some fallback configuration (`is_bare = -1` and\n`hash_algo = GIT_HASH_SHA1`?), but \"clear()\" and/or \"read()\" seem like\nthe wrong places to enforce fallback configurations. Let's leave things\nas \"undefined\" instead to encourage users to check `version`.\n\nSigned-off-by: Martin Ågren \n---\n The error state can always be defined later. Defining it now, then\n trying to backpedal, is probably not so fun. Filling the struct with\n non-zero values might help flush out bugs like the one fixed in the\n previous patch, but I'm wary of going that far in this patch.\n\n cache.h | 6 ++++++\n repository.c | 1 +\n setup.c | 14 ++++++++++++++\n 3 files changed, 21 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 8b9e592c65..53ac01efa7 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -979,6 +979,12 @@ struct repository_format {\n */\n void read_repository_format(struct repository_format *format, const char *path);\n \n+/*\n+ * Free the memory held onto by `format`, but not the struct itself.\n+ * (No need to use this after `read_repository_format()` fails.)\n+ */\n+void clear_repository_format(struct repository_format *format);\n+\n /*\n * Verify that the repository described by repository_format is something we\n * can read. If it is, return 0. Otherwise, return -1, and \"err\" will describe\ndiff --git a/repository.c b/repository.c\nindex 5dd1486718..efa9d1d960 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -159,6 +159,7 @@ int repo_init(struct repository *repo,\n \tif (worktree)\n \t\trepo_set_worktree(repo, worktree);\n \n+\tclear_repository_format(&format);\n \treturn 0;\n \n error:\ndiff --git a/setup.c b/setup.c\nindex 52c3c9d31f..babe5ea156 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -517,6 +517,18 @@ void read_repository_format(struct repository_format *format, const char *path)\n \tformat->hash_algo = GIT_HASH_SHA1;\n \tstring_list_init(&format->unknown_extensions, 1);\n \tgit_config_from_file(check_repo_format, path, format);\n+\tif (format->version == -1) {\n+\t\tclear_repository_format(format);\n+\t\tformat->version = -1;\n+\t}\n+}\n+\n+void clear_repository_format(struct repository_format *format)\n+{\n+\tstring_list_clear(&format->unknown_extensions, 0);\n+\tfree(format->work_tree);\n+\tfree(format->partial_clone);\n+\tmemset(format, 0, sizeof(*format));\n }\n \n int verify_repository_format(const struct repository_format *format,\n@@ -1043,9 +1055,11 @@ int discover_git_directory(struct strbuf *commondir,\n \t\tstrbuf_release(&err);\n \t\tstrbuf_setlen(commondir, commondir_offset);\n \t\tstrbuf_setlen(gitdir, gitdir_offset);\n+\t\tclear_repository_format(&candidate);\n \t\treturn -1;\n \t}\n \n+\tclear_repository_format(&candidate);\n \treturn 0;\n }\n \n-- \n2.20.1\n\n"},{"id":"365558","messageId":"20181219001801.GA520988@genre.crustytoothpaste.net","threadId":"50054","inReplyTo":"20181218072528.3870492-3-martin.agren@gmail.com","subject":"Re: [PATCH 2/3] setup: do not use invalid `repository_format`","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-12-19T00:18:01Z","receivedAt":"2018-12-19T00:18:11Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Dec 18, 2018 at 08:25:27AM +0100, Martin Ågren wrote:\n> I fully admit to not understanding all of this setup code, neither in\n> its current incarnation, nor in terms of an ideal end game. This check\n> seems like a good thing to do though.\n\nIt's definitely complex.\n\n> diff --git a/setup.c b/setup.c\n> index 27747af7a3..52c3c9d31f 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1138,7 +1138,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> \t\t\t\tgitdir = DEFAULT_GIT_DIR_ENVIRONMENT;\n> \t\t\tsetup_git_env(gitdir);\n> \t\t}\n> -\t\tif (startup_info->have_repository)\n> +\t\tif (startup_info->have_repository && repo_fmt.version > -1)\n> \t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n> \t}\n\nI think this change is fine, because we initialize the value in\nthe_repository elsewhere, and if there's no repository, this should\nnever have a value other than the default anyway.\n\nI looked at the other patches in the series and thought they looked sane\nas well.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"365577","messageId":"20181219152735.GA14802@sigill.intra.peff.net","threadId":"50054","inReplyTo":"20181218072528.3870492-2-martin.agren@gmail.com","subject":"Re: [PATCH 1/3] setup: drop return value from `read_repository_format()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-19T15:27:35Z","receivedAt":"2018-12-19T15:27:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 18, 2018 at 08:25:26AM +0100, Martin Ågren wrote:\n\n> No-one looks at the return value, so we might as well drop it. It's\n> still available as `format->version`.\n> \n> In v1 of what became commit 2cc7c2c737 (\"setup: refactor repo format\n> reading and verification\", 2016-03-11), this function actually had\n> return type \"void\", but that was changed in v2. Almost three years\n> later, no-one has used this return value.\n\nHmm. If we have to pick one, I'd say that just returning a sane exit\nvalue would be the more conventional thing to do. But looking at the\ncallers, many of them want to just pass the struct on to the verify\nfunction.\n\nThat said, there is a long-standing curiosity here that we may want to\nconsider: read_repository_format() does not distinguish between these\nthree cases:\n\n 1. the config file is missing\n\n 2. the config file is present, but does not have a version field\n\n 3. the config file is malformed, or we experience an I/O error\n (although I think there are still some cases that cause the config\n parser to die(), which may be sufficient)\n\nThe comment in check_repository_format_gently() implies that git-init\nneeds (1) to be a non-error for historical reasons. We could probably\ntighten this up for other callers.\n\nI think (2) probably should be an error, but note that it makes t1300\nvery unhappy, since it stomps all over .git/config. I'm not sure if any\nreal-world cases would be affected.\n\nCase (3) I think we probably ought to do a better job of diagnosing. So\nI wonder if the rule should be:\n\n - if we encounter a real error reading the config,\n read_repository_format() should return -1. Most callers should\n detect this and complain.\n\n - otherwise, a missing config returns 0 but puts \"-1\" into the version\n field\n\n - possibly verify_repository_format() should issue a warning when it\n sees \"version == -1\" and then rewrite the result into \"0\"\n\nI dunno. This is one of those dark corners of the code where we appear\nto do the wrong thing, but nobody seems to have noticed or cared much,\nand changing it runs the risk of breaking some obscure cases. I'm not\nsure if we should bite the bullet and try to address that, or just back\naway slowly and pretend we never looked at it. ;)\n\n> Signed-off-by: Martin Ågren \n> ---\n> I only discovered the full history after writing the patch. Had I known\n> it from the beginning, maybe I'd have just skipped this step, but I was\n> sufficiently disturbed by the redundant and unused return value that I\n> dropped it before working on the actual meat of this series.\n> \n> cache.h | 7 +++----\n> setup.c | 3 +--\n> 2 files changed, 4 insertions(+), 6 deletions(-)\n\nFWIW, the patch itself seems fine, and obviously doesn't make anything\nworse on its own. The question is just whether we want to do more\ncleanup here.\n\n-Peff\n"},{"id":"365578","messageId":"20181219153841.GB14802@sigill.intra.peff.net","threadId":"50054","inReplyTo":"20181218072528.3870492-3-martin.agren@gmail.com","subject":"Re: [PATCH 2/3] setup: do not use invalid `repository_format`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-19T15:38:41Z","receivedAt":"2018-12-19T15:38:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 18, 2018 at 08:25:27AM +0100, Martin Ågren wrote:\n\n> If `read_repository_format()` encounters an error, `format->version`\n> will be -1 and all other fields of `format` will be undefined. However,\n> in `setup_git_directory_gently()`, we use `repo_fmt.hash_algo`\n> regardless of the value of `repo_fmt.version`.\n> \n> This can be observed by adding this to the end of\n> `read_repository_format()`:\n> \n> \tif (format->version == -1)\n> \t\tformat->hash_algo = 0; /* no-one should peek at this! */\n> \n> This causes, e.g., \"git branch -m q q2 without config should succeed\" in\n> t3200 to fail with \"fatal: Failed to resolve HEAD as a valid ref.\"\n> because it has moved .git/config out of the way and is now trying to use\n> a bad hash algorithm.\n> \n> Check that `version` is non-negative before using `hash_algo`.\n> \n> This patch adds no tests, but do note that if we skip this patch, the\n> next patch would cause existing tests to fail as outlined above.\n> \n> Signed-off-by: Martin Ågren \n\nHmm. It looks like we never set repo_fmt.hash_algo to anything besides\nGIT_HASH_SHA1 anyway. I guess the existing field is really just there in\npreparation for us eventually respecting extensions.hashAlgorithm (or\nwhatever it's called).\n\nGiven what I said in my previous email about repos with a missing\n\"version\" field, I wondered if this patch would be breaking config like:\n\n [core]\n # no repositoryformatversion!\n [extensions]\n hashAlgorithm = sha256\n\nBut I'd argue that:\n\n 1. That's pretty dumb config that we shouldn't need to support. Even\n if we care about handling the missing version for historical repos,\n they wouldn't be talking sha256.\n\n 2. Arguably we should not even look at extensions.* unless we see a\n version >= 1. But we do process them as we parse the config file.\n This is mostly an oversight, I think. We have to handle them as we\n see them, because they may come out of order with respect to the\n repositoryformatversion field. But we could put them into a\n string_list, and then only process them after we've decided which\n version we have.\n\nSo I think your patch is doing the right thing, and won't hurt any real\ncases. But (of course) there are more opportunities to clean things up.\n\n-Peff\n"},{"id":"365579","messageId":"20181219154853.GC14802@sigill.intra.peff.net","threadId":"50054","inReplyTo":"20181218072528.3870492-4-martin.agren@gmail.com","subject":"Re: [PATCH 3/3] setup: add `clear_repository_format()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-19T15:48:53Z","receivedAt":"2018-12-19T15:48:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 18, 2018 at 08:25:28AM +0100, Martin Ågren wrote:\n\n> After we set up a `struct repository_format`, it owns various pieces of\n> allocated memory. We then either use those members, because we decide we\n> want to use the \"candidate\" repository format, or we discard the\n> candidate / scratch space. In the first case, we transfer ownership of\n> the memory to a few global variables. In the latter case, we just\n> silently drop the struct and end up leaking memory.\n> \n> Introduce a function `clear_repository_format()` which frees the memory\n> the struct holds on to. Call it in the code paths where we currently\n> leak the memory. Also call it in the error path of\n> `read_repository_format()` to clean up any partial result.\n> \n> For hygiene, we need to at least set the pointers that we free to NULL.\n> For future-proofing, let's zero the entire struct instead. It just means\n> that in the error path of `read_...()` we need to restore the error\n> sentinel in the `version` field.\n\nThis seems reasonable, and I very much agree on the zero-ing (even\nthough it _shouldn't_ matter due to the \"undefined\" rule). That also\nmakes it safe to clear() multiple times, which is a nice property.\n\n> +void clear_repository_format(struct repository_format *format)\n> +{\n> +\tstring_list_clear(&format->unknown_extensions, 0);\n> +\tfree(format->work_tree);\n> +\tfree(format->partial_clone);\n> +\tmemset(format, 0, sizeof(*format));\n> }\n\nFor the callers that actually pick the values out, I think it might be a\nlittle less error-prone if they actually copied the strings and then\ncalled clear_repository_format(). That avoids leaks of values that they\ndidn't know or care about (and the cost of an extra strdup for\nrepository setup is not a big deal).\n\nSomething like this on top of your patch, I guess (with the idea being\nthat functions which return an error would clear the format, but a\n\"successful\" one would get returned back up the stack to\nsetup_git_directory_gently(), which then clears it before returning.\n\n-- >8 --\ndiff --git a/setup.c b/setup.c\nindex babe5ea156..a5699f9ee6 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -470,6 +470,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\t\twarning(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t\t*nongit_ok = -1;\n+\t\t\tclear_repository_format(candidate);\n \t\t\treturn -1;\n \t\t}\n \t\tdie(\"%s\", err.buf);\n@@ -499,7 +500,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\t}\n \t\tif (candidate->work_tree) {\n \t\t\tfree(git_work_tree_cfg);\n-\t\t\tgit_work_tree_cfg = candidate->work_tree;\n+\t\t\tgit_work_tree_cfg = xstrdup(candidate->work_tree);\n \t\t\tinside_work_tree = -1;\n \t\t}\n \t} else {\n@@ -1158,6 +1159,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \n \tstrbuf_release(&dir);\n \tstrbuf_release(&gitdir);\n+\tclear_repository_format(&repo_fmt);\n \n \treturn prefix;\n }\n\n-Peff\n"},{"id":"365596","messageId":"CAN0heSpfksXsKZrFNSaO_qitAB6gbF7QePQkuN37kAerSe2xWA@mail.gmail.com","threadId":"50054","inReplyTo":"20181219152735.GA14802@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] setup: drop return value from `read_repository_format()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-12-19T21:42:12Z","receivedAt":"2018-12-19T21:42:27Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 19 Dec 2018 at 16:27, Jeff King wrote:\n>\n> On Tue, Dec 18, 2018 at 08:25:26AM +0100, Martin Ågren wrote:\n>\n> > No-one looks at the return value, so we might as well drop it. It's\n> > still available as `format->version`.\n>\n> Hmm. If we have to pick one, I'd say that just returning a sane exit\n> value would be the more conventional thing to do. But looking at the\n> callers, many of them want to just pass the struct on to the verify\n> function.\n>\n> That said, there is a long-standing curiosity here that we may want to\n> consider: read_repository_format() does not distinguish between these\n> three cases:\n\n[snip several valuable insights]\n\n> I dunno. This is one of those dark corners of the code where we appear\n> to do the wrong thing, but nobody seems to have noticed or cared much,\n> and changing it runs the risk of breaking some obscure cases. I'm not\n> sure if we should bite the bullet and try to address that, or just back\n> away slowly and pretend we never looked at it. ;)\n\nThat was my reaction when I first dug into this. :-/ It's only now that\nI've been brave enough to even try to dig through the first layer.\n\n> FWIW, the patch itself seems fine, and obviously doesn't make anything\n> worse on its own. The question is just whether we want to do more\n> cleanup here.\n\nWell, if we do want to make more cleanups around here, one of the more\nobvious ideas is to actually use the return value. If such a cleanup is\ngoing to start with a (moral) revert of this patch, then we're probably\nbetter off just dropping this patch.\n\nThanks for your thoughtful response\n\n\nMartin\n"},{"id":"365597","messageId":"CAN0heSoORpNWSVmMiG0MF3f07FDtV5NfUhSuO1o1iDn7Mos-NQ@mail.gmail.com","threadId":"50054","inReplyTo":"20181219001801.GA520988@genre.crustytoothpaste.net","subject":"Re: [PATCH 2/3] setup: do not use invalid `repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-12-19T21:43:21Z","receivedAt":"2018-12-19T21:43:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 19 Dec 2018 at 01:18, brian m. carlson\n wrote:\n> I think this change is fine, because we initialize the value in\n> the_repository elsewhere, and if there's no repository, this should\n> never have a value other than the default anyway.\n\nThanks, it feels good that this patch matches how you think about the\n`hash_algo` field.\n\n> I looked at the other patches in the series and thought they looked sane\n> as well.\n\nThanks for a review, I appreciate it.\n\n\nMartin\n"},{"id":"365599","messageId":"CAN0heSpc_sap1cZktteRn3hyeuRx2w86Hd2kqxu4XFgHs75_Kw@mail.gmail.com","threadId":"50054","inReplyTo":"20181219153841.GB14802@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup: do not use invalid `repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-12-19T21:46:52Z","receivedAt":"2018-12-19T21:47:06Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 19 Dec 2018 at 16:38, Jeff King wrote:\n>\n> On Tue, Dec 18, 2018 at 08:25:27AM +0100, Martin Ågren wrote:\n>\n> > Check that `version` is non-negative before using `hash_algo`.\n\n> Hmm. It looks like we never set repo_fmt.hash_algo to anything besides\n> GIT_HASH_SHA1 anyway. I guess the existing field is really just there in\n> preparation for us eventually respecting extensions.hashAlgorithm (or\n> whatever it's called).\n\nThat was my understanding as well. Maybe I should have spelled it out.\n\nI think of the diff of this patch as \"let's check `foo->valid` before we\n`use(foo->bar)`\", which should only be able to regress in case foo isn't\nvalid. And ...\n\n> Given what I said in my previous email about repos with a missing\n> \"version\" field, I wondered if this patch would be breaking config like:\n>\n> [core]\n> # no repositoryformatversion!\n> [extensions]\n> hashAlgorithm = sha256\n>\n> But I'd argue that:\n>\n> 1. That's pretty dumb config that we shouldn't need to support. Even\n> if we care about handling the missing version for historical repos,\n> they wouldn't be talking sha256.\n\n... this matches my thinking.\n\n> 2. Arguably we should not even look at extensions.* unless we see a\n> version >= 1. But we do process them as we parse the config file.\n> This is mostly an oversight, I think. We have to handle them as we\n> see them, because they may come out of order with respect to the\n> repositoryformatversion field. But we could put them into a\n> string_list, and then only process them after we've decided which\n> version we have.\n\nI hadn't thought too much about this. I guess that for some simpler\nextensions--versions dependencies it would be feasible to first parse\neverything, then, depending on the version we've identified, forget\nabout any \"irrelevant\" extensions. Again, nothing I've thought much\nabout, and seems to be safely out of scope for this patch.\n\n\n> So I think your patch is doing the right thing, and won't hurt any real\n> cases. But (of course) there are more opportunities to clean things up.\n"},{"id":"365600","messageId":"CAN0heSr5KFK02XHk2rK39zEUoyoJCkqA=22qSkETehyKewM7GQ@mail.gmail.com","threadId":"50054","inReplyTo":"20181219154853.GC14802@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] setup: add `clear_repository_format()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-12-19T21:49:40Z","receivedAt":"2018-12-19T21:49:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 19 Dec 2018 at 16:48, Jeff King wrote:\n>\n> On Tue, Dec 18, 2018 at 08:25:28AM +0100, Martin Ågren wrote:\n>\n> > +void clear_repository_format(struct repository_format *format)\n> > +{\n> > + string_list_clear(&format->unknown_extensions, 0);\n> > + free(format->work_tree);\n> > + free(format->partial_clone);\n> > + memset(format, 0, sizeof(*format));\n> > }\n>\n> For the callers that actually pick the values out, I think it might be a\n> little less error-prone if they actually copied the strings and then\n> called clear_repository_format(). That avoids leaks of values that they\n> didn't know or care about (and the cost of an extra strdup for\n> repository setup is not a big deal).\n>\n> Something like this on top of your patch, I guess (with the idea being\n> that functions which return an error would clear the format, but a\n> \"successful\" one would get returned back up the stack to\n> setup_git_directory_gently(), which then clears it before returning.\n\nThanks for the suggestion. I'll ponder 1) how to go about this\nrobustifying, 2) how to present the result as part of a v2 series.\n\nTo Junio on the sidelines in a cast (hope you're feeling better!):\nyou can expect a v2 of this series.\n\n\nMartin\n"},{"id":"365605","messageId":"20181219231716.GA21283@sigill.intra.peff.net","threadId":"50054","inReplyTo":"CAN0heSpc_sap1cZktteRn3hyeuRx2w86Hd2kqxu4XFgHs75_Kw@mail.gmail.com","subject":"Re: [PATCH 2/3] setup: do not use invalid `repository_format`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-19T23:17:16Z","receivedAt":"2018-12-19T23:17:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 19, 2018 at 10:46:52PM +0100, Martin Ågren wrote:\n\n> > 2. Arguably we should not even look at extensions.* unless we see a\n> > version >= 1. But we do process them as we parse the config file.\n> > This is mostly an oversight, I think. We have to handle them as we\n> > see them, because they may come out of order with respect to the\n> > repositoryformatversion field. But we could put them into a\n> > string_list, and then only process them after we've decided which\n> > version we have.\n> \n> I hadn't thought too much about this. I guess that for some simpler\n> extensions--versions dependencies it would be feasible to first parse\n> everything, then, depending on the version we've identified, forget\n> about any \"irrelevant\" extensions. Again, nothing I've thought much\n> about, and seems to be safely out of scope for this patch.\n\nThe decision is actually pretty straight-forward: if version < 1, ignore\nextensions, otherwise respect them (and complain about any we don't know\nabout).\n\nSo I think we could just do in verify_repository_format() something\nlike:\n\n if (version < 1) {\n /* \"undo\" any extensions we might have parsed */\n data->precious_objects = 0;\n FREE_AND_NULL(data->partial_clone);\n data->worktree_config = 0;\n data->hash_algo = GIT_HASH_SHA1;\n } else {\n /* complain about unknown extension; we already do this! */\n }\n\nIt's a little ugly to have to know about all the extensions here, but we\nalready initialize them in read_repository_format(). We could probably\nfactor that out into a shared function.\n\n-Peff\n"},{"id":"365612","messageId":"20181220001752.GA35965@genre.crustytoothpaste.net","threadId":"50054","inReplyTo":"20181219152735.GA14802@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] setup: drop return value from `read_repository_format()`","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-12-20T00:17:53Z","receivedAt":"2018-12-20T00:18:32Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Dec 19, 2018 at 10:27:35AM -0500, Jeff King wrote:\n> I dunno. This is one of those dark corners of the code where we appear\n> to do the wrong thing, but nobody seems to have noticed or cared much,\n> and changing it runs the risk of breaking some obscure cases. I'm not\n> sure if we should bite the bullet and try to address that, or just back\n> away slowly and pretend we never looked at it. ;)\n\nI will point out that with the SHA-256 work, reading the config file\nbecomes essential for SHA-256 repositories, because we need to know the\nobject format. Removing the config file leads to things blowing up in a\nbad way (what specific bad way I don't remember).\n\nThat may influence the direction we want to take in this work, or not.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"365613","messageId":"20181220002137.GB35965@genre.crustytoothpaste.net","threadId":"50054","inReplyTo":"20181219153841.GB14802@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] setup: do not use invalid `repository_format`","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-12-20T00:21:38Z","receivedAt":"2018-12-20T00:21:45Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Dec 19, 2018 at 10:38:41AM -0500, Jeff King wrote:\n> Hmm. It looks like we never set repo_fmt.hash_algo to anything besides\n> GIT_HASH_SHA1 anyway. I guess the existing field is really just there in\n> preparation for us eventually respecting extensions.hashAlgorithm (or\n> whatever it's called).\n\nYeah, it is.\n\nI haven't tested, but since we just read the value of\nextensions.objectFormat, this patch shouldn't have any effect on the\nSHA-256 code. The default remains SHA-1 if a value isn't specified\nsomehow.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"365616","messageId":"20181220025211.GB24002@sigill.intra.peff.net","threadId":"50054","inReplyTo":"20181220001752.GA35965@genre.crustytoothpaste.net","subject":"Re: [PATCH 1/3] setup: drop return value from `read_repository_format()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-20T02:52:12Z","receivedAt":"2018-12-20T02:52:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 20, 2018 at 12:17:53AM +0000, brian m. carlson wrote:\n\n> On Wed, Dec 19, 2018 at 10:27:35AM -0500, Jeff King wrote:\n> > I dunno. This is one of those dark corners of the code where we appear\n> > to do the wrong thing, but nobody seems to have noticed or cared much,\n> > and changing it runs the risk of breaking some obscure cases. I'm not\n> > sure if we should bite the bullet and try to address that, or just back\n> > away slowly and pretend we never looked at it. ;)\n> \n> I will point out that with the SHA-256 work, reading the config file\n> becomes essential for SHA-256 repositories, because we need to know the\n> object format. Removing the config file leads to things blowing up in a\n> bad way (what specific bad way I don't remember).\n> \n> That may influence the direction we want to take in this work, or not.\n\nWouldn't we just treat that the same way we do now? I.e., assume the\ndefault of sha1, just like we assume repositoryformatversion==0?\n\n-Peff\n"},{"id":"365617","messageId":"20181220034555.GC35965@genre.crustytoothpaste.net","threadId":"50054","inReplyTo":"20181220025211.GB24002@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] setup: drop return value from `read_repository_format()`","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-12-20T03:45:55Z","receivedAt":"2018-12-20T03:46:11Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Dec 19, 2018 at 09:52:12PM -0500, Jeff King wrote:\n> On Thu, Dec 20, 2018 at 12:17:53AM +0000, brian m. carlson wrote:\n> \n> > On Wed, Dec 19, 2018 at 10:27:35AM -0500, Jeff King wrote:\n> > > I dunno. This is one of those dark corners of the code where we appear\n> > > to do the wrong thing, but nobody seems to have noticed or cared much,\n> > > and changing it runs the risk of breaking some obscure cases. I'm not\n> > > sure if we should bite the bullet and try to address that, or just back\n> > > away slowly and pretend we never looked at it. ;)\n> > \n> > I will point out that with the SHA-256 work, reading the config file\n> > becomes essential for SHA-256 repositories, because we need to know the\n> > object format. Removing the config file leads to things blowing up in a\n> > bad way (what specific bad way I don't remember).\n> > \n> > That may influence the direction we want to take in this work, or not.\n> \n> Wouldn't we just treat that the same way we do now? I.e., assume the\n> default of sha1, just like we assume repositoryformatversion==0?\n\nYeah, we'll default to SHA-1, but the repository will be broken. HEAD\ncan't be read. Trying to run git status dies with \"fatal: Unknown index\nentry format\". And so on. We've written data with 64-character object\nIDs, which can't be read by Git in SHA-1 mode.\n\nMy point is essentially that in an SHA-256 repository, the config file\nisn't optional anymore. We probably need to consider that and error out\nin more situations (e.g. unreadable file or I/O error) instead of\nsilently falling back to the defaults, since failing loudly in a visible\nway is better than having the user try to figure out why the index is\nsuddenly \"corrupt\".\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"365642","messageId":"20181220145322.GA27361@sigill.intra.peff.net","threadId":"50054","inReplyTo":"20181220034555.GC35965@genre.crustytoothpaste.net","subject":"Re: [PATCH 1/3] setup: drop return value from `read_repository_format()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-20T14:53:22Z","receivedAt":"2018-12-20T14:53:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 20, 2018 at 03:45:55AM +0000, brian m. carlson wrote:\n\n> > > I will point out that with the SHA-256 work, reading the config file\n> > > becomes essential for SHA-256 repositories, because we need to know the\n> > > object format. Removing the config file leads to things blowing up in a\n> > > bad way (what specific bad way I don't remember).\n> > > \n> > > That may influence the direction we want to take in this work, or not.\n> > \n> > Wouldn't we just treat that the same way we do now? I.e., assume the\n> > default of sha1, just like we assume repositoryformatversion==0?\n> \n> Yeah, we'll default to SHA-1, but the repository will be broken. HEAD\n> can't be read. Trying to run git status dies with \"fatal: Unknown index\n> entry format\". And so on. We've written data with 64-character object\n> IDs, which can't be read by Git in SHA-1 mode.\n\nOh, I see. Yes, if you have a SHA-256 repository and you don't tell\nanybody (via a config entry), then everything will fail to work. That\nseems like a perfectly reasonable outcome to me.\n\n> My point is essentially that in an SHA-256 repository, the config file\n> isn't optional anymore. We probably need to consider that and error out\n> in more situations (e.g. unreadable file or I/O error) instead of\n> silently falling back to the defaults, since failing loudly in a visible\n> way is better than having the user try to figure out why the index is\n> suddenly \"corrupt\".\n\nYes, I agree that ideally we'd produce a better error message. I'd just\nbe wary of breaking compatibility for the existing cases by making new\nrequirements when we don't yet suspect the repo is SHA-256.\n\nWhen we see such a corruption, would it be possible to poke at the data\nas if it were the old SHA-1 format, and if _that_ looks sane, suggest to\nthe user what the problem might be? That would help a number of cases\nbeyond this one (i.e., you're missing config, you have config but it has\nthe wrong repo format, you're missing the correct extensions field,\netc).\n\n-Peff\n"},{"id":"366666","messageId":"cover.1547488709.git.martin.agren@gmail.com","threadId":"50054","inReplyTo":"20181218072528.3870492-1-martin.agren@gmail.com","subject":"[PATCH v2 0/3] setup: add `clear_repository_format()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-14T18:34:54Z","receivedAt":"2019-01-14T18:35:07Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This patch series addresses memory leaks related to `struct\nrepository_format`. Compared to v1 [1], this v2 has dropped the first\npatch (\"setup: drop return value from `read_repository_format()`\") and\nadded another patch 1/3 (which solves a different problem).\n\nPatch 2/3 is unchanged. Patch 3/3 now calls `clear_...()` not only in\nthe error paths, but in all paths, as suggested in the review of v1.\n\nThanks to Peff and brian for reviewing v1.\n\nThere's a minor textual conflict with ed/simplify-setup-git-dir. A merge\nof these two topics still passes the test suite (and these leaks remain\nplugged).\n\n[1] https://public-inbox.org/git/20181218072528.3870492-1-martin.agren@gmail.com/#t\n\nMartin Ågren (3):\n setup: free old value before setting `work_tree`\n setup: do not use invalid `repository_format`\n setup: add `clear_repository_format()`\n\n cache.h | 12 ++++++++++++\n builtin/init-db.c | 3 ++-\n repository.c | 3 ++-\n setup.c | 33 ++++++++++++++++++++++++---------\n worktree.c | 4 +++-\n 5 files changed, 43 insertions(+), 12 deletions(-)\n\n-- \n2.20.1\n\n"},{"id":"366667","messageId":"8585c0bfdc071f17e75346b801a16580d360c18a.1547488709.git.martin.agren@gmail.com","threadId":"50054","inReplyTo":"cover.1547488709.git.martin.agren@gmail.com","subject":"[PATCH v2 1/3] setup: free old value before setting `work_tree`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-14T18:34:55Z","receivedAt":"2019-01-14T18:35:10Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Before assigning to `data->work_tree` in `read_worktree_config()`, free\nany value we might already have picked up, so that we do not leak it.\n\nSigned-off-by: Martin Ågren \n---\n setup.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/setup.c b/setup.c\nindex 1be5037f12..bb633942bb 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -411,6 +411,7 @@ static int read_worktree_config(const char *var, const char *value, void *vdata)\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}\n \treturn 0;\n-- \n2.20.1\n\n"},{"id":"366668","messageId":"a7e58385290e6789105d2d5b794f4bf8607285dd.1547488709.git.martin.agren@gmail.com","threadId":"50054","inReplyTo":"cover.1547488709.git.martin.agren@gmail.com","subject":"[PATCH v2 2/3] setup: do not use invalid `repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-14T18:34:56Z","receivedAt":"2019-01-14T18:35:12Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If `read_repository_format()` encounters an error, `format->version`\nwill be -1 and all other fields of `format` will be undefined. However,\nin `setup_git_directory_gently()`, we use `repo_fmt.hash_algo`\nregardless of the value of `repo_fmt.version`.\n\nThis can be observed by adding this to the end of\n`read_repository_format()`:\n\n\tif (format->version == -1)\n\t\tformat->hash_algo = 0; /* no-one should peek at this! */\n\nThis causes, e.g., \"git branch -m q q2 without config should succeed\" in\nt3200 to fail with \"fatal: Failed to resolve HEAD as a valid ref.\"\nbecause it has moved .git/config out of the way and is now trying to use\na bad hash algorithm.\n\nCheck that `version` is non-negative before using `hash_algo`.\n\nThis patch adds no tests, but do note that if we skip this patch, the\nnext patch would cause existing tests to fail as outlined above.\n\nSigned-off-by: Martin Ågren \n---\n setup.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex bb633942bb..4d3d67c50b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1140,7 +1140,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\t\tgitdir = DEFAULT_GIT_DIR_ENVIRONMENT;\n \t\t\tsetup_git_env(gitdir);\n \t\t}\n-\t\tif (startup_info->have_repository)\n+\t\tif (startup_info->have_repository && repo_fmt.version > -1)\n \t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n \t}\n \n-- \n2.20.1\n\n"},{"id":"366669","messageId":"7b0ca165bce684bde38b753ab98f7858f20130c0.1547488709.git.martin.agren@gmail.com","threadId":"50054","inReplyTo":"cover.1547488709.git.martin.agren@gmail.com","subject":"[PATCH v2 3/3] setup: add `clear_repository_format()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-14T18:34:57Z","receivedAt":"2019-01-14T18:35:15Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After we set up a `struct repository_format`, it owns various pieces of\nallocated memory. We then either use those members, because we decide we\nwant to use the \"candidate\" repository format, or we discard the\ncandidate / scratch space. In the first case, we transfer ownership of\nthe memory to a few global variables. In the latter case, we just\nsilently drop the struct and end up leaking memory.\n\nFixing these memory leaks is complicated by two issues:\n\n 1) As described above, we \"steal\" the fields of the struct in case of\n success.\n\n 2) We might end up calling `read_repository_format()` more than once\n -- as we enter it a second time, we lose track of our pointers and\n leak memory. As a further complication, we do not initialize (zero)\n the structs before calling `read_...()` so as we first enter it, we\n cannot blindly free the pointers in it.\n\nTo free this memory, in light of (1), we must either carefully cover all\nerror paths but no success paths; or we should stop grabbing the\npointers. To address 2), we must either zero the struct before calling\n`read_repository_format()`, or try to keep track of when we should zero\nit and when we should first free the memory.\n\nIntroduce `REPO_FORMAT_INIT` and `clear_repository_format()` to be used\non each side of `read_repository_format()`. Make the users duplicate the\nmemory from the structs, rather than copying the pointers.\n\nCall `clear_...()` at the start of `read_...()` instead of just zeroing\nthe struct. In the error path of the same function, be sure to restore\nthe error sentinel after we clear it with the rest of the struct.\n\nSigned-off-by: Martin Ågren \n---\n I do wonder if introducing and depending on `REPOSITORY_FORMAT_INIT`\n like this is 100% sane. Out-of-tree users could be in for a nasty\n surprise. :-/\n\n cache.h | 12 ++++++++++++\n builtin/init-db.c | 3 ++-\n repository.c | 3 ++-\n setup.c | 30 ++++++++++++++++++++++--------\n worktree.c | 4 +++-\n 5 files changed, 41 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex ca36b44ee0..3ef63d27c4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -972,6 +972,12 @@ struct repository_format {\n \tstruct string_list unknown_extensions;\n };\n \n+/**\n+ * Always use this to initialize a `struct repository_format`\n+ * to a well-defined state before calling `read_repository()`.\n+ */\n+#define REPOSITORY_FORMAT_INIT { 0 }\n+\n /*\n * Read the repository format characteristics from the config file \"path\" into\n * \"format\" struct. Returns the numeric version. On error, -1 is returned,\n@@ -980,6 +986,12 @@ struct repository_format {\n */\n int read_repository_format(struct repository_format *format, const char *path);\n \n+/*\n+ * Free the memory held onto by `format`, but not the struct itself.\n+ * (No need to use this after `read_repository_format()` fails.)\n+ */\n+void clear_repository_format(struct repository_format *format);\n+\n /*\n * Verify that the repository described by repository_format is something we\n * can read. If it is, return 0. Otherwise, return -1, and \"err\" will describe\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 41faffd28d..04c60eaad5 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -96,7 +96,7 @@ static void copy_templates(const char *template_dir)\n \tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf template_path = STRBUF_INIT;\n \tsize_t template_len;\n-\tstruct repository_format template_format;\n+\tstruct repository_format template_format = REPOSITORY_FORMAT_INIT;\n \tstruct strbuf err = STRBUF_INIT;\n \tDIR *dir;\n \tchar *to_free = NULL;\n@@ -148,6 +148,7 @@ static void copy_templates(const char *template_dir)\n \tfree(to_free);\n \tstrbuf_release(&path);\n \tstrbuf_release(&template_path);\n+\tclear_repository_format(&template_format);\n }\n \n static int git_init_db_config(const char *k, const char *v, void *cb)\ndiff --git a/repository.c b/repository.c\nindex 7b02e1dffa..df88705574 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -148,7 +148,7 @@ int repo_init(struct repository *repo,\n \t const char *gitdir,\n \t const char *worktree)\n {\n-\tstruct repository_format format;\n+\tstruct repository_format format = REPOSITORY_FORMAT_INIT;\n \tmemset(repo, 0, sizeof(*repo));\n \n \trepo->objects = raw_object_store_new();\n@@ -165,6 +165,7 @@ int repo_init(struct repository *repo,\n \tif (worktree)\n \t\trepo_set_worktree(repo, worktree);\n \n+\tclear_repository_format(&format);\n \treturn 0;\n \n error:\ndiff --git a/setup.c b/setup.c\nindex 4d3d67c50b..70d9007ae5 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -477,7 +477,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t}\n \n \trepository_format_precious_objects = candidate->precious_objects;\n-\trepository_format_partial_clone = candidate->partial_clone;\n+\trepository_format_partial_clone = xstrdup_or_null(candidate->partial_clone);\n \trepository_format_worktree_config = candidate->worktree_config;\n \tstring_list_clear(&candidate->unknown_extensions, 0);\n \n@@ -500,11 +500,9 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\t}\n \t\tif (candidate->work_tree) {\n \t\t\tfree(git_work_tree_cfg);\n-\t\t\tgit_work_tree_cfg = candidate->work_tree;\n+\t\t\tgit_work_tree_cfg = xstrdup(candidate->work_tree);\n \t\t\tinside_work_tree = -1;\n \t\t}\n-\t} else {\n-\t\tfree(candidate->work_tree);\n \t}\n \n \treturn 0;\n@@ -512,15 +510,27 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \n int read_repository_format(struct repository_format *format, const char *path)\n {\n-\tmemset(format, 0, sizeof(*format));\n+\tclear_repository_format(format);\n \tformat->version = -1;\n \tformat->is_bare = -1;\n \tformat->hash_algo = GIT_HASH_SHA1;\n \tstring_list_init(&format->unknown_extensions, 1);\n \tgit_config_from_file(check_repo_format, path, format);\n+\tif (format->version == -1) {\n+\t\tclear_repository_format(format);\n+\t\tformat->version = -1;\n+\t}\n \treturn format->version;\n }\n \n+void clear_repository_format(struct repository_format *format)\n+{\n+\tstring_list_clear(&format->unknown_extensions, 0);\n+\tfree(format->work_tree);\n+\tfree(format->partial_clone);\n+\tmemset(format, 0, sizeof(*format));\n+}\n+\n int verify_repository_format(const struct repository_format *format,\n \t\t\t struct strbuf *err)\n {\n@@ -1008,7 +1018,7 @@ int discover_git_directory(struct strbuf *commondir,\n \tstruct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;\n \tsize_t gitdir_offset = gitdir->len, cwd_len;\n \tsize_t commondir_offset = commondir->len;\n-\tstruct repository_format candidate;\n+\tstruct repository_format candidate = REPOSITORY_FORMAT_INIT;\n \n \tif (strbuf_getcwd(&dir))\n \t\treturn -1;\n@@ -1045,9 +1055,11 @@ int discover_git_directory(struct strbuf *commondir,\n \t\tstrbuf_release(&err);\n \t\tstrbuf_setlen(commondir, commondir_offset);\n \t\tstrbuf_setlen(gitdir, gitdir_offset);\n+\t\tclear_repository_format(&candidate);\n \t\treturn -1;\n \t}\n \n+\tclear_repository_format(&candidate);\n \treturn 0;\n }\n \n@@ -1056,7 +1068,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \tstatic struct strbuf cwd = STRBUF_INIT;\n \tstruct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;\n \tconst char *prefix;\n-\tstruct repository_format repo_fmt;\n+\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n \n \t/*\n \t * We may have read an incomplete configuration before\n@@ -1146,6 +1158,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \n \tstrbuf_release(&dir);\n \tstrbuf_release(&gitdir);\n+\tclear_repository_format(&repo_fmt);\n \n \treturn prefix;\n }\n@@ -1203,9 +1216,10 @@ int git_config_perm(const char *var, const char *value)\n \n void check_repository_format(void)\n {\n-\tstruct repository_format repo_fmt;\n+\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n \tcheck_repository_format_gently(get_git_dir(), &repo_fmt, NULL);\n \tstartup_info->have_repository = 1;\n+\tclear_repository_format(&repo_fmt);\n }\n \n /*\ndiff --git a/worktree.c b/worktree.c\nindex d6a0ee7f73..b45bfeb9d3 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -444,7 +444,7 @@ int submodule_uses_worktrees(const char *path)\n \tDIR *dir;\n \tstruct dirent *d;\n \tint ret = 0;\n-\tstruct repository_format format;\n+\tstruct repository_format format = REPOSITORY_FORMAT_INIT;\n \n \tsubmodule_gitdir = git_pathdup_submodule(path, \"%s\", \"\");\n \tif (!submodule_gitdir)\n@@ -462,8 +462,10 @@ int submodule_uses_worktrees(const char *path)\n \tread_repository_format(&format, sb.buf);\n \tif (format.version != 0) {\n \t\tstrbuf_release(&sb);\n+\t\tclear_repository_format(&format);\n \t\treturn 1;\n \t}\n+\tclear_repository_format(&format);\n \n \t/* Replace config by worktrees. */\n \tstrbuf_setlen(&sb, sb.len - strlen(\"config\"));\n-- \n2.20.1\n\n"},{"id":"366742","messageId":"20190115193112.GE4886@sigill.intra.peff.net","threadId":"50054","inReplyTo":"a7e58385290e6789105d2d5b794f4bf8607285dd.1547488709.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 2/3] setup: do not use invalid `repository_format`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-15T19:31:13Z","receivedAt":"2019-01-15T19:31:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 14, 2019 at 07:34:56PM +0100, Martin Ågren wrote:\n\n> If `read_repository_format()` encounters an error, `format->version`\n> will be -1 and all other fields of `format` will be undefined. However,\n> in `setup_git_directory_gently()`, we use `repo_fmt.hash_algo`\n> regardless of the value of `repo_fmt.version`.\n> \n> This can be observed by adding this to the end of\n> `read_repository_format()`:\n> \n> \tif (format->version == -1)\n> \t\tformat->hash_algo = 0; /* no-one should peek at this! */\n> \n> This causes, e.g., \"git branch -m q q2 without config should succeed\" in\n> t3200 to fail with \"fatal: Failed to resolve HEAD as a valid ref.\"\n> because it has moved .git/config out of the way and is now trying to use\n> a bad hash algorithm.\n> \n> Check that `version` is non-negative before using `hash_algo`.\n> \n> This patch adds no tests, but do note that if we skip this patch, the\n> next patch would cause existing tests to fail as outlined above.\n\nI'm still somewhat confused about how this breaks. If we move\n\".git/config\" out of the way, then we have no version indicator and\npresumably we should guess GIT_HASH_SHA1. Which is what's happening if\nwe fail to call repo_set_hash_algo(), no? In other words, wouldn't\nrepo_set_hash_algo() always be a noop in that case?\n\nI get why adding the code snippet above would cause that assumption to\nbreak, but I am just not sure why we would add that code snippet. ;)\n\nI also get why read_repository_format() doing this in patch 3 would be a\nproblem:\n\n + if (format->version == -1) {\n + clear_repository_format(format);\n + format->version = -1;\n + }\n\nbut doesn't that point out that clear_repository_format() should be\nsetting hash_algo to GIT_HASH_SHA1 as the default (and likewise \"bare =\n-1\", etc, that is done in that function)?\n\n-Peff\n"},{"id":"366901","messageId":"20190117063114.1901775-1-martin.agren@gmail.com","threadId":"50054","inReplyTo":"20190115193112.GE4886@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] setup: do not use invalid `repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-17T06:31:14Z","receivedAt":"2019-01-17T06:31:50Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 15 Jan 2019 at 20:31, Jeff King wrote:\n>\n> On Mon, Jan 14, 2019 at 07:34:56PM +0100, Martin Ågren wrote:\n>\n> > This can be observed by adding this to the end of\n> > `read_repository_format()`:\n> >\n> > if (format->version == -1)\n> > format->hash_algo = 0; /* no-one should peek at this! */\n> >\n> > Check that `version` is non-negative before using `hash_algo`.\n\n> I'm still somewhat confused about how this breaks. If we move\n> \".git/config\" out of the way, then we have no version indicator and\n> presumably we should guess GIT_HASH_SHA1. Which is what's happening if\n> we fail to call repo_set_hash_algo(), no? In other words, wouldn't\n> repo_set_hash_algo() always be a noop in that case?\n>\n> I get why adding the code snippet above would cause that assumption to\n> break, but I am just not sure why we would add that code snippet. ;)\n>\n> I also get why read_repository_format() doing this in patch 3 would be a\n> problem:\n>\n> + if (format->version == -1) {\n> + clear_repository_format(format);\n> + format->version = -1;\n> + }\n>\n> but doesn't that point out that clear_repository_format() should be\n> setting hash_algo to GIT_HASH_SHA1 as the default (and likewise \"bare =\n> -1\", etc, that is done in that function)?\n\nSomething like the below on top of this series (then rebased). (The last\nhunk below is a revert of this patch.)\n\nI'd like to think of the situation before this patch above as a\nsituation where the API promises something and the user uses the API\nbeyond that. The next patch in this series changes the internals of the\nAPI in a way that is consistent with the promise made, but which ends up\naffecting an over-eager user.\n\nWhat this patch above does is to make the user do what the API promise\nallows them to do, i.e., no more shortcuts. What you're saying is, why\nisn't the promise stronger? So the user won't have to think as much?\n\nSo in particular, why doesn't `clear...()` and the error path in\n`read_...()` impose sane, usable defaults? My first concern is that it\nmeans we need to make a stronger promise, which might then be hard to\nback away from, if we want to. Maybe we'll never want to...\n\nMy second concern is, what should we be falling back to, going forward?\nAt some point, the hash indicated by `REPOSITORY_FORMAT_INIT` will be\nSHA-256. Before that, and as soon as we support both hashes, what if we\npick up SHA-256 before stumbling on some other piece of the config --\nshould we now reset the struct to indicate SHA-1, or rather keep the\nSHA-256 value, which by itself is valid? (The same could be argued now,\nfor something other than hash functions, but the SHA-1/256 example might\nbe more obvious in the context of this patch.)\n\nMy third worry is that we should then equip `clear_...()` or at least\nthe error path of `read_...()` with some logic to keep \"as much as\npossible\" of what we've picked up and reset the rest, all the while\nmaking sure we don't end up with something self-contradicting or stupid.\nAfter all, we'll have promised the users that they can ignore any errors\nand just run ahead.\n\nMaybe I'm worrying way too much, and I shouldn't be so afraid of making\na stronger promise here and now because of vague slippery-slope thinking.\n\nThanks for pushing back and forcing me to articulate my thinking.\n\nMartin\n\n\ndiff --git a/cache.h b/cache.h\nindex 3ef63d27c4..acd86e9f9f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -974,15 +974,21 @@ struct repository_format {\n \n /**\n * Always use this to initialize a `struct repository_format`\n- * to a well-defined state before calling `read_repository()`.\n+ * to a well-defined, default state before calling\n+ * `read_repository()`.\n */\n-#define REPOSITORY_FORMAT_INIT { 0 }\n+#define REPOSITORY_FORMAT_INIT (struct repository_format){ \\\n+\t\t\t\t .version = -1, \\\n+\t\t\t\t .is_bare = -1, \\\n+\t\t\t\t .hash_algo = GIT_HASH_SHA1, \\\n+\t\t\t\t .unknown_extensions = STRING_LIST_INIT_DUP, \\\n+\t\t\t }\n \n /*\n * Read the repository format characteristics from the config file \"path\" into\n * \"format\" struct. Returns the numeric version. On error, -1 is returned,\n * format->version is set to -1, and all other fields in the struct are\n- * undefined.\n+ * set to the default configuration (REPOSITORY_FORMAT_INIT).\n */\n int read_repository_format(struct repository_format *format, const char *path);\n \ndiff --git a/setup.c b/setup.c\nindex 70d9007ae5..f3ea479ad9 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -511,15 +511,9 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n int read_repository_format(struct repository_format *format, const char *path)\n {\n \tclear_repository_format(format);\n-\tformat->version = -1;\n-\tformat->is_bare = -1;\n-\tformat->hash_algo = GIT_HASH_SHA1;\n-\tstring_list_init(&format->unknown_extensions, 1);\n \tgit_config_from_file(check_repo_format, path, format);\n-\tif (format->version == -1) {\n+\tif (format->version == -1)\n \t\tclear_repository_format(format);\n-\t\tformat->version = -1;\n-\t}\n \treturn format->version;\n }\n \n@@ -528,7 +522,7 @@ void clear_repository_format(struct repository_format *format)\n \tstring_list_clear(&format->unknown_extensions, 0);\n \tfree(format->work_tree);\n \tfree(format->partial_clone);\n-\tmemset(format, 0, sizeof(*format));\n+\t*format = REPOSITORY_FORMAT_INIT;\n }\n \n int verify_repository_format(const struct repository_format *format,\n@@ -1152,7 +1146,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\t\tgitdir = DEFAULT_GIT_DIR_ENVIRONMENT;\n \t\t\tsetup_git_env(gitdir);\n \t\t}\n-\t\tif (startup_info->have_repository && repo_fmt.version > -1)\n+\t\tif (startup_info->have_repository)\n \t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n \t}\n \n"},{"id":"367282","messageId":"20190122070725.GA28555@sigill.intra.peff.net","threadId":"50054","inReplyTo":"20190117063114.1901775-1-martin.agren@gmail.com","subject":"Re: [PATCH v2 2/3] setup: do not use invalid `repository_format`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-22T07:07:25Z","receivedAt":"2019-01-22T07:07:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 17, 2019 at 07:31:14AM +0100, Martin Ågren wrote:\n\n> > I also get why read_repository_format() doing this in patch 3 would be a\n> > problem:\n> >\n> > + if (format->version == -1) {\n> > + clear_repository_format(format);\n> > + format->version = -1;\n> > + }\n> >\n> > but doesn't that point out that clear_repository_format() should be\n> > setting hash_algo to GIT_HASH_SHA1 as the default (and likewise \"bare =\n> > -1\", etc, that is done in that function)?\n> \n> Something like the below on top of this series (then rebased). (The last\n> hunk below is a revert of this patch.)\n\nYes, that's exactly what I had in mind. Usually our clear() functions\nput the struct back into some default state from which it can be used\ngain. But the state after clear() here (without the patch below) is\nsomething that nobody is ever expected to look at.\n\nGranted, the only function which fills it in is read_...(), and it sets\nthose defaults itself. But it just seems to me if we're going to have to\nput _something_ in the struct to initialize or clear it, it might as\nwell be those.\n\n> I'd like to think of the situation before this patch above as a\n> situation where the API promises something and the user uses the API\n> beyond that. The next patch in this series changes the internals of the\n> API in a way that is consistent with the promise made, but which ends up\n> affecting an over-eager user.\n\nAs with many parts of Git, there really isn't a clear promise. :) I\ndon't think you're wrong at all about the current state of things. I'm\nmostly basing my comments on \"what would I _expect_ the promise to be\nbased on our general patterns\". If that's far from what we promise now,\nthen it's a hassle to convert. But I think it's actually pretty close.\n\n> What this patch above does is to make the user do what the API promise\n> allows them to do, i.e., no more shortcuts. What you're saying is, why\n> isn't the promise stronger? So the user won't have to think as much?\n> \n> So in particular, why doesn't `clear...()` and the error path in\n> `read_...()` impose sane, usable defaults? My first concern is that it\n> means we need to make a stronger promise, which might then be hard to\n> back away from, if we want to. Maybe we'll never want to...\n\nI'm not too worried about that personally. I think the more likely\nproblem is that the API is misunderstood and misused. ;)\n\n> My second concern is, what should we be falling back to, going forward?\n> At some point, the hash indicated by `REPOSITORY_FORMAT_INIT` will be\n> SHA-256. Before that, and as soon as we support both hashes, what if we\n> pick up SHA-256 before stumbling on some other piece of the config --\n> should we now reset the struct to indicate SHA-1, or rather keep the\n> SHA-256 value, which by itself is valid? (The same could be argued now,\n> for something other than hash functions, but the SHA-1/256 example might\n> be more obvious in the context of this patch.)\n\nI'd think this would _always_ be sha-1. Because it's not about \"what's\nthe default for this program running\". It's about \"what have I read from\nthis on-disk repo config\". And the rule there is \"if they don't say\notherwise, it is sha1\". That won't change even in a sha256 world,\nbecause we'll maintain backwards-compatibility with legacy repositories\nforever.\n\nNow if your next question is: \"does any caller misuse this as more than\nlooking at the repo format\", I don't know the answer for sure. That\nwould be worth poking at (or perhaps having just poked yourself, you\nmight have an idea already).\n\n> My third worry is that we should then equip `clear_...()` or at least\n> the error path of `read_...()` with some logic to keep \"as much as\n> possible\" of what we've picked up and reset the rest, all the while\n> making sure we don't end up with something self-contradicting or stupid.\n> After all, we'll have promised the users that they can ignore any errors\n> and just run ahead.\n\nI think clear() should always throw everything away. Saving partial bits\nfrom the error path of read() is harder. My gut says \"no\", but I agree\nthat's a trickier question. I think the real-world thing here is: we're\nreading repo config and see an extensions.* field that says \"use\nsha256\". But then we encounter an error, or don't otherwise have a\nversion. What do we do?\n\nIf that's an undefined setup (and I think it is -- if you're using\nextensions.* you're supposed to always set the version field), then I\ndon't know that it really matters that much. But throwing the whole\nthing away (even if it means a buggy code path is more likely to use\nsha1) seems OK to me.\n\n> Maybe I'm worrying way too much, and I shouldn't be so afraid of making\n> a stronger promise here and now because of vague slippery-slope thinking.\n> \n> Thanks for pushing back and forcing me to articulate my thinking.\n\nFor the record, I can live with it either way. There are so many funky\nlittle setup corner cases in the code already, and we don't even really\nhave a real-world case to dissect at this point. So the right thing may\nalso just be to finish this patch series as quickly as possible and move\non to something more useful. :)\n\n-Peff\n"},{"id":"367301","messageId":"CAN0heSq0Nb-WdhDFpdwgjUMrkJNbviAtietn=B5nJg-rDgcR_g@mail.gmail.com","threadId":"50054","inReplyTo":"20190122070725.GA28555@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] setup: do not use invalid `repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-22T13:34:12Z","receivedAt":"2019-01-22T13:34:29Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 22 Jan 2019 at 08:07, Jeff King wrote:\n>\n> On Thu, Jan 17, 2019 at 07:31:14AM +0100, Martin Ågren wrote:\n>\n> > Something like the below on top of this series (then rebased). (The last\n> > hunk below is a revert of this patch.)\n>\n> Yes, that's exactly what I had in mind. Usually our clear() functions\n> put the struct back into some default state from which it can be used\n> gain. But the state after clear() here (without the patch below) is\n> something that nobody is ever expected to look at.\n\n> > So in particular, why doesn't `clear...()` and the error path in\n> > `read_...()` impose sane, usable defaults? My first concern is that it\n> > means we need to make a stronger promise, which might then be hard to\n> > back away from, if we want to. Maybe we'll never want to...\n>\n> I'm not too worried about that personally. I think the more likely\n> problem is that the API is misunderstood and misused. ;)\n\nHeh. Agreed. :-)\n\n> Now if your next question is: \"does any caller misuse this as more than\n> looking at the repo format\", I don't know the answer for sure. That\n> would be worth poking at (or perhaps having just poked yourself, you\n> might have an idea already).\n\nNot really. I've stumbled around a little, but I'll need to do that some\nmore.\n\n> For the record, I can live with it either way. There are so many funky\n> little setup corner cases in the code already, and we don't even really\n> have a real-world case to dissect at this point. So the right thing may\n> also just be to finish this patch series as quickly as possible and move\n> on to something more useful. :)\n\nI rebased the \"something like this?\" into this series yesterday and I\nthink the end result is better, but also that the way there is clearer,\nmostly because this patch is then gone. I wanted to double-check it\ntonight and submit it. I'll do that tonight.\n\nThank you for your comments. They're really helpful.\n\n\nMartin\n"},{"id":"367380","messageId":"cover.1548186510.git.martin.agren@gmail.com","threadId":"50054","inReplyTo":"CAN0heSq0Nb-WdhDFpdwgjUMrkJNbviAtietn=B5nJg-rDgcR_g@mail.gmail.com","subject":"[PATCH v3 0/2] setup: fix memory leaks with `struct repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-22T21:45:46Z","receivedAt":"2019-01-22T21:46:05Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 22 Jan 2019 at 14:34, Martin Ågren wrote:\n>\n> On Tue, 22 Jan 2019 at 08:07, Jeff King wrote:\n>\n> > For the record, I can live with it either way. There are so many funky\n> > little setup corner cases in the code already, and we don't even really\n> > have a real-world case to dissect at this point. So the right thing may\n> > also just be to finish this patch series as quickly as possible and move\n> > on to something more useful. :)\n>\n> I rebased the \"something like this?\" into this series yesterday and I\n> think the end result is better, but also that the way there is clearer,\n> mostly because this patch is then gone. I wanted to double-check it\n> tonight and submit it. I'll do that tonight.\n\nHere's that reroll. I now reset the entire struct in the error path of\n`clear_...()`. Thus, the user that is reading `repo_fmt.hash_algo`\ndespite not being supposed to, can keep reading it, now knowing that the\nvalue has a default value.\n\nI also expanded on the documentation a little to point out that we'll\nreset to the default struct state if we don't find any\n\"core.repositoryformatversion\".\n\nMartin\n\nMartin Ågren (2):\n setup: free old value before setting `work_tree`\n setup: fix memory leaks with `struct repository_format`\n\n cache.h | 27 ++++++++++++++++++++++++---\n builtin/init-db.c | 3 ++-\n repository.c | 3 ++-\n setup.c | 33 +++++++++++++++++++++------------\n worktree.c | 4 +++-\n 5 files changed, 52 insertions(+), 18 deletions(-)\n\n-- \n2.20.1.98.gecbdaf0899\n\n"},{"id":"367381","messageId":"5be3e4fb47f534e7b1467c1d844996bcd1121ca0.1548186510.git.martin.agren@gmail.com","threadId":"50054","inReplyTo":"cover.1548186510.git.martin.agren@gmail.com","subject":"[PATCH v3 1/2] setup: free old value before setting `work_tree`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-22T21:45:47Z","receivedAt":"2019-01-22T21:46:08Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Before assigning to `data->work_tree` in `read_worktree_config()`, free\nany value we might already have picked up, so that we do not leak it.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\n setup.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/setup.c b/setup.c\nindex 1be5037f12..bb633942bb 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -411,6 +411,7 @@ static int read_worktree_config(const char *var, const char *value, void *vdata)\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}\n \treturn 0;\n-- \n2.20.1.98.gecbdaf0899\n\n"},{"id":"367382","messageId":"f8b021033b887923662eb9fa63f6df1677ebbbb5.1548186510.git.martin.agren@gmail.com","threadId":"50054","inReplyTo":"cover.1548186510.git.martin.agren@gmail.com","subject":"[PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-22T21:45:48Z","receivedAt":"2019-01-22T21:46:12Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After we set up a `struct repository_format`, it owns various pieces of\nallocated memory. We then either use those members, because we decide we\nwant to use the \"candidate\" repository format, or we discard the\ncandidate / scratch space. In the first case, we transfer ownership of\nthe memory to a few global variables. In the latter case, we just\nsilently drop the struct and end up leaking memory.\n\nIntroduce an initialization macro `REPOSITORY_FORMAT_INIT` and a\nfunction `clear_repository_format()`, to be used on each side of\n`read_repository_format()`. To have a clear and simple memory ownership,\nlet all users of `struct repository_format` duplicate the strings that\nthey take from it, rather than stealing the pointers.\n\nCall `clear_...()` at the start of `read_...()` instead of just zeroing\nthe struct, since we sometimes enter the function multiple times. This\nmeans that it is important to initialize the struct before calling\n`read_...()`, so document that. Teach `read_...()` to clear the struct\nupon an error, so that it is reset to a safe state, and document this.\n\nAbout that very last point: In `setup_git_directory_gently()`, we look\nat `repo_fmt.hash_algo` even if `repo_fmt.version` is -1, which we\nweren't actually supposed to do per the API. After this commit, that's\nok.\n\nWe inherit the existing code's combining \"error\" and \"no version found\".\nBoth are signalled through `version == -1` and now both cause us to\nclear any partial configuration we have picked up. For \"extensions.*\",\nthat's fine, since they require a positive version number. For\n\"core.bare\" and \"core.worktree\", we're already verifying that we have a\nnon-negative version number before using them.\n\nSigned-off-by: Martin Ågren \n---\n cache.h | 27 ++++++++++++++++++++++++---\n builtin/init-db.c | 3 ++-\n repository.c | 3 ++-\n setup.c | 32 ++++++++++++++++++++------------\n worktree.c | 4 +++-\n 5 files changed, 51 insertions(+), 18 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex ca36b44ee0..ec7c043412 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -972,14 +972,35 @@ struct repository_format {\n \tstruct string_list unknown_extensions;\n };\n \n+/*\n+ * Always use this to initialize a `struct repository_format`\n+ * to a well-defined, default state before calling\n+ * `read_repository()`.\n+ */\n+#define REPOSITORY_FORMAT_INIT \\\n+{ \\\n+\t.version = -1, \\\n+\t.is_bare = -1, \\\n+\t.hash_algo = GIT_HASH_SHA1, \\\n+\t.unknown_extensions = STRING_LIST_INIT_DUP, \\\n+}\n+\n /*\n * Read the repository format characteristics from the config file \"path\" into\n- * \"format\" struct. Returns the numeric version. On error, -1 is returned,\n- * format->version is set to -1, and all other fields in the struct are\n- * undefined.\n+ * \"format\" struct. Returns the numeric version. On error, or if no version is\n+ * found in the configuration, -1 is returned, format->version is set to -1,\n+ * and all other fields in the struct are set to the default configuration\n+ * (REPOSITORY_FORMAT_INIT). Always initialize the struct using\n+ * REPOSITORY_FORMAT_INIT before calling this function.\n */\n int read_repository_format(struct repository_format *format, const char *path);\n \n+/*\n+ * Free the memory held onto by `format`, but not the struct itself.\n+ * (No need to use this after `read_repository_format()` fails.)\n+ */\n+void clear_repository_format(struct repository_format *format);\n+\n /*\n * Verify that the repository described by repository_format is something we\n * can read. If it is, return 0. Otherwise, return -1, and \"err\" will describe\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 41faffd28d..04c60eaad5 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -96,7 +96,7 @@ static void copy_templates(const char *template_dir)\n \tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf template_path = STRBUF_INIT;\n \tsize_t template_len;\n-\tstruct repository_format template_format;\n+\tstruct repository_format template_format = REPOSITORY_FORMAT_INIT;\n \tstruct strbuf err = STRBUF_INIT;\n \tDIR *dir;\n \tchar *to_free = NULL;\n@@ -148,6 +148,7 @@ static void copy_templates(const char *template_dir)\n \tfree(to_free);\n \tstrbuf_release(&path);\n \tstrbuf_release(&template_path);\n+\tclear_repository_format(&template_format);\n }\n \n static int git_init_db_config(const char *k, const char *v, void *cb)\ndiff --git a/repository.c b/repository.c\nindex 7b02e1dffa..df88705574 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -148,7 +148,7 @@ int repo_init(struct repository *repo,\n \t const char *gitdir,\n \t const char *worktree)\n {\n-\tstruct repository_format format;\n+\tstruct repository_format format = REPOSITORY_FORMAT_INIT;\n \tmemset(repo, 0, sizeof(*repo));\n \n \trepo->objects = raw_object_store_new();\n@@ -165,6 +165,7 @@ int repo_init(struct repository *repo,\n \tif (worktree)\n \t\trepo_set_worktree(repo, worktree);\n \n+\tclear_repository_format(&format);\n \treturn 0;\n \n error:\ndiff --git a/setup.c b/setup.c\nindex bb633942bb..14c2f8ce24 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -477,7 +477,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t}\n \n \trepository_format_precious_objects = candidate->precious_objects;\n-\trepository_format_partial_clone = candidate->partial_clone;\n+\trepository_format_partial_clone = xstrdup_or_null(candidate->partial_clone);\n \trepository_format_worktree_config = candidate->worktree_config;\n \tstring_list_clear(&candidate->unknown_extensions, 0);\n \n@@ -500,11 +500,9 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\t}\n \t\tif (candidate->work_tree) {\n \t\t\tfree(git_work_tree_cfg);\n-\t\t\tgit_work_tree_cfg = candidate->work_tree;\n+\t\t\tgit_work_tree_cfg = xstrdup(candidate->work_tree);\n \t\t\tinside_work_tree = -1;\n \t\t}\n-\t} else {\n-\t\tfree(candidate->work_tree);\n \t}\n \n \treturn 0;\n@@ -512,15 +510,21 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \n int read_repository_format(struct repository_format *format, const char *path)\n {\n-\tmemset(format, 0, sizeof(*format));\n-\tformat->version = -1;\n-\tformat->is_bare = -1;\n-\tformat->hash_algo = GIT_HASH_SHA1;\n-\tstring_list_init(&format->unknown_extensions, 1);\n+\tclear_repository_format(format);\n \tgit_config_from_file(check_repo_format, path, format);\n+\tif (format->version == -1)\n+\t\tclear_repository_format(format);\n \treturn format->version;\n }\n \n+void clear_repository_format(struct repository_format *format)\n+{\n+\tstring_list_clear(&format->unknown_extensions, 0);\n+\tfree(format->work_tree);\n+\tfree(format->partial_clone);\n+\t*format = (struct repository_format)REPOSITORY_FORMAT_INIT;\n+}\n+\n int verify_repository_format(const struct repository_format *format,\n \t\t\t struct strbuf *err)\n {\n@@ -1008,7 +1012,7 @@ int discover_git_directory(struct strbuf *commondir,\n \tstruct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;\n \tsize_t gitdir_offset = gitdir->len, cwd_len;\n \tsize_t commondir_offset = commondir->len;\n-\tstruct repository_format candidate;\n+\tstruct repository_format candidate = REPOSITORY_FORMAT_INIT;\n \n \tif (strbuf_getcwd(&dir))\n \t\treturn -1;\n@@ -1045,9 +1049,11 @@ int discover_git_directory(struct strbuf *commondir,\n \t\tstrbuf_release(&err);\n \t\tstrbuf_setlen(commondir, commondir_offset);\n \t\tstrbuf_setlen(gitdir, gitdir_offset);\n+\t\tclear_repository_format(&candidate);\n \t\treturn -1;\n \t}\n \n+\tclear_repository_format(&candidate);\n \treturn 0;\n }\n \n@@ -1056,7 +1062,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \tstatic struct strbuf cwd = STRBUF_INIT;\n \tstruct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;\n \tconst char *prefix;\n-\tstruct repository_format repo_fmt;\n+\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n \n \t/*\n \t * We may have read an incomplete configuration before\n@@ -1146,6 +1152,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \n \tstrbuf_release(&dir);\n \tstrbuf_release(&gitdir);\n+\tclear_repository_format(&repo_fmt);\n \n \treturn prefix;\n }\n@@ -1203,9 +1210,10 @@ int git_config_perm(const char *var, const char *value)\n \n void check_repository_format(void)\n {\n-\tstruct repository_format repo_fmt;\n+\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n \tcheck_repository_format_gently(get_git_dir(), &repo_fmt, NULL);\n \tstartup_info->have_repository = 1;\n+\tclear_repository_format(&repo_fmt);\n }\n \n /*\ndiff --git a/worktree.c b/worktree.c\nindex d6a0ee7f73..b45bfeb9d3 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -444,7 +444,7 @@ int submodule_uses_worktrees(const char *path)\n \tDIR *dir;\n \tstruct dirent *d;\n \tint ret = 0;\n-\tstruct repository_format format;\n+\tstruct repository_format format = REPOSITORY_FORMAT_INIT;\n \n \tsubmodule_gitdir = git_pathdup_submodule(path, \"%s\", \"\");\n \tif (!submodule_gitdir)\n@@ -462,8 +462,10 @@ int submodule_uses_worktrees(const char *path)\n \tread_repository_format(&format, sb.buf);\n \tif (format.version != 0) {\n \t\tstrbuf_release(&sb);\n+\t\tclear_repository_format(&format);\n \t\treturn 1;\n \t}\n+\tclear_repository_format(&format);\n \n \t/* Replace config by worktrees. */\n \tstrbuf_setlen(&sb, sb.len - strlen(\"config\"));\n-- \n2.20.1.98.gecbdaf0899\n\n"},{"id":"367404","messageId":"20190123055704.GA19601@sigill.intra.peff.net","threadId":"50054","inReplyTo":"f8b021033b887923662eb9fa63f6df1677ebbbb5.1548186510.git.martin.agren@gmail.com","subject":"Re: [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-23T05:57:05Z","receivedAt":"2019-01-23T05:57:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 22, 2019 at 10:45:48PM +0100, Martin Ågren wrote:\n\n> Call `clear_...()` at the start of `read_...()` instead of just zeroing\n> the struct, since we sometimes enter the function multiple times. This\n> means that it is important to initialize the struct before calling\n> `read_...()`, so document that.\n\nThis part is a little counter-intuitive to me. Is anybody ever going to\npass in anything except a struct initialized to REPOSITORY_FORMAT_INIT?\n\nIf so, might it be kinder for read_...() to not assume anything about\nthe incoming struct, and initialize it from scratch? I.e., not to use\nclear() but just do the initialization step?\n\nA caller which calls read_() multiple times would presumably have an\nintervening clear (either their own, or the one done on an error return\nfrom the read function).\n\nOther than that minor nit, I like the overall shape of this.\n\nOne interesting tidbit:\n\n> +/*\n> + * Always use this to initialize a `struct repository_format`\n> + * to a well-defined, default state before calling\n> + * `read_repository()`.\n> + */\n> +#define REPOSITORY_FORMAT_INIT \\\n> +{ \\\n> +\t.version = -1, \\\n> +\t.is_bare = -1, \\\n> +\t.hash_algo = GIT_HASH_SHA1, \\\n> +\t.unknown_extensions = STRING_LIST_INIT_DUP, \\\n> +}\n> [...]\n> +\tstruct repository_format candidate = REPOSITORY_FORMAT_INIT;\n\nThis uses designated initializers, which is a C99-ism, but one we've\nused previously and feel confident in. But...\n\n> +void clear_repository_format(struct repository_format *format)\n> +{\n> +\tstring_list_clear(&format->unknown_extensions, 0);\n> +\tfree(format->work_tree);\n> +\tfree(format->partial_clone);\n> +\t*format = (struct repository_format)REPOSITORY_FORMAT_INIT;\n> +}\n\n...this uses that expression not as an initializer, but as a compound\nliteral. That's also C99, but AFAIK it's the first usage in our code\nbase. I don't know if it will cause problems or not.\n\nThe \"old\" way to do it is:\n\n struct repository_format foo = REPOSITORY_FORMAT_INIT;\n memcpy(format, &foo, sizeof(foo));\n\nGiven how simple it is to fix if it turns out to be a problem, I'm OK\nincluding it as a weather balloon.\n\n-Peff\n"},{"id":"367510","messageId":"20190124001450.GR423984@genre.crustytoothpaste.net","threadId":"50054","inReplyTo":"20190123055704.GA19601@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-01-24T00:14:51Z","receivedAt":"2019-01-24T00:15:01Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Jan 23, 2019 at 12:57:05AM -0500, Jeff King wrote:\n> This uses designated initializers, which is a C99-ism, but one we've\n> used previously and feel confident in. But...\n> \n> > +void clear_repository_format(struct repository_format *format)\n> > +{\n> > +\tstring_list_clear(&format->unknown_extensions, 0);\n> > +\tfree(format->work_tree);\n> > +\tfree(format->partial_clone);\n> > +\t*format = (struct repository_format)REPOSITORY_FORMAT_INIT;\n> > +}\n> \n> ...this uses that expression not as an initializer, but as a compound\n> literal. That's also C99, but AFAIK it's the first usage in our code\n> base. I don't know if it will cause problems or not.\n> \n> The \"old\" way to do it is:\n> \n> struct repository_format foo = REPOSITORY_FORMAT_INIT;\n> memcpy(format, &foo, sizeof(foo));\n> \n> Given how simple it is to fix if it turns out to be a problem, I'm OK\n> including it as a weather balloon.\n\nIt's my understanding that MSVC doesn't support this construct. If we\ncare about supporting MSVC, then we need to write it without the\ncompound literal. MSVC doesn't support any C99 feature that is not also\nin C++, unfortunately.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"367674","messageId":"CAN0heSoNvTVfC6A8fFK83u4TBX3sLaTJ_NqKwkCZORiCKdVwcA@mail.gmail.com","threadId":"50054","inReplyTo":"20190123055704.GA19601@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-25T19:24:35Z","receivedAt":"2019-01-25T19:24:52Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 23 Jan 2019 at 06:57, Jeff King wrote:\n>\n> On Tue, Jan 22, 2019 at 10:45:48PM +0100, Martin Ågren wrote:\n>\n> > Call `clear_...()` at the start of `read_...()` instead of just zeroing\n> > the struct, since we sometimes enter the function multiple times. This\n> > means that it is important to initialize the struct before calling\n> > `read_...()`, so document that.\n>\n> This part is a little counter-intuitive to me. Is anybody ever going to\n> pass in anything except a struct initialized to REPOSITORY_FORMAT_INIT?\n\nI do update all users in git.git, but yeah, out-of-tree users and\nin-flight topics would segfault.\n\n> If so, might it be kinder for read_...() to not assume anything about\n> the incoming struct, and initialize it from scratch? I.e., not to use\n> clear() but just do the initialization step?\n\nI have some vague memory from going down that route and giving up. Now\nthat I'm looking at it again, I think we can at least try to do\nsomething. We can make sure that \"external\" users that call into setup.c\nare fine (they'll leak, but won't crash). Out-of-tree users inside\nsetup.c will still be able to trip on this. I don't have much spare time\nover the next few days, but I'll get to this.\n\nOr we could accept that we may leak when we end up calling `read()`\nmultiple times (I could catch all leaks now, but new ones might sneak in\nafter that) and come back to this after X months, when we can perhaps\nafford to be a bit more aggressive.\n\nI guess we could just rename the struct to have the compiler catch\nout-of-tree users...\n\n> A caller which calls read_() multiple times would presumably have an\n> intervening clear (either their own, or the one done on an error return\n> from the read function).\n>\n> Other than that minor nit, I like the overall shape of this.\n\nThank you.\n\nMartin\n"},{"id":"367675","messageId":"CAN0heSr6ssaLc6kDdAzny=HxCtxfV8S0+CFJ9-HcU+O1tJ2=rQ@mail.gmail.com","threadId":"50054","inReplyTo":"20190124001450.GR423984@genre.crustytoothpaste.net","subject":"Re: [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-25T19:25:39Z","receivedAt":"2019-01-25T19:25:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 24 Jan 2019 at 01:15, brian m. carlson\n wrote:\n>\n> On Wed, Jan 23, 2019 at 12:57:05AM -0500, Jeff King wrote:\n> > > +void clear_repository_format(struct repository_format *format)\n> > > +{\n> > > + string_list_clear(&format->unknown_extensions, 0);\n> > > + free(format->work_tree);\n> > > + free(format->partial_clone);\n> > > + *format = (struct repository_format)REPOSITORY_FORMAT_INIT;\n> > > +}\n> >\n> > ...this uses that expression not as an initializer, but as a compound\n> > literal. That's also C99, but AFAIK it's the first usage in our code\n> > base. I don't know if it will cause problems or not.\n\n> > Given how simple it is to fix if it turns out to be a problem, I'm OK\n> > including it as a weather balloon.\n\nThanks for pointing out this potential problem.\n\n> It's my understanding that MSVC doesn't support this construct. If we\n> care about supporting MSVC, then we need to write it without the\n> compound literal. MSVC doesn't support any C99 feature that is not also\n> in C++, unfortunately.\n\nOk, better play it safe then. Thanks.\n\nMartin\n"},{"id":"367678","messageId":"20190125195107.GA6520@sigill.intra.peff.net","threadId":"50054","inReplyTo":"CAN0heSoNvTVfC6A8fFK83u4TBX3sLaTJ_NqKwkCZORiCKdVwcA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-25T19:51:07Z","receivedAt":"2019-01-25T19:51:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 25, 2019 at 08:24:35PM +0100, Martin Ågren wrote:\n\n> On Wed, 23 Jan 2019 at 06:57, Jeff King wrote:\n> >\n> > On Tue, Jan 22, 2019 at 10:45:48PM +0100, Martin Ågren wrote:\n> >\n> > > Call `clear_...()` at the start of `read_...()` instead of just zeroing\n> > > the struct, since we sometimes enter the function multiple times. This\n> > > means that it is important to initialize the struct before calling\n> > > `read_...()`, so document that.\n> >\n> > This part is a little counter-intuitive to me. Is anybody ever going to\n> > pass in anything except a struct initialized to REPOSITORY_FORMAT_INIT?\n> \n> I do update all users in git.git, but yeah, out-of-tree users and\n> in-flight topics would segfault.\n> \n> > If so, might it be kinder for read_...() to not assume anything about\n> > the incoming struct, and initialize it from scratch? I.e., not to use\n> > clear() but just do the initialization step?\n> \n> I have some vague memory from going down that route and giving up. Now\n> that I'm looking at it again, I think we can at least try to do\n> something. We can make sure that \"external\" users that call into setup.c\n> are fine (they'll leak, but won't crash). Out-of-tree users inside\n> setup.c will still be able to trip on this. I don't have much spare time\n> over the next few days, but I'll get to this.\n> \n> Or we could accept that we may leak when we end up calling `read()`\n> multiple times (I could catch all leaks now, but new ones might sneak in\n> after that) and come back to this after X months, when we can perhaps\n> afford to be a bit more aggressive.\n> \n> I guess we could just rename the struct to have the compiler catch\n> out-of-tree users...\n\nI'm less worried about out-of-tree users, and more concerned with just\nhaving a calling convention that matches usual conventions (and is\nharder to get wrong).\n\nIt's a pretty minor point, though, so I can live with it either way.\n\n-Peff\n"},{"id":"370124","messageId":"CAN0heSq=0=B-FoPWcuRxp0oJ6capo3Pu4txZZOP2uDeNn5N1Og@mail.gmail.com","threadId":"50054","inReplyTo":"20190125195107.GA6520@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-02-25T19:21:07Z","receivedAt":"2019-02-25T19:21:21Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Fri, 25 Jan 2019 at 20:51, Jeff King wrote:\n>\n> > On Wed, 23 Jan 2019 at 06:57, Jeff King wrote:\n> > >\n> > > On Tue, Jan 22, 2019 at 10:45:48PM +0100, Martin Ågren wrote:\n> > >\n> > > > Call `clear_...()` at the start of `read_...()` instead of just zeroing\n> > > > the struct, since we sometimes enter the function multiple times. This\n> > > > means that it is important to initialize the struct before calling\n> > > > `read_...()`, so document that.\n> > >\n> > > This part is a little counter-intuitive to me. Is anybody ever going to\n> > > pass in anything except a struct initialized to REPOSITORY_FORMAT_INIT?\n\n> > > If so, might it be kinder for read_...() to not assume anything about\n> > > the incoming struct, and initialize it from scratch? I.e., not to use\n> > > clear() but just do the initialization step?\n>\n> I'm less worried about out-of-tree users, and more concerned with just\n> having a calling convention that matches usual conventions (and is\n> harder to get wrong).\n>\n> It's a pretty minor point, though, so I can live with it either way.\n\nIt's time to resurrect this thread. I've reworked this patch to avoid\nthe compound literal when re-initing a struct, and I've been going back\nand forth on this point about having to initialize to `..._INIT` or risk\ncrashing. And I keep coming back to thinking that it's not *that*\ndifferent from how `STRBUF_INIT` works.\n\nThere's the obvious difference that there aren't as many functions to\ncall, so there's certainly a difference in scale. And you'd think that\nyou'll always start with `read_...()`, but another potential first\nfunction to call is `clear_...()` (see builtin/init-db.c), in which case\nyou better have used `..._INIT` first.\n\nI'm tempted to address this point by documenting as good as I can in the\n.h-file that one has to use this initializer macro. I'll obviously\nconvert all users, so copy-paste programming should work fine...\n\nHow does that sound to you?\n\nMartin\n"},{"id":"370256","messageId":"20190226174655.GE19606@sigill.intra.peff.net","threadId":"50054","inReplyTo":"CAN0heSq=0=B-FoPWcuRxp0oJ6capo3Pu4txZZOP2uDeNn5N1Og@mail.gmail.com","subject":"Re: [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-26T17:46:55Z","receivedAt":"2019-02-26T17:46:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 25, 2019 at 08:21:07PM +0100, Martin Ågren wrote:\n\n> It's time to resurrect this thread. I've reworked this patch to avoid\n> the compound literal when re-initing a struct, and I've been going back\n> and forth on this point about having to initialize to `..._INIT` or risk\n> crashing. And I keep coming back to thinking that it's not *that*\n> different from how `STRBUF_INIT` works.\n> \n> There's the obvious difference that there aren't as many functions to\n> call, so there's certainly a difference in scale. And you'd think that\n> you'll always start with `read_...()`, but another potential first\n> function to call is `clear_...()` (see builtin/init-db.c), in which case\n> you better have used `..._INIT` first.\n> \n> I'm tempted to address this point by documenting as good as I can in the\n> .h-file that one has to use this initializer macro. I'll obviously\n> convert all users, so copy-paste programming should work fine...\n> \n> How does that sound to you?\n\nIt sounds pretty good.\n\n-Peff\n"},{"id":"370372","messageId":"cover.1551385992.git.martin.agren@gmail.com","threadId":"50054","inReplyTo":"20190226174655.GE19606@sigill.intra.peff.net","subject":"[PATCH v4 0/2] setup: fix memory leaks with `struct repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-02-28T20:36:26Z","receivedAt":"2019-02-28T20:36:57Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is a follow-up to v3 [1] from about a month ago. Patch 1 is\nunchanged; patch 2 provides some additional documentation of the\ninitialization that is required, plus I've gotten rid of the compound\nliteral. Range-diff below.\n\nThanks Peff and brian for very helpful comments and discussion.\n\nMartin\n \n[1] https://public-inbox.org/git/cover.1548186510.git.martin.agren@gmail.com/\n\nMartin Ågren (2):\n setup: free old value before setting `work_tree`\n setup: fix memory leaks with `struct repository_format`\n\n cache.h | 31 ++++++++++++++++++++++++++++---\n builtin/init-db.c | 3 ++-\n repository.c | 3 ++-\n setup.c | 40 ++++++++++++++++++++++++++++------------\n worktree.c | 4 +++-\n 5 files changed, 63 insertions(+), 18 deletions(-)\n\nRange-diff against v3:\n1: 13019979b8 = 1: 13019979b8 setup: free old value before setting `work_tree`\n2: e0c4a73119 ! 2: b21704c1e4 setup: fix memory leaks with `struct repository_format`\n @@ -16,15 +16,16 @@\n they take from it, rather than stealing the pointers.\n \n Call `clear_...()` at the start of `read_...()` instead of just zeroing\n - the struct, since we sometimes enter the function multiple times. This\n - means that it is important to initialize the struct before calling\n - `read_...()`, so document that. Teach `read_...()` to clear the struct\n - upon an error, so that it is reset to a safe state, and document this.\n + the struct, since we sometimes enter the function multiple times. Thus,\n + it is important to initialize the struct before calling `read_...()`, so\n + document that. It's also important because we might not even call\n + `read_...()` before we call `clear_...()`, see, e.g., builtin/init-db.c.\n \n - About that very last point: In `setup_git_directory_gently()`, we look\n - at `repo_fmt.hash_algo` even if `repo_fmt.version` is -1, which we\n + Teach `read_...()` to clear the struct on error, so that it is reset to\n + a safe state, and document this. (In `setup_git_directory_gently()`, we\n + look at `repo_fmt.hash_algo` even if `repo_fmt.version` is -1, which we\n weren't actually supposed to do per the API. After this commit, that's\n - ok.\n + ok.)\n \n We inherit the existing code's combining \"error\" and \"no version found\".\n Both are signalled through `version == -1` and now both cause us to\n @@ -34,11 +35,21 @@\n non-negative version number before using them.\n \n Signed-off-by: Martin Ågren \n - Signed-off-by: Junio C Hamano \n \n diff --git a/cache.h b/cache.h\n --- a/cache.h\n +++ b/cache.h\n +@@\n + extern const char *core_partial_clone_filter_default;\n + extern int repository_format_worktree_config;\n + \n ++/*\n ++ * You _have_ to initialize a `struct repository_format` using\n ++ * `= REPOSITORY_FORMAT_INIT` before calling `read_repository_format()`.\n ++ */\n + struct repository_format {\n + \tint version;\n + \tint precious_objects;\n @@\n \tstruct string_list unknown_extensions;\n };\n @@ -146,8 +157,15 @@\n \t}\n \n \treturn 0;\n -@@\n + }\n \n ++static void init_repository_format(struct repository_format *format)\n ++{\n ++\tconst struct repository_format fresh = REPOSITORY_FORMAT_INIT;\n ++\n ++\tmemcpy(format, &fresh, sizeof(fresh));\n ++}\n ++\n int read_repository_format(struct repository_format *format, const char *path)\n {\n -\tmemset(format, 0, sizeof(*format));\n @@ -167,7 +185,7 @@\n +\tstring_list_clear(&format->unknown_extensions, 0);\n +\tfree(format->work_tree);\n +\tfree(format->partial_clone);\n -+\t*format = (struct repository_format)REPOSITORY_FORMAT_INIT;\n ++\tinit_repository_format(format);\n +}\n +\n int verify_repository_format(const struct repository_format *format,\n-- \n2.21.0\n\n"},{"id":"370373","messageId":"13019979b811d26f4838d09331c7ddd8223d270d.1551385992.git.martin.agren@gmail.com","threadId":"50054","inReplyTo":"cover.1551385992.git.martin.agren@gmail.com","subject":"[PATCH v4 1/2] setup: free old value before setting `work_tree`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-02-28T20:36:27Z","receivedAt":"2019-02-28T20:36:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Before assigning to `data->work_tree` in `read_worktree_config()`, free\nany value we might already have picked up, so that we do not leak it.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Junio C Hamano \n---\n setup.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/setup.c b/setup.c\nindex 1be5037f12..bb633942bb 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -411,6 +411,7 @@ static int read_worktree_config(const char *var, const char *value, void *vdata)\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}\n \treturn 0;\n-- \n2.21.0\n\n"},{"id":"370374","messageId":"b21704c1e408ff3fc961199f2fc98fd9798f6efd.1551385992.git.martin.agren@gmail.com","threadId":"50054","inReplyTo":"cover.1551385992.git.martin.agren@gmail.com","subject":"[PATCH v4 2/2] setup: fix memory leaks with `struct repository_format`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-02-28T20:36:28Z","receivedAt":"2019-02-28T20:37:00Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After we set up a `struct repository_format`, it owns various pieces of\nallocated memory. We then either use those members, because we decide we\nwant to use the \"candidate\" repository format, or we discard the\ncandidate / scratch space. In the first case, we transfer ownership of\nthe memory to a few global variables. In the latter case, we just\nsilently drop the struct and end up leaking memory.\n\nIntroduce an initialization macro `REPOSITORY_FORMAT_INIT` and a\nfunction `clear_repository_format()`, to be used on each side of\n`read_repository_format()`. To have a clear and simple memory ownership,\nlet all users of `struct repository_format` duplicate the strings that\nthey take from it, rather than stealing the pointers.\n\nCall `clear_...()` at the start of `read_...()` instead of just zeroing\nthe struct, since we sometimes enter the function multiple times. Thus,\nit is important to initialize the struct before calling `read_...()`, so\ndocument that. It's also important because we might not even call\n`read_...()` before we call `clear_...()`, see, e.g., builtin/init-db.c.\n\nTeach `read_...()` to clear the struct on error, so that it is reset to\na safe state, and document this. (In `setup_git_directory_gently()`, we\nlook at `repo_fmt.hash_algo` even if `repo_fmt.version` is -1, which we\nweren't actually supposed to do per the API. After this commit, that's\nok.)\n\nWe inherit the existing code's combining \"error\" and \"no version found\".\nBoth are signalled through `version == -1` and now both cause us to\nclear any partial configuration we have picked up. For \"extensions.*\",\nthat's fine, since they require a positive version number. For\n\"core.bare\" and \"core.worktree\", we're already verifying that we have a\nnon-negative version number before using them.\n\nSigned-off-by: Martin Ågren \n---\n cache.h | 31 ++++++++++++++++++++++++++++---\n builtin/init-db.c | 3 ++-\n repository.c | 3 ++-\n setup.c | 39 +++++++++++++++++++++++++++------------\n worktree.c | 4 +++-\n 5 files changed, 62 insertions(+), 18 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex ca36b44ee0..8c32c904c3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -961,6 +961,10 @@ extern char *repository_format_partial_clone;\n extern const char *core_partial_clone_filter_default;\n extern int repository_format_worktree_config;\n \n+/*\n+ * You _have_ to initialize a `struct repository_format` using\n+ * `= REPOSITORY_FORMAT_INIT` before calling `read_repository_format()`.\n+ */\n struct repository_format {\n \tint version;\n \tint precious_objects;\n@@ -972,14 +976,35 @@ struct repository_format {\n \tstruct string_list unknown_extensions;\n };\n \n+/*\n+ * Always use this to initialize a `struct repository_format`\n+ * to a well-defined, default state before calling\n+ * `read_repository()`.\n+ */\n+#define REPOSITORY_FORMAT_INIT \\\n+{ \\\n+\t.version = -1, \\\n+\t.is_bare = -1, \\\n+\t.hash_algo = GIT_HASH_SHA1, \\\n+\t.unknown_extensions = STRING_LIST_INIT_DUP, \\\n+}\n+\n /*\n * Read the repository format characteristics from the config file \"path\" into\n- * \"format\" struct. Returns the numeric version. On error, -1 is returned,\n- * format->version is set to -1, and all other fields in the struct are\n- * undefined.\n+ * \"format\" struct. Returns the numeric version. On error, or if no version is\n+ * found in the configuration, -1 is returned, format->version is set to -1,\n+ * and all other fields in the struct are set to the default configuration\n+ * (REPOSITORY_FORMAT_INIT). Always initialize the struct using\n+ * REPOSITORY_FORMAT_INIT before calling this function.\n */\n int read_repository_format(struct repository_format *format, const char *path);\n \n+/*\n+ * Free the memory held onto by `format`, but not the struct itself.\n+ * (No need to use this after `read_repository_format()` fails.)\n+ */\n+void clear_repository_format(struct repository_format *format);\n+\n /*\n * Verify that the repository described by repository_format is something we\n * can read. If it is, return 0. Otherwise, return -1, and \"err\" will describe\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 41faffd28d..04c60eaad5 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -96,7 +96,7 @@ static void copy_templates(const char *template_dir)\n \tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf template_path = STRBUF_INIT;\n \tsize_t template_len;\n-\tstruct repository_format template_format;\n+\tstruct repository_format template_format = REPOSITORY_FORMAT_INIT;\n \tstruct strbuf err = STRBUF_INIT;\n \tDIR *dir;\n \tchar *to_free = NULL;\n@@ -148,6 +148,7 @@ static void copy_templates(const char *template_dir)\n \tfree(to_free);\n \tstrbuf_release(&path);\n \tstrbuf_release(&template_path);\n+\tclear_repository_format(&template_format);\n }\n \n static int git_init_db_config(const char *k, const char *v, void *cb)\ndiff --git a/repository.c b/repository.c\nindex 7b02e1dffa..df88705574 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -148,7 +148,7 @@ int repo_init(struct repository *repo,\n \t const char *gitdir,\n \t const char *worktree)\n {\n-\tstruct repository_format format;\n+\tstruct repository_format format = REPOSITORY_FORMAT_INIT;\n \tmemset(repo, 0, sizeof(*repo));\n \n \trepo->objects = raw_object_store_new();\n@@ -165,6 +165,7 @@ int repo_init(struct repository *repo,\n \tif (worktree)\n \t\trepo_set_worktree(repo, worktree);\n \n+\tclear_repository_format(&format);\n \treturn 0;\n \n error:\ndiff --git a/setup.c b/setup.c\nindex bb633942bb..ab6e8fd4c3 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -477,7 +477,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t}\n \n \trepository_format_precious_objects = candidate->precious_objects;\n-\trepository_format_partial_clone = candidate->partial_clone;\n+\trepository_format_partial_clone = xstrdup_or_null(candidate->partial_clone);\n \trepository_format_worktree_config = candidate->worktree_config;\n \tstring_list_clear(&candidate->unknown_extensions, 0);\n \n@@ -500,27 +500,38 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\t}\n \t\tif (candidate->work_tree) {\n \t\t\tfree(git_work_tree_cfg);\n-\t\t\tgit_work_tree_cfg = candidate->work_tree;\n+\t\t\tgit_work_tree_cfg = xstrdup(candidate->work_tree);\n \t\t\tinside_work_tree = -1;\n \t\t}\n-\t} else {\n-\t\tfree(candidate->work_tree);\n \t}\n \n \treturn 0;\n }\n \n+static void init_repository_format(struct repository_format *format)\n+{\n+\tconst struct repository_format fresh = REPOSITORY_FORMAT_INIT;\n+\n+\tmemcpy(format, &fresh, sizeof(fresh));\n+}\n+\n int read_repository_format(struct repository_format *format, const char *path)\n {\n-\tmemset(format, 0, sizeof(*format));\n-\tformat->version = -1;\n-\tformat->is_bare = -1;\n-\tformat->hash_algo = GIT_HASH_SHA1;\n-\tstring_list_init(&format->unknown_extensions, 1);\n+\tclear_repository_format(format);\n \tgit_config_from_file(check_repo_format, path, format);\n+\tif (format->version == -1)\n+\t\tclear_repository_format(format);\n \treturn format->version;\n }\n \n+void clear_repository_format(struct repository_format *format)\n+{\n+\tstring_list_clear(&format->unknown_extensions, 0);\n+\tfree(format->work_tree);\n+\tfree(format->partial_clone);\n+\tinit_repository_format(format);\n+}\n+\n int verify_repository_format(const struct repository_format *format,\n \t\t\t struct strbuf *err)\n {\n@@ -1008,7 +1019,7 @@ int discover_git_directory(struct strbuf *commondir,\n \tstruct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;\n \tsize_t gitdir_offset = gitdir->len, cwd_len;\n \tsize_t commondir_offset = commondir->len;\n-\tstruct repository_format candidate;\n+\tstruct repository_format candidate = REPOSITORY_FORMAT_INIT;\n \n \tif (strbuf_getcwd(&dir))\n \t\treturn -1;\n@@ -1045,9 +1056,11 @@ int discover_git_directory(struct strbuf *commondir,\n \t\tstrbuf_release(&err);\n \t\tstrbuf_setlen(commondir, commondir_offset);\n \t\tstrbuf_setlen(gitdir, gitdir_offset);\n+\t\tclear_repository_format(&candidate);\n \t\treturn -1;\n \t}\n \n+\tclear_repository_format(&candidate);\n \treturn 0;\n }\n \n@@ -1056,7 +1069,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \tstatic struct strbuf cwd = STRBUF_INIT;\n \tstruct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;\n \tconst char *prefix;\n-\tstruct repository_format repo_fmt;\n+\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n \n \t/*\n \t * We may have read an incomplete configuration before\n@@ -1146,6 +1159,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \n \tstrbuf_release(&dir);\n \tstrbuf_release(&gitdir);\n+\tclear_repository_format(&repo_fmt);\n \n \treturn prefix;\n }\n@@ -1203,9 +1217,10 @@ int git_config_perm(const char *var, const char *value)\n \n void check_repository_format(void)\n {\n-\tstruct repository_format repo_fmt;\n+\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n \tcheck_repository_format_gently(get_git_dir(), &repo_fmt, NULL);\n \tstartup_info->have_repository = 1;\n+\tclear_repository_format(&repo_fmt);\n }\n \n /*\ndiff --git a/worktree.c b/worktree.c\nindex d6a0ee7f73..b45bfeb9d3 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -444,7 +444,7 @@ int submodule_uses_worktrees(const char *path)\n \tDIR *dir;\n \tstruct dirent *d;\n \tint ret = 0;\n-\tstruct repository_format format;\n+\tstruct repository_format format = REPOSITORY_FORMAT_INIT;\n \n \tsubmodule_gitdir = git_pathdup_submodule(path, \"%s\", \"\");\n \tif (!submodule_gitdir)\n@@ -462,8 +462,10 @@ int submodule_uses_worktrees(const char *path)\n \tread_repository_format(&format, sb.buf);\n \tif (format.version != 0) {\n \t\tstrbuf_release(&sb);\n+\t\tclear_repository_format(&format);\n \t\treturn 1;\n \t}\n+\tclear_repository_format(&format);\n \n \t/* Replace config by worktrees. */\n \tstrbuf_setlen(&sb, sb.len - strlen(\"config\"));\n-- \n2.21.0\n\n"},{"id":"370783","messageId":"20190306045642.GD6664@sigill.intra.peff.net","threadId":"50054","inReplyTo":"cover.1551385992.git.martin.agren@gmail.com","subject":"Re: [PATCH v4 0/2] setup: fix memory leaks with `struct repository_format`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-06T04:56:42Z","receivedAt":"2019-03-06T04:56:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 28, 2019 at 09:36:26PM +0100, Martin Ågren wrote:\n\n> This is a follow-up to v3 [1] from about a month ago. Patch 1 is\n> unchanged; patch 2 provides some additional documentation of the\n> initialization that is required, plus I've gotten rid of the compound\n> literal. Range-diff below.\n\nThanks, this version looks good to me.\n\n-Peff\n"}]}