[PATCH 0/1] maintenance: Fix a SEGFAULT when no repository when running git maintenance run/start

[PATCH 0/1] maintenance: Fix a SEGFAULT when no repository when running git maintenance run/start

[Rafael Silva]

In d7514f6ed5 (maintenance: take a lock on the objects directory, 2020-09-17) [1],
and 2fec604f8d (maintenance: add start/stop subcommands, 2020-09-11) [2] The
"git maintenance run" and "git maintenance start" was taught to hold a file-based
lock at the .git/maintenance.lock and .git/schedule.lock respectively because these
operations involves writing data into the .git/repository. 

The lock file path string is built using the the_repository->objects->odb->path,
in case the_repository->objects->odb is NULL when there is not repository available,
resulting in a SEGFAULT.

[1] https://lore.kernel.org/git/1a0a3eebb825ac3eabfdd86f82ed7ef6abb454c5.1600366313.git.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/5194f6b1facbd14cc17eea0337c0cc397a2a51fc.1602782524.git.gitgitgadget@gmail.com/

In order to reproduce the error, one can execute maintenance "run" and/or
"start" subcommand with a non valid repository: 

    $ git -C /tmp maintenance start
    Segmentation fault

    $ git -C /tmp maintenance run
    Segmentation fault

The above test was executed from a git built from commit: faefdd61ec (Sixth batch, 2020-11-18):

For reference here's the output from GDB when debugging the "start" command

	Program received signal SIGSEGV, Segmentation fault.
	0x00005555555b9b4c in maintenance_run_tasks (opts=0x7fffffffded4) at builtin/gc.c:1268
	1268		char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path);

This patch serie adds a check on the maintenance_run_tasks() and update_background_schedule()
to check if the_repository->git_dir is set to validate if a git repository is available and
return error otherwise. The logic is borrowed from maintenance_unregister() that performs
the same validation with different error message.

A new test case is added into t/t7900-maintenance.sh to cover these cases. The test is based on
the assumption that the `/tmp` directory is available, readable and is not a git repository
which I hope is fine for the running the test is all platforms. 

This patch is based on faefdd61ec (Sixth batch, 2020-11-18) (master branch) as the both commits
that introduced the file-based lock are graduated to master already. Hope this also plays nice
with the integration branches maintenance-part-v[1,4]. 

Rafael Silva (1):
  maintenance: fix a SEGFAULT when no repository

 builtin/gc.c           | 14 ++++++++++++--
 t/t7900-maintenance.sh |  5 +++++
 2 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.29.2.505.g04529851e5

[PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[Rafael Silva]

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(-)

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
-- 
2.29.2.505.g04529851e5

Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[Derrick Stolee]

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

Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[Eric Sunshine]

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

Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[Martin Ågren]

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

Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[SZEDER Gábor]

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

Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[Eric Sunshine]

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.

Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[Junio C Hamano]

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.

Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[Derrick Stolee]

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

Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[Rafael Silva]

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.

Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[Rafael Silva]

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. 

Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[Rafael Silva]

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

Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository

[Derrick Stolee]

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

[PATCH v2 0/1] maintenance: Fix SEGFAULT when running outside of a repository

[Rafael Silva]

In d7514f6ed5 (maintenance: take a lock on the objects directory, 2020-09-17) [1],
and 2fec604f8d (maintenance: add start/stop subcommands, 2020-09-11) [2] The
"git maintenance run" and "git maintenance start" was taught to hold a file-based
lock at the .git/maintenance.lock and .git/schedule.lock respectively because these
operations involves writing data into the .git/repository. 

The lock file path string is built using the the_repository->objects->odb->path,
in case the_repository->objects->odb is NULL when there is not repository available,
resulting in a SEGFAULT.

In order to reproduce the error, one can execute maintenance "run" and/or
"start" subcommand with a non valid repository: 

    $ git -C /tmp maintenance start
    Segmentation fault

    $ git -C /tmp maintenance run
    Segmentation fault

The above test was executed from a git built from commit: faefdd61ec (Sixth batch, 2020-11-18):

For reference here's the output from GDB when debugging the "start" command

	Program received signal SIGSEGV, Segmentation fault.
	0x00005555555b9b4c in maintenance_run_tasks (opts=0x7fffffffded4) at builtin/gc.c:1268
	1268		char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path);


