{"thread":{"id":"55627","subject":"[PATCH] builtin/init-db: preemptively clear repo_fmt to avoid leaks","startedAt":"2021-05-05T18:40:26Z","lastAt":"2021-05-06T14:01:05Z","messageCount":4,"participants":["Andrzej Hunt via GitGitGadget","Johannes Schindelin","Andrzej Hunt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"423710","messageId":"pull.1018.git.git.1620240022594.gitgitgadget@gmail.com","threadId":"55627","inReplyTo":null,"subject":"[PATCH] builtin/init-db: preemptively clear repo_fmt to avoid leaks","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-05T18:40:22Z","receivedAt":"2021-05-05T18:40:26Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt <ajrhunt@google.com>\n\ninit_db() could populate various fields on repo_fmt, we add a\nclear_repo_fmt() call to guarantee that we won't leak any of repo_fmt's\nmembers.\n\nAt this time, we do not allocate any new data into repo_fmt, but that\nchanges in the following patch that is currently in seen:\n  https://lore.kernel.org/git/2fd7cb8c0983501e2af2f195aec81d7c17fb80e1.1618832277.git.gitgitgadget@gmail.com/\nBecause it adds the following allocation in init_db():\n  repo_fmt.ref_storage = xstrdup(ref_storage_format);\n\nThe patch linked to above is only exposing a preexisting problem - we\ncan add clear_repository_format() independently to preemptively avoid\nthis class of leak entirely.\n\nLeaks in init_db() are of little significance as this function is called\nimmediately at the end of cmd_init_db(). However a leak in init_db()\nwill cause all known leak-free tests (t0000+1+5) to start failing when\nrun with LSAN - preemptively plugging such leaks is therefore helpful to\navoid regressing on those tests, and more significantly helps the ongoing\nefforts to get all of t0000-t0099 to run leak-free.\n\nLSAN output from t0000 on seen:\n\nDirect leak of 6 byte(s) in 1 object(s) allocated from:\n    #0 0x4868b4 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3\n    #1 0xaa1218 in xstrdup wrapper.c:29:14\n    #2 0x5a4b0a in init_db builtin/init-db.c:442:25\n    #3 0x5a7a17 in cmd_init_db builtin/init-db.c:742:9\n    #4 0x4ce8fe in run_builtin git.c:461:11\n    #5 0x4ccbc8 in handle_builtin git.c:718:3\n    #6 0x4cb0cc in run_argv git.c:785:4\n    #7 0x4cb0cc in cmd_main git.c:916:19\n    #8 0x6bf63d in main common-main.c:52:11\n    #9 0x7f3222f5f349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n\nSUMMARY: AddressSanitizer: 6 byte(s) leaked in 1 allocation(s).\n\nSigned-off-by: Andrzej Hunt <ajrhunt@google.com>\n---\n    builtin/init-db: preemptively plug repo_fmt leak\n    \n    This patch preemptively plugs a leak which is currently only occuring in\n    seen in hn/reftable.\n    \n    This patch is orthogonal to that series (that series starts allocating\n    new memory into an already existing struct - and my patch only adds a\n    clear call for that struct - in other words my patch is safe to use\n    independent of hn/reftable). But there's also no good reason to use this\n    patch independently of that series since we're not leaking prior to that\n    series. I'm not sure what the optimal logistics with my patch are, feel\n    free to integrate it into that series and/or to fold my change into the\n    relevant commit if that's easiest!\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1018%2Fahunt%2Finit_db_leak-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1018/ahunt/init_db_leak-v1\nPull-Request: https://github.com/git/git/pull/1018\n\n builtin/init-db.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 31b718259a64..b25147ebaf59 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -512,6 +512,7 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t\t\t       git_dir, len && git_dir[len-1] != '/' ? \"/\" : \"\");\n \t}\n \n+\tclear_repository_format(&repo_fmt);\n \tfree(original_git_dir);\n \treturn 0;\n }\n\nbase-commit: c3901c8daf043cdc16ffb20d3a410f3a2d5494fd\n-- \ngitgitgadget\n"},{"id":"423715","messageId":"nycvar.QRO.7.76.6.2105052125452.50@tvgsbejvaqbjf.bet","threadId":"55627","inReplyTo":"pull.1018.git.git.1620240022594.gitgitgadget@gmail.com","subject":"Re: [PATCH] builtin/init-db: preemptively clear repo_fmt to avoid leaks","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-05-05T19:28:26Z","receivedAt":"2021-05-05T19:28:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Andrzej,\n\nOn Wed, 5 May 2021, Andrzej Hunt via GitGitGadget wrote:\n\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index 31b718259a64..b25147ebaf59 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -512,6 +512,7 @@ int init_db(const char *git_dir, const char *real_git_dir,\n>  \t\t\t       git_dir, len && git_dir[len-1] != '/' ? \"/\" : \"\");\n>  \t}\n>\n> +\tclear_repository_format(&repo_fmt);\n\nI am afraid that this might not be correct, as t0410.27 experienced a\nsegmentation fault (see\nhttps://github.com/git/git/pull/1018/checks?check_run_id=2511749719#step:5:2845\nfor the full details):\n\n-- snip --\n[...]\n + repack_and_check -a bd6a66906d425462f8fbf1e3a61cda48cc4e456c\n3251618c5193c77083052fb7d6ed5e8b0b227a94\n+ rm -rf repo2\n+ cp -r repo repo2\n+ git -C repo2 repack -a -d\nwarning: reflog of 'HEAD' references pruned commits\nwarning: reflog of 'refs/heads/master' references pruned commits\nSegmentation fault (core dumped)\nerror: last command exited with $?=139\nnot ok 27 - repack -d does not irreversibly delete promisor objects\n-- snap --\n\nWhen I encountered similar problems with my patches in the past, I found\nthe `--valgrind-only` option incredibly useful, i.e. something like\n\n\tcd t && sh t0410-*.sh -i -v -x --valgrind-only=27\n\nMaybe you can give it a try?\n\nCiao,\nDscho\n\n>  \tfree(original_git_dir);\n>  \treturn 0;\n>  }\n>\n> base-commit: c3901c8daf043cdc16ffb20d3a410f3a2d5494fd\n> --\n> gitgitgadget\n>\n>\n"},{"id":"423742","messageId":"79cd3791-bf6e-df3e-1045-c51801406905@ahunt.org","threadId":"55627","inReplyTo":"nycvar.QRO.7.76.6.2105052125452.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] builtin/init-db: preemptively clear repo_fmt to avoid leaks","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-05-06T05:40:13Z","receivedAt":"2021-05-06T05:41:37Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"Hi Johannes,\n\nOn 05/05/2021 21:28, Johannes Schindelin wrote:\n> Hi Andrzej,\n> \n> On Wed, 5 May 2021, Andrzej Hunt via GitGitGadget wrote:\n> \n>> diff --git a/builtin/init-db.c b/builtin/init-db.c\n>> index 31b718259a64..b25147ebaf59 100644\n>> --- a/builtin/init-db.c\n>> +++ b/builtin/init-db.c\n>> @@ -512,6 +512,7 @@ int init_db(const char *git_dir, const char *real_git_dir,\n>>   \t\t\t       git_dir, len && git_dir[len-1] != '/' ? \"/\" : \"\");\n>>   \t}\n>>\n>> +\tclear_repository_format(&repo_fmt);\n> \n> I am afraid that this might not be correct, as t0410.27 experienced a\n> segmentation fault (see\n> https://github.com/git/git/pull/1018/checks?check_run_id=2511749719#step:5:2845\n> for the full details):\n\n\nThanks for spotting that. On further investigation this looks like a \npreexisting issue on seen (my github PR was based on seen - in hindsight \nthat was probably not a good idea) - here's a CI run from seen this \nmorning exhibiting the same failure:\n   https://github.com/git/git/runs/2515095446?check_suite_focus=true\n\nTo gain more confidence, I've rebased my patch onto next, and I no \nlonger hit any CI failures:\n   https://github.com/ahunt/git/runs/2515309312?check_suite_focus=true\n(I ran this on my fork because changing the base branch appears to have \nconfused the PR's CI configuration.)\n\n\nNevertheless, I was still curious about what might be causing the \nfailure in the first place: it appears to only happen in CI with gcc on \nLinux, and I was not able to repro on my machine using (tested with \ngcc-7.5 and gcc-10, with seen from this morning):\n   make CC=gcc-10 T=t0410-partial-clone.sh test\n\nI'm guessing that understanding this failure might require the help of \nsomeone with an Ubuntu install to debug (unless there's some easy way of \nusing Github's Actions to run a bisect)?\n\n-- snip --\n\nATB,\n\n   Andrzej\n"},{"id":"423774","messageId":"nycvar.QRO.7.76.6.2105061551360.50@tvgsbejvaqbjf.bet","threadId":"55627","inReplyTo":"79cd3791-bf6e-df3e-1045-c51801406905@ahunt.org","subject":"Re: [PATCH] builtin/init-db: preemptively clear repo_fmt to avoid leaks","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-05-06T14:00:58Z","receivedAt":"2021-05-06T14:01:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Andrzej,\n\nOn Thu, 6 May 2021, Andrzej Hunt wrote:\n\n> On 05/05/2021 21:28, Johannes Schindelin wrote:\n>\n> > On Wed, 5 May 2021, Andrzej Hunt via GitGitGadget wrote:\n> >\n> > > diff --git a/builtin/init-db.c b/builtin/init-db.c\n> > > index 31b718259a64..b25147ebaf59 100644\n> > > --- a/builtin/init-db.c\n> > > +++ b/builtin/init-db.c\n> > > @@ -512,6 +512,7 @@ int init_db(const char *git_dir, const char\n> > > *real_git_dir,\n> > >    \t\t       git_dir, len && git_dir[len-1] != '/' ? \"/\" : \"\");\n> > >    }\n> > >\n> > > +\tclear_repository_format(&repo_fmt);\n> >\n> > I am afraid that this might not be correct, as t0410.27 experienced a\n> > segmentation fault (see\n> > https://github.com/git/git/pull/1018/checks?check_run_id=2511749719#step:5:2845\n> > for the full details):\n>\n>\n> Thanks for spotting that. On further investigation this looks like a\n> preexisting issue on seen (my github PR was based on seen - in hindsight that\n> was probably not a good idea) - here's a CI run from seen this morning\n> exhibiting the same failure:\n>   https://github.com/git/git/runs/2515095446?check_suite_focus=true\n>\n> To gain more confidence, I've rebased my patch onto next, and I no longer hit\n> any CI failures:\n>   https://github.com/ahunt/git/runs/2515309312?check_suite_focus=true\n> (I ran this on my fork because changing the base branch appears to have\n> confused the PR's CI configuration.)\n\nOh, I'm sorry! But it is great news that this is not _your_ patch's\nproblem (I had a brief look over it and wondered why it would cause this\nsegfault).\n\n> Nevertheless, I was still curious about what might be causing the failure in\n> the first place: it appears to only happen in CI with gcc on Linux, and I was\n> not able to repro on my machine using (tested with gcc-7.5 and gcc-10, with\n> seen from this morning):\n>   make CC=gcc-10 T=t0410-partial-clone.sh test\n\nThere are more settings used in the CI run, indeed. So yeah, it would be\ngood to debug this in an environment where it does fail.\n\n> I'm guessing that understanding this failure might require the help of someone\n> with an Ubuntu install to debug (unless there's some easy way of using\n> Github's Actions to run a bisect)?\n\nAs a matter of fact, there is a neat way in GitHub Actions to debug this:\nthe `action-tmate` Action. It installs and starts a `tmate` instance,\nwhich is a `tmux` inside kind of a proxied SSH server.\n\nThis is what I usually do when I have time to track down such problems: I\nedit `.github/workflows/main.yml` and push it into a branch in my fork.\nThe edits are essentially:\n\n- I delete the `ci-config` job (because I know that I want to run this\n  workflow and don't want to wait for that job to be queued and run)\n\n- Obviously, this requires all the `needs: ci-config` and\n  `if: needs.ci-config.outputs.enabled == 'yes'` lines to be deleted, too.\n\n- I also delete the jobs, tasks, and matrix elements that are not\n  necessary for my debugging session (in your case, that would be e.g. the\n  `dockerized` job as well as the vectors in `regular` that are not\n  `linux-gcc`).\n\n- Then comes the most important part: after the\n  `ci/print-test-failures.sh` task, I insert this task:\n\n    - name: action-tmate\n      if: failure()\n      uses: mxschmitt/action-tmate@v3\n      with:\n        limit-access-to-actor: true\n\nThe `limit-access-to-actor` thing only applies if you uploaded your public\nSSH key to your GitHub profile. Otherwise, anybody who monitors the run\ncould see the command-line to connect to your debugging instance and\ninterfere with you.\n\nFor details, see https://mxschmitt.github.io/action-tmate/\n\nThanks,\nDscho\n"}]}