| Message ID | 20201124164405.29327-2-rafaeloliveira.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | maintenance: Fix a SEGFAULT when no repository when running git maintenance run/start | expand |
On 11/24/2020 11:44 AM, Rafael Silva wrote: > The "git maintenance run" and "git maintenance start" commands holds a > file-based lock at the .git/maintenance.lock and .git/schedule.lock > respectively. These locks are used to ensure only one maintenance process > is executed at the time as both operations involves writing data into > the git repository. > > The path to the lock file is built using the "the_repository->objects->odb->path" > that results in SEGFAULT when we have no repository available as > "the_repository->objects->odb" is set to NULL. > > Let's teach the maintenance_run_tasks() and update_background_schedule() to return > an error and fails the command when we have no repository available. Thank you for noticing this problem, and for a quick fix. While I don't necessarily have a problem with this approach, perhaps it would be more robust to change the options in git.c to require a GIT_DIR, as in this diff? -- >8 -- diff --git a/git.c b/git.c index 1cab64b5d1..c3dabd2553 100644 --- a/git.c +++ b/git.c @@ -530,7 +530,7 @@ static struct cmd_struct commands[] = { { "ls-tree", cmd_ls_tree, RUN_SETUP }, { "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY | NO_PARSEOPT }, { "mailsplit", cmd_mailsplit, NO_PARSEOPT }, - { "maintenance", cmd_maintenance, RUN_SETUP_GENTLY | NO_PARSEOPT }, + { "maintenance", cmd_maintenance, RUN_SETUP | NO_PARSEOPT }, { "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE }, { "merge-base", cmd_merge_base, RUN_SETUP }, { "merge-file", cmd_merge_file, RUN_SETUP_GENTLY }, -- >8 -- If the above code change fixes your test (below), then that would probably be a safer change. The reason to use RUN_SETUP_GENTLY was probably due to some thought of modifying the background maintenance schedule without being in a Git repository. However, we currently run the [un]register logic inside of the stop|start subcommands, so a GIT_DIR is required there, too. > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index d9e68bb2bf..bb3556888d 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' ' > test_config maintenance.strategy incremental > ' > > +test_expect_success 'run and start command fails when no git repository' ' > + test_must_fail git -C /tmp/ maintenance run && > + test_must_fail git -C /tmp/ maintenance start > +' > + > test_done Thanks, -Stolee
On Tue, Nov 24, 2020 at 11:45 AM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > The "git maintenance run" and "git maintenance start" commands holds a > file-based lock at the .git/maintenance.lock and .git/schedule.lock > respectively. These locks are used to ensure only one maintenance process > is executed at the time as both operations involves writing data into > the git repository. > > The path to the lock file is built using the "the_repository->objects->odb->path" > that results in SEGFAULT when we have no repository available as > "the_repository->objects->odb" is set to NULL. This issue came up in review recently[1] in an unrelated way. [1]: https://lore.kernel.org/git/CAPig+cRFQfg-NLx5dO+BjQpYduhOYs-_+ZRd=DhO8ebWjGB0iA@mail.gmail.com/ > Let's teach the maintenance_run_tasks() and update_background_schedule() to return > an error and fails the command when we have no repository available. > > Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> > --- > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' ' > +test_expect_success 'run and start command fails when no git repository' ' > + test_must_fail git -C /tmp/ maintenance run && > + test_must_fail git -C /tmp/ maintenance start > +' I wouldn't feel comfortable relying upon existence of /tmp/. It might be sufficient to do this instead: mv .git save.git && test_when_finished "mv save.git .git" && test_must_fail git maintenance run && test_must_fail git maintenance start
On Tue, 24 Nov 2020 at 17:47, Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > @@ -1265,9 +1265,14 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts) > { > int i, found_selected = 0; > int result = 0; > + char *lock_path; > struct lock_file lk; > struct repository *r = the_repository; > - char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path); > + > + if (!r || !r->gitdir) > + return error(_("not a git repository")); > + > + lock_path = xstrfmt("%s/maintenance", the_repository->objects->odb->path); s/the_repository/r/ (The preimage uses "r" and you check using "r".) > @@ -1513,8 +1518,13 @@ static int update_background_schedule(int run_maintenance) > FILE *cron_list, *cron_in; > const char *crontab_name; > struct strbuf line = STRBUF_INIT; > + char *lock_path; > struct lock_file lk; > - char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path); > + > + if (!the_repository || !the_repository->gitdir) > + return error(_("not a git repository")); > + > + lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path); Here it's "the_repository" both before and after. Ok. Martin
On Tue, Nov 24, 2020 at 12:24:57PM -0500, Eric Sunshine wrote: > On Tue, Nov 24, 2020 at 11:45 AM Rafael Silva > <rafaeloliveira.cs@gmail.com> wrote: > > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > > @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' ' > > +test_expect_success 'run and start command fails when no git repository' ' > > + test_must_fail git -C /tmp/ maintenance run && > > + test_must_fail git -C /tmp/ maintenance start > > +' > > I wouldn't feel comfortable relying upon existence of /tmp/. Indeed. > It might > be sufficient to do this instead: > > mv .git save.git && > test_when_finished "mv save.git .git" && > test_must_fail git maintenance run && > test_must_fail git maintenance start Our test library contains the 'nongit' helper function exactly for this purpose: nongit test_must_fail git maintenance run && nongit test_must_fail git maintenance start
On Tue, Nov 24, 2020 at 2:14 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > On Tue, Nov 24, 2020 at 12:24:57PM -0500, Eric Sunshine wrote: > > mv .git save.git && > > test_when_finished "mv save.git .git" && > > test_must_fail git maintenance run && > > test_must_fail git maintenance start > > Our test library contains the 'nongit' helper function exactly for > this purpose: > > nongit test_must_fail git maintenance run && > nongit test_must_fail git maintenance start Perfect. I forgot about nongit(). Thanks. I had intended on suggesting GIT_CEILING_DIRECTORIES in my response -- which is what nongit() employs -- but couldn't get it to work for some reason, so I instead suggested moving .git/ aside temporarily.
Derrick Stolee <stolee@gmail.com> writes: > The reason to use RUN_SETUP_GENTLY was probably due to some thought > of modifying the background maintenance schedule without being in a > Git repository. However, we currently run the [un]register logic > inside of the stop|start subcommands, so a GIT_DIR is required there, > too. Meaning all the operations we currently support requires to be done in a repository? If so, that may be acceptable. When a new operation that does not require to be in a repository is added, or when an existing operation is updated not to require to be in a repository, reverting the change and then checking in the implementation of each operation if we are in a repository instead should be easy enough---it pretty much should amount to Rafael's patch, right? But then, being prepared for that future already is also OK, so I can go either way. Please figure it out between you ;-) Thanks, all.
On 11/24/2020 4:48 PM, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >> The reason to use RUN_SETUP_GENTLY was probably due to some thought >> of modifying the background maintenance schedule without being in a >> Git repository. However, we currently run the [un]register logic >> inside of the stop|start subcommands, so a GIT_DIR is required there, >> too. > > Meaning all the operations we currently support requires to be done > in a repository? If so, that may be acceptable. That is the current case. > When a new operation that does not require to be in a repository is > added, or when an existing operation is updated not to require to be > in a repository, reverting the change and then checking in the > implementation of each operation if we are in a repository instead > should be easy enough---it pretty much should amount to Rafael's > patch, right? Yes, I agree. > But then, being prepared for that future already is also OK, so I > can go either way. Please figure it out between you ;-) The only thing I can think is that using RUN_SETUP protects all subcommands immediately. It is equally possible that we add a new subcommand that _also_ requires the GIT_DIR and we need to be sure to implement this check at the beginning of that method, too! But again, I'm just happy that this was found and is being fixed. Rafael: please feel free to take or ignore my suggestion at your discretion. The existing review has also been illuminating. Thanks, -Stolee
On Tue, Nov 24, 2020 at 08:03:33PM +0100, Martin Ågren wrote: > On Tue, 24 Nov 2020 at 17:47, Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > > @@ -1265,9 +1265,14 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts) > > { > > int i, found_selected = 0; > > int result = 0; > > + char *lock_path; > > struct lock_file lk; > > struct repository *r = the_repository; > > - char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path); > > + > > + if (!r || !r->gitdir) > > + return error(_("not a git repository")); > > + > > + lock_path = xstrfmt("%s/maintenance", the_repository->objects->odb->path); > > s/the_repository/r/ > > (The preimage uses "r" and you check using "r".) Thanks. will revise this in the next patch version.
On Tue, Nov 24, 2020 at 08:14:07PM +0100, SZEDER Gábor wrote: > On Tue, Nov 24, 2020 at 12:24:57PM -0500, Eric Sunshine wrote: > > On Tue, Nov 24, 2020 at 11:45 AM Rafael Silva > > <rafaeloliveira.cs@gmail.com> wrote: > > > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > > > @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' ' > > > +test_expect_success 'run and start command fails when no git repository' ' > > > + test_must_fail git -C /tmp/ maintenance run && > > > + test_must_fail git -C /tmp/ maintenance start > > > +' > > > > I wouldn't feel comfortable relying upon existence of /tmp/. > > Indeed. > > > It might > > be sufficient to do this instead: > > > > mv .git save.git && > > test_when_finished "mv save.git .git" && > > test_must_fail git maintenance run && > > test_must_fail git maintenance start > > Our test library contains the 'nongit' helper function exactly for > this purpose: > > nongit test_must_fail git maintenance run && > nongit test_must_fail git maintenance start > I did not know that we have such a test helper and will definitely change on the next revision. Thank you.
On Tue, Nov 24, 2020 at 12:22:40PM -0500, Derrick Stolee wrote: > On 11/24/2020 11:44 AM, Rafael Silva wrote: > > The "git maintenance run" and "git maintenance start" commands holds a > > file-based lock at the .git/maintenance.lock and .git/schedule.lock > > respectively. These locks are used to ensure only one maintenance process > > is executed at the time as both operations involves writing data into > > the git repository. > > > > The path to the lock file is built using the "the_repository->objects->odb->path" > > that results in SEGFAULT when we have no repository available as > > "the_repository->objects->odb" is set to NULL. > > > > Let's teach the maintenance_run_tasks() and update_background_schedule() to return > > an error and fails the command when we have no repository available. > > Thank you for noticing this problem, and for a quick fix. > > While I don't necessarily have a problem with this approach, perhaps > it would be more robust to change the options in git.c to require a > GIT_DIR, as in this diff? > > -- >8 -- > > diff --git a/git.c b/git.c > index 1cab64b5d1..c3dabd2553 100644 > --- a/git.c > +++ b/git.c > @@ -530,7 +530,7 @@ static struct cmd_struct commands[] = { > { "ls-tree", cmd_ls_tree, RUN_SETUP }, > { "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY | NO_PARSEOPT }, > { "mailsplit", cmd_mailsplit, NO_PARSEOPT }, > - { "maintenance", cmd_maintenance, RUN_SETUP_GENTLY | NO_PARSEOPT }, > + { "maintenance", cmd_maintenance, RUN_SETUP | NO_PARSEOPT }, > { "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE }, > { "merge-base", cmd_merge_base, RUN_SETUP }, > { "merge-file", cmd_merge_file, RUN_SETUP_GENTLY }, > > -- >8 -- > Thank you for the review and the attached patch! > If the above code change fixes your test (below), then that would > probably be a safer change. I agree, switching the maintenance command option to use RUN_SETUP seems like a nicer approach here. Given the all current operations requires the command to be executed inside the a git repository this will make the command consistent across the subcommand. Also, it seems this provides an opportunity to cleanup the register and unregister subcommands that currently implement the check to ensure the commands are running from a git repository. > > The reason to use RUN_SETUP_GENTLY was probably due to some thought > of modifying the background maintenance schedule without being in a > Git repository. However, we currently run the [un]register logic > inside of the stop|start subcommands, so a GIT_DIR is required there, > too. > Indeed. Aside from this reason, another concern that I have is that switching the validation to all subcommands (on this case by switching the maintenance command option) will change a bit the behaviour of register subcommand. Currently, the behaviour of "register" subcommand is to return with 0 without any messages when running outside of repository and switching will make the command fail instead. Nevertheless, I am inclined to go with your suggestion given that it seems better approach to support the automatically and make the behaviour consistent for all subcommands given that changing the behaviour of "git maintenance register" command will (hopefully) be okay. Thanks, Rafael
On 11/26/2020 3:22 AM, Rafael Silva wrote: > On Tue, Nov 24, 2020 at 12:22:40PM -0500, Derrick Stolee wrote: >> If the above code change fixes your test (below), then that would >> probably be a safer change. > > I agree, switching the maintenance command option to use RUN_SETUP > seems like a nicer approach here. Given the all current operations > requires the command to be executed inside the a git repository this > will make the command consistent across the subcommand. > > Also, it seems this provides an opportunity to cleanup the > register and unregister subcommands that currently implement the > check to ensure the commands are running from a git repository. > >> >> The reason to use RUN_SETUP_GENTLY was probably due to some thought >> of modifying the background maintenance schedule without being in a >> Git repository. However, we currently run the [un]register logic >> inside of the stop|start subcommands, so a GIT_DIR is required there, >> too. >> > > Indeed. Aside from this reason, another concern that I have is that > switching the validation to all subcommands (on this case by switching > the maintenance command option) will change a bit the behaviour of register > subcommand. Currently, the behaviour of "register" subcommand is to return > with 0 without any messages when running outside of repository and switching > will make the command fail instead. Excellent point. It would be good to cover this case with a test, to demonstrate that as _intended_ behavior. It makes sense to fail instead of "succeed" when doing nothing. > Nevertheless, I am inclined to go with your suggestion given that it seems > better approach to support the automatically and make the behaviour > consistent for all subcommands given that changing the behaviour of > "git maintenance register" command will (hopefully) be okay. Yes. I would say that changing that behavior aligns it with what it should be doing. The best news is that the 'register' subcommand does not exist in a released version of Git, so no one depends on the current behavior. Thanks, -Stolee
diff --git a/builtin/gc.c b/builtin/gc.c index 3d258b60c2..d133d93a86 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1265,9 +1265,14 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts) { int i, found_selected = 0; int result = 0; + char *lock_path; struct lock_file lk; struct repository *r = the_repository; - char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path); + + if (!r || !r->gitdir) + return error(_("not a git repository")); + + lock_path = xstrfmt("%s/maintenance", the_repository->objects->odb->path); if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) { /* @@ -1513,8 +1518,13 @@ static int update_background_schedule(int run_maintenance) FILE *cron_list, *cron_in; const char *crontab_name; struct strbuf line = STRBUF_INIT; + char *lock_path; struct lock_file lk; - char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path); + + if (!the_repository || !the_repository->gitdir) + return error(_("not a git repository")); + + lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path); if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) return error(_("another process is scheduling background maintenance")); diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index d9e68bb2bf..bb3556888d 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' ' test_config maintenance.strategy incremental ' +test_expect_success 'run and start command fails when no git repository' ' + test_must_fail git -C /tmp/ maintenance run && + test_must_fail git -C /tmp/ maintenance start +' + test_done
The "git maintenance run" and "git maintenance start" commands holds a file-based lock at the .git/maintenance.lock and .git/schedule.lock respectively. These locks are used to ensure only one maintenance process is executed at the time as both operations involves writing data into the git repository. The path to the lock file is built using the "the_repository->objects->odb->path" that results in SEGFAULT when we have no repository available as "the_repository->objects->odb" is set to NULL. Let's teach the maintenance_run_tasks() and update_background_schedule() to return an error and fails the command when we have no repository available. Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> --- builtin/gc.c | 14 ++++++++++++-- t/t7900-maintenance.sh | 5 +++++ 2 files changed, 17 insertions(+), 2 deletions(-)