Updates in v2
=============

  * Instead of implementing the check on the subcommands, a more robust approach
    is taken by replacing the "maintenance" command option to use RUN_SETUP
    instead of RUN_SETUP_GENTLY as suggested by Derrick Stolee in [3] as part of
    review cycle from v1.

    This provides protection for all maintenance subcommands given that
    currently all the commands are required to be executed inside a repository.

  * As the RUN_SETUP will enable protection to all commands, the checks in
    maintenance_register() and maintenance_unregister() are removed as they
    are not required anymore.

  * Use the "nongit" helper function for testing it instead of relying on the
    `/tmp` directory for a non valid git repository as suggested by
    SZEDER Gábor and Eric Sunshine in [4].

  * All "git maintenance" subcommands are included on the test to ensure the
   behaviour is consistent for all subcommands. In particular the "register"
   command that current exists without a message and code 0 which will be
   changed by this patch to fail when running outside of a repository. 

   It also worth noting that "register" command does not exists in a released
   version of Git as mentioned in [5] which make it easier for changing the
   current behaviour of the command

[1] https://lore.kernel.org/git/1a0a3eebb825ac3eabfdd86f82ed7ef6abb454c5.1600366313.git.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/5194f6b1facbd14cc17eea0337c0cc397a2a51fc.1602782524.git.gitgitgadget@gmail.com/
[3] https://lore.kernel.org/git/1bfd84da-5b74-be10-fc2c-dee80111ee2d@gmail.com/
[4] https://lore.kernel.org/git/20201124191407.GC8396@szeder.dev/
[5] https://lore.kernel.org/git/54fa678c-7150-8c48-50e5-b33923a69249@gmail.com/

Thanks everyone for the insightful and helpful feedback.

Rafael Silva (1):
  maintenance: fix SEGFAULT when no repository

 builtin/gc.c           | 7 -------
 git.c                  | 2 +-
 t/t7900-maintenance.sh | 8 ++++++++
 3 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.29.2.367.g37477fb670

[PATCH v2 1/1] maintenance: fix SEGFAULT when no repository

[Rafael Silva]

The "git maintenance run" and "git maintenance start/stop" 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_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 maintenance command to use RUN_SETUP option that will
provide the validation and fail when running outside of a repository.
Hence fixing the SEGFAULT for all three operations and making the
behaviour consistent across all subcommands.

Setting the RUN_SETUP also provides the same protection for all
subcommands given that the "register" and "unregister" also requires to
be executed inside a repository.

Furthermore let's remove the local validation implemented by the
"register" and "unregister" as this will not be required anymore with
the new option.

Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
 builtin/gc.c           | 7 -------
 git.c                  | 2 +-
 t/t7900-maintenance.sh | 8 ++++++++
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index bc25ad52c7..ebb0158308 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1446,10 +1446,6 @@ static int maintenance_register(void)
 	struct child_process config_set = CHILD_PROCESS_INIT;
 	struct child_process config_get = CHILD_PROCESS_INIT;
 
-	/* There is no current repository, so skip registering it */
-	if (!the_repository || !the_repository->gitdir)
-		return 0;
-
 	/* Disable foreground maintenance */
 	git_config_set("maintenance.auto", "false");
 
@@ -1486,9 +1482,6 @@ static int maintenance_unregister(void)
 {
 	struct child_process config_unset = CHILD_PROCESS_INIT;
 
-	if (!the_repository || !the_repository->gitdir)
-		return error(_("no current repository to unregister"));
-
 	config_unset.git_cmd = 1;
 	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
 		     "maintenance.repo",
diff --git a/git.c b/git.c
index 4b7bd77b80..a00a0a4d94 100644
--- a/git.c
+++ b/git.c
@@ -535,7 +535,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 },
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index d9e68bb2bf..ae5c29b0ff 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -441,4 +441,12 @@ test_expect_success 'register preserves existing strategy' '
 	test_config maintenance.strategy incremental
 '
 
+test_execpt_success 'fails when running outside of a repository' '
+	nongit test_must_fail git maintenance run &&
+	nongit test_must_fail git maintenance stop &&
+	nongit test_must_fail git maintenance start &&
+	nongit test_must_fail git maintenance register &&
+	nongit test_must_fail git maintenance unregister
+'
+
 test_done
-- 
2.29.2.367.g37477fb670

Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository

[Derrick Stolee]

On 11/26/2020 3:41 PM, Rafael Silva wrote:
> The "git maintenance run" and "git maintenance start/stop" 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_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 maintenance command to use RUN_SETUP option that will
> provide the validation and fail when running outside of a repository.
> Hence fixing the SEGFAULT for all three operations and making the
> behaviour consistent across all subcommands.
> 
> Setting the RUN_SETUP also provides the same protection for all
> subcommands given that the "register" and "unregister" also requires to
> be executed inside a repository.
> 
> Furthermore let's remove the local validation implemented by the
> "register" and "unregister" as this will not be required anymore with
> the new option.

Thank you for this very clean patch!

Reviewed-by: Derrick Stolee <dstolee@microsoft.com>

Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository

[Josh Steadmon]

On 2020.11.26 20:41, Rafael Silva wrote:
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index d9e68bb2bf..ae5c29b0ff 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -441,4 +441,12 @@ test_expect_success 'register preserves existing strategy' '
>  	test_config maintenance.strategy incremental
>  '
>  
> +test_execpt_success 'fails when running outside of a repository' '
> +	nongit test_must_fail git maintenance run &&
> +	nongit test_must_fail git maintenance stop &&
> +	nongit test_must_fail git maintenance start &&
> +	nongit test_must_fail git maintenance register &&
> +	nongit test_must_fail git maintenance unregister
> +'
> +
>  test_done

Caught a typo here, sending this as a squash patch since it's already in
next:

-- >8 --
Subject: [PATCH] t7900: fix typo: "test_execpt_success"

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/t7900-maintenance.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 4ca3617173..8c061197a6 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -441,7 +441,7 @@ test_expect_success 'register preserves existing strategy' '
 	test_config maintenance.strategy incremental
 '
 
-test_execpt_success 'fails when running outside of a repository' '
+test_expect_success 'fails when running outside of a repository' '
 	nongit test_must_fail git maintenance run &&
 	nongit test_must_fail git maintenance stop &&
 	nongit test_must_fail git maintenance start &&
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> Caught a typo here, sending this as a squash patch since it's already in
> next:

The breakage and the fix looks obvious to me, but...

How did CI allow 'next' to pass with such a typo, I wonder?
Moreover, my pre-push tests of all the integration branches
I didn't notice this to fail, but I cannot see how it could
have been succeeded.  Puzzled...

Will queue, thanks.

>
> -- >8 --
> Subject: [PATCH] t7900: fix typo: "test_execpt_success"
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  t/t7900-maintenance.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 4ca3617173..8c061197a6 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -441,7 +441,7 @@ test_expect_success 'register preserves existing strategy' '
>  	test_config maintenance.strategy incremental
>  '
>  
> -test_execpt_success 'fails when running outside of a repository' '
> +test_expect_success 'fails when running outside of a repository' '
>  	nongit test_must_fail git maintenance run &&
>  	nongit test_must_fail git maintenance stop &&
>  	nongit test_must_fail git maintenance start &&

Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Josh Steadmon <steadmon@google.com> writes:
>
>> Caught a typo here, sending this as a squash patch since it's already in
>> next:
>
> The breakage and the fix looks obvious to me, but...
>
> How did CI allow 'next' to pass with such a typo, I wonder?
> Moreover, my pre-push tests of all the integration branches
> I didn't notice this to fail, but I cannot see how it could
> have been succeeded.  Puzzled...

That is because of this:

    $ (sh t7900-maintenance.sh 2>&1; echo $?) | tail -5
    ok 25 - register preserves existing strategy
    t7900-maintenance.sh: line 444: test_execpt_success: command not found
    # passed all 25 test(s)
    1..25
    0

The story is the same with prove.

    $ prove t7900-maintenance-sh
    t7900-maintenance.sh .. 24/? t7900-maintenance.sh: line 444: test_execpt_success: command not found
    t7900-maintenance.sh .. ok
    All tests successful.
    Files=1, Tests=25,  2 wallclock secs ( 0.02 usr  0.01 sys +  0.97 cusr  0.97 csys =  1.97 CPU)
    Result: PASS

Since this typo appeared immediately before test_done, we _could_
improve test_done to pay attention to $? when it starts (and in a
similar fashion, we _could_ also check $? at the beginning of the
test_expect_* for the previous step), but I do not think that is a
good approach that would scale well.  There are legitimate reasons
we have to write things other than test_expect_* at the top level
of the script (e.g. test helper function may have to be defined to
be shared amongst the test pieces in the same script).

I wonder if it is a good direction to go to run the tests with the
"set -e" option on, and accept its peculiarities.

Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository

[Rafael Silva]

Josh Steadmon writes:

> On 2020.11.26 20:41, Rafael Silva wrote:
>> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
>> index d9e68bb2bf..ae5c29b0ff 100755
>> --- a/t/t7900-maintenance.sh
>> +++ b/t/t7900-maintenance.sh
>> @@ -441,4 +441,12 @@ test_expect_success 'register preserves existing strategy' '
>>  	test_config maintenance.strategy incremental
>>  '
>>  
>> +test_execpt_success 'fails when running outside of a repository' '
>> +	nongit test_must_fail git maintenance run &&
>> +	nongit test_must_fail git maintenance stop &&
>> +	nongit test_must_fail git maintenance start &&
>> +	nongit test_must_fail git maintenance register &&
>> +	nongit test_must_fail git maintenance unregister
>> +'
>> +
>>  test_done
>
> Caught a typo here, sending this as a squash patch since it's already in
> next:
>

Ufff. For some reason I completely missed the test error message when
working on the v2.

Thank you Josh, for the catch and quick patch.

Apologize guys for such mistake.

-- 
Thanks, Rafael

Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository

[Christian Couder]

On Wed, Dec 9, 2020 at 2:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Josh Steadmon <steadmon@google.com> writes:
> >
> >> Caught a typo here, sending this as a squash patch since it's already in
> >> next:
> >
> > The breakage and the fix looks obvious to me, but...
> >
> > How did CI allow 'next' to pass with such a typo, I wonder?
> > Moreover, my pre-push tests of all the integration branches
> > I didn't notice this to fail, but I cannot see how it could
> > have been succeeded.  Puzzled...
>
> That is because of this:
>
>     $ (sh t7900-maintenance.sh 2>&1; echo $?) | tail -5
>     ok 25 - register preserves existing strategy
>     t7900-maintenance.sh: line 444: test_execpt_success: command not found
>     # passed all 25 test(s)
>     1..25
>     0
>
> The story is the same with prove.
>
>     $ prove t7900-maintenance-sh
>     t7900-maintenance.sh .. 24/? t7900-maintenance.sh: line 444: test_execpt_success: command not found
>     t7900-maintenance.sh .. ok
>     All tests successful.
>     Files=1, Tests=25,  2 wallclock secs ( 0.02 usr  0.01 sys +  0.97 cusr  0.97 csys =  1.97 CPU)
>     Result: PASS
>
> Since this typo appeared immediately before test_done, we _could_
> improve test_done to pay attention to $? when it starts (and in a
> similar fashion, we _could_ also check $? at the beginning of the
> test_expect_* for the previous step), but I do not think that is a
> good approach that would scale well.  There are legitimate reasons
> we have to write things other than test_expect_* at the top level
> of the script (e.g. test helper function may have to be defined to
> be shared amongst the test pieces in the same script).
>
> I wonder if it is a good direction to go to run the tests with the
> "set -e" option on, and accept its peculiarities.

Another solution could be to define a command_not_found_handle
function as bourne shells should call that.

By the way it's not the first time we get such an issue, see:

https://lore.kernel.org/git/CAP8UFD15+p+xKwJ=B9WVsrc+2TvLHKmu78SBCLUFZVSYoTtbbg@mail.gmail.com/

Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

>> I wonder if it is a good direction to go to run the tests with the
>> "set -e" option on, and accept its peculiarities.
>
> Another solution could be to define a command_not_found_handle
> function as bourne shells should call that.

I am reasonably sure it is bash-ism and a rather stale stackexchange
question seems to say that command_not_found_handler function (note
the 'r' at the end) is its equivalent in zsh.

Having said that, we already have other bash-specific debugging
support in our test harness, and it may not be a bad idea to use the
facility to catch these bugs, even if the support is available only
when running the tests under bash and no other shell.

> By the way it's not the first time we get such an issue, see:
>
> https://lore.kernel.org/git/CAP8UFD15+p+xKwJ=B9WVsrc+2TvLHKmu78SBCLUFZVSYoTtbbg@mail.gmail.com/

Excellent memory.  Thanks.