git maintenance broken on FreeBSD

git maintenance broken on FreeBSD

[Renato Botelho]

As reported at [1], git maintenance is not working on FreeBSD.  I didn't 
find the time to dig into it but it seems like it's calling crontab 
using parameters not supported on FreeBSD.

[1] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=260746
-- 
Renato Botelho

Re: git maintenance broken on FreeBSD

[Đoàn Trần Công Danh]

On 2022-08-12 10:51:03-0300, Renato Botelho <garga@FreeBSD.org> wrote:
> As reported at [1], git maintenance is not working on FreeBSD.  I didn't
> find the time to dig into it but it seems like it's calling crontab using
> parameters not supported on FreeBSD.
> 
> [1] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=260746

It seems like FreeBSD's cron is vixie-cron which requires <file>
passed to crontab(1).

     The crontab command conforms to IEEE Std 1003.2 (“POSIX.2”) with the
     exception that the dangerous variant of calling crontab without a file
     name in the first form of the command is not allowed by this
     implementation.  The pseudo-filename ‘-’ must be specified to read from
     standard input.  The new command syntax differs from previous versions of
     Vixie Cron, as well as from the classic SVR3 syntax.

I think other crontab implementation also accept "-" as filename for stdin.
At least cronie, fcron, dcron, and busybox's crontab both supports "-" as stdin.

I think this patch can fix FreeBSD's problem:

---- 8< -----
diff --git a/builtin/gc.c b/builtin/gc.c
index eeff2b760e..45d908def3 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2087,6 +2087,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 	rewind(cron_list);
 
 	strvec_split(&crontab_edit.args, cmd);
+	strvec_push(&crontab_edit.args, "-");
 	crontab_edit.in = -1;
 	crontab_edit.git_cmd = 0;
 
---- 8< ---------


-- 
Danh

Re: git maintenance broken on FreeBSD

[Todd Zullinger]

Đoàn Trần Công Danh wrote:
> On 2022-08-12 10:51:03-0300, Renato Botelho <garga@FreeBSD.org> wrote:
>> As reported at [1], git maintenance is not working on FreeBSD.  I didn't
>> find the time to dig into it but it seems like it's calling crontab using
>> parameters not supported on FreeBSD.
>> 
>> [1] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=260746
> 
> It seems like FreeBSD's cron is vixie-cron which requires <file>
> passed to crontab(1).
> 
>      The crontab command conforms to IEEE Std 1003.2 (“POSIX.2”) with the
>      exception that the dangerous variant of calling crontab without a file
>      name in the first form of the command is not allowed by this
>      implementation.  The pseudo-filename ‘-’ must be specified to read from
>      standard input.  The new command syntax differs from previous versions of
>      Vixie Cron, as well as from the classic SVR3 syntax.
> 
> I think other crontab implementation also accept "-" as filename for stdin.
> At least cronie, fcron, dcron, and busybox's crontab both supports "-" as stdin.

A similar issue was noted in Fedora with cronie shortly
after the git maintenance command was released:

    https://bugzilla.redhat.com/show_bug.cgi?id=1939930#c1

I noted that a patch just like the one below would suffice,
but I was concerned that it wouldn't be welcome here because
the behavior of crontab was specified by POSIX (even though
it's very unfriendly and, apparently, supported by fewer and
fewer implementations).

If a change like this is made, aren't we trading one group
of broken users for another?  It would fix users of newer
systems at the expense of those on older systems, I would
suspect.

> I think this patch can fix FreeBSD's problem:
> 
> ---- 8< -----
> diff --git a/builtin/gc.c b/builtin/gc.c
> index eeff2b760e..45d908def3 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2087,6 +2087,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)
>  	rewind(cron_list);
>  
>  	strvec_split(&crontab_edit.args, cmd);
> +	strvec_push(&crontab_edit.args, "-");
>  	crontab_edit.in = -1;
>  	crontab_edit.git_cmd = 0;
>  
> ---- 8< ---------

In the end, cronie adjusted it's behavior, which was similar
to that of the newer vixie-cron, in 8b0241f (Partially
revert the behavior of crontab command without arguments,
2021-03-17)¹.  It now behaves as required by POSIX if stdin
is not a TTY.  That seems like a reasonable compromise and
perhaps vixie-cron would be willing to do the same?

¹ https://github.com/cronie-crond/cronie/commit/8b0241f

-- 
Todd

Re: git maintenance broken on FreeBSD

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

> If a change like this is made, aren't we trading one group
> of broken users for another?  It would fix users of newer
> systems at the expense of those on older systems, I would
> suspect.

Thanks for raising this.  The description of POSIX "crontab"
command, cf.

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/crontab.html 

talks about optional "file", but it is explicit that it has to be a
real file, i.e.

    file
        The pathname of a file that contains specifications, in the
        format defined in the INPUT FILES section, for crontab
        entries.

I would suspect that implementations may treat it as a sign to read
the standard input, but I do not think that is what the above
specifies.  For example, description of "file" argument of another
command, "diff", cf.

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html

explicitly calls out that "-" stands for the standard input, i.e.

    diff [-c|-e|-f|-u|-C n|-U n] [-br] file1 file2

    file1, file2
        A pathname of a file to be compared. If either the file1 or
        file2 operand is '-', the standard input shall be used in
        its place.

So, it is fairly clear that "crontab" wants a real file.  Somebody's
POSIX compliant "crontab" can be fed "-", attempt to read from a
file with such a name, and legitimately fail.  And on such a system,
the proposed patch causes a regression.

> In the end, cronie adjusted it's behavior, which was similar
> to that of the newer vixie-cron, in 8b0241f (Partially
> revert the behavior of crontab command without arguments,
> 2021-03-17)¹.  It now behaves as required by POSIX if stdin
> is not a TTY.  That seems like a reasonable compromise and
> perhaps vixie-cron would be willing to do the same?

It indeed is a pragmatic solution to use isatty() as a hint.  

Re: git maintenance broken on FreeBSD

[Đoàn Trần Công Danh]

On 2022-08-12 22:02:08-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Todd Zullinger <tmz@pobox.com> writes:
> 
> > If a change like this is made, aren't we trading one group
> > of broken users for another?  It would fix users of newer
> > systems at the expense of those on older systems, I would
> > suspect.
> 
> So, it is fairly clear that "crontab" wants a real file.  Somebody's
> POSIX compliant "crontab" can be fed "-", attempt to read from a
> file with such a name, and legitimately fail.  And on such a system,
> the proposed patch causes a regression.

Then, we are getting back to point #0, we don't have universally way
to specify stdin as input file for crontab(1) and "crontab -e" is
optional.

Perhaps, FreeBSD needs to carry this patch downstream; or
we will invent new preprocessor, let's say CRONTAB_DASH_IS_STDIN
which is defined in FreeBSD, and another config, let's say
crontab.dashIsStdin (in order to allow users swap their default
cron) which is default to 1 if CRONTAB_DASH_IS_STDIN is defined,
and 0 otherwise.  So, everyone will be happy.  Thought?

-- 
Danh

Re: git maintenance broken on FreeBSD

[Junio C Hamano]

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> Then, we are getting back to point #0, we don't have universally way
> to specify stdin as input file for crontab(1) and "crontab -e" is
> optional.
>
> Perhaps, FreeBSD needs to carry this patch downstream; or
> we will invent new preprocessor, let's say CRONTAB_DASH_IS_STDIN
> which is defined in FreeBSD,

Does FreeBSD offer choices of cron implementations other than Vixie,
just like some Linux distributions?  If somebody on a non-FreeBSD
platform happens to choose to use Vixie, then they would presumably
have the same problem, so a compile-time switch, whose default is
hardcoded based on the target platform, would not work very well.
The default will be wrong for some users, and users can later choose
to switch between different cron implementations.

Configuration knob can be used as a workaround, but in this case, I
am not sure if it is worth doing.  What's the downside of securely
opening a temporary file and write whatever we are currently piping
to a spawned "crontab" command and then giving the path to that
temporary file to the "crontab" command?  Wouldn't that give us the
maximal portability without that much code, no?

I think this is all Derrick's code from 2fec604f (maintenance: add
start/stop subcommands, 2020-09-11), so let's add him to the
discussion.

Re: git maintenance broken on FreeBSD

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

On 2022-08-13 at 17:26:05, Junio C Hamano wrote:
> Does FreeBSD offer choices of cron implementations other than Vixie,
> just like some Linux distributions?  If somebody on a non-FreeBSD
> platform happens to choose to use Vixie, then they would presumably
> have the same problem, so a compile-time switch, whose default is
> hardcoded based on the target platform, would not work very well.
> The default will be wrong for some users, and users can later choose
> to switch between different cron implementations.

I'm using Debian unstable, and I'm using Vixie cron.  I believe that's
the default implementation.  However, I could also well use cronie,
since that's available in Debian as well.  So, yeah, I think this is a
thing to consider.

> Configuration knob can be used as a workaround, but in this case, I
> am not sure if it is worth doing.  What's the downside of securely
> opening a temporary file and write whatever we are currently piping
> to a spawned "crontab" command and then giving the path to that
> temporary file to the "crontab" command?  Wouldn't that give us the
> maximal portability without that much code, no?

I think we should try to provide an option which works across at least
the versions on a particular OS.  The temporary file seems like a nice,
portable option, so I think we should just do that unless there's some
practical objection.

If Derrick doesn't get to it this next week, I can send a patch.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: git maintenance broken on FreeBSD

[Derrick Stolee]

On 8/13/2022 1:35 PM, brian m. carlson wrote:
> On 2022-08-13 at 17:26:05, Junio C Hamano wrote:
>> Does FreeBSD offer choices of cron implementations other than Vixie,
>> just like some Linux distributions?  If somebody on a non-FreeBSD
>> platform happens to choose to use Vixie, then they would presumably
>> have the same problem, so a compile-time switch, whose default is
>> hardcoded based on the target platform, would not work very well.
>> The default will be wrong for some users, and users can later choose
>> to switch between different cron implementations.
> 
> I'm using Debian unstable, and I'm using Vixie cron.  I believe that's
> the default implementation.  However, I could also well use cronie,
> since that's available in Debian as well.  So, yeah, I think this is a
> thing to consider.
> 
>> Configuration knob can be used as a workaround, but in this case, I
>> am not sure if it is worth doing.  What's the downside of securely
>> opening a temporary file and write whatever we are currently piping
>> to a spawned "crontab" command and then giving the path to that
>> temporary file to the "crontab" command?  Wouldn't that give us the
>> maximal portability without that much code, no?
> 
> I think we should try to provide an option which works across at least
> the versions on a particular OS.  The temporary file seems like a nice,
> portable option, so I think we should just do that unless there's some
> practical objection.
> 
> If Derrick doesn't get to it this next week, I can send a patch.

I agree that the tempfile approach makes the most sense in terms of
what we can do within the Git codebase.

I won't be able to get to this change this week, so I'd be happy to
review one of yours, brian. Be careful to test manually when making
this change, because our tests don't actually interact with the system's
crontab and instead verify the interaction using replacement commands.

Thanks,
-Stolee

Re: git maintenance broken on FreeBSD

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> I agree that the tempfile approach makes the most sense in terms of
> what we can do within the Git codebase.
>
> I won't be able to get to this change this week, so I'd be happy to
> review one of yours, brian. Be careful to test manually when making
> this change, because our tests don't actually interact with the system's
> crontab and instead verify the interaction using replacement commands.

Thanks.  I didn't mean "it's your code, go fix it".  It was "you
are one of the folks who know the code well, any comments?"

[PATCH] gc: use temporary file for editing crontab

[brian m. carlson]

While cron is specified by POSIX, there are a wide variety of
implementations in use.  On FreeBSD, the cron implementation requires a
file name argument: if the user wants to edit standard input, they must
specify "-".  However, this notation is not specified by POSIX, allowing
the possibility that making such a change may break other, less common
implementations.

Since POSIX tells us that cron must accept a file name argument, let's
solve this problem by specifying a temporary file instead.  This will
ensure that we work with the vast majority of implementations.

Reported-by: Renato Botelho <garga@FreeBSD.org>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
My apologies for the delay on this.  I forgot when I'd send a patch that
I had a wedding out of town.

 builtin/gc.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index eeff2b760e..168dbdb5d9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2065,6 +2065,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 	struct child_process crontab_edit = CHILD_PROCESS_INIT;
 	FILE *cron_list, *cron_in;
 	struct strbuf line = STRBUF_INIT;
+	struct tempfile *tmpedit;
 
 	get_schedule_cmd(&cmd, NULL);
 	strvec_split(&crontab_list.args, cmd);
@@ -2079,6 +2080,15 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 	/* Ignore exit code, as an empty crontab will return error. */
 	finish_command(&crontab_list);
 
+	tmpedit = mks_tempfile_t(".git_cron_edit_tmpXXXXXX");
+	if (!tmpedit)
+		return error(_("failed to create crontab temporary file"));
+	cron_in = fdopen_tempfile(tmpedit, "w");
+	if (!cron_in) {
+		result = error(_("failed to open temporary file"));
+		goto out;
+	}
+
 	/*
 	 * Read from the .lock file, filtering out the old
 	 * schedule while appending the new schedule.
@@ -2086,19 +2096,6 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 	cron_list = fdopen(fd, "r");
 	rewind(cron_list);
 
-	strvec_split(&crontab_edit.args, cmd);
-	crontab_edit.in = -1;
-	crontab_edit.git_cmd = 0;
-
-	if (start_command(&crontab_edit))
-		return error(_("failed to run 'crontab'; your system might not support 'cron'"));
-
-	cron_in = fdopen(crontab_edit.in, "w");
-	if (!cron_in) {
-		result = error(_("failed to open stdin of 'crontab'"));
-		goto done_editing;
-	}
-
 	while (!strbuf_getline_lf(&line, cron_list)) {
 		if (!in_old_region && !strcmp(line.buf, BEGIN_LINE))
 			in_old_region = 1;
@@ -2132,14 +2129,22 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 	}
 
 	fflush(cron_in);
-	fclose(cron_in);
-	close(crontab_edit.in);
 
-done_editing:
+	strvec_split(&crontab_edit.args, cmd);
+	strvec_push(&crontab_edit.args, get_tempfile_path(tmpedit));
+	crontab_edit.git_cmd = 0;
+
+	if (start_command(&crontab_edit)) {
+		result = error(_("failed to run 'crontab'; your system might not support 'cron'"));
+		goto out;
+	}
+
 	if (finish_command(&crontab_edit))
 		result = error(_("'crontab' died"));
 	else
 		fclose(cron_list);
+out:
+	close_tempfile_gently(tmpedit);
 	return result;
 }
 

Re: [PATCH] gc: use temporary file for editing crontab

[Johannes Schindelin]

Hi brian,

On Tue, 23 Aug 2022, brian m. carlson wrote:

> While cron is specified by POSIX, there are a wide variety of
> implementations in use.  On FreeBSD, the cron implementation requires a
> file name argument: if the user wants to edit standard input, they must
> specify "-".  However, this notation is not specified by POSIX, allowing
> the possibility that making such a change may break other, less common
> implementations.
>
> Since POSIX tells us that cron must accept a file name argument, let's
> solve this problem by specifying a temporary file instead.  This will
> ensure that we work with the vast majority of implementations.
>
> Reported-by: Renato Botelho <garga@FreeBSD.org>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>

Beautiful commit message. Thank you!

> diff --git a/builtin/gc.c b/builtin/gc.c
> index eeff2b760e..168dbdb5d9 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2065,6 +2065,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)
>  	struct child_process crontab_edit = CHILD_PROCESS_INIT;
>  	FILE *cron_list, *cron_in;
>  	struct strbuf line = STRBUF_INIT;
> +	struct tempfile *tmpedit;
>
>  	get_schedule_cmd(&cmd, NULL);
>  	strvec_split(&crontab_list.args, cmd);
> @@ -2079,6 +2080,15 @@ static int crontab_update_schedule(int run_maintenance, int fd)
>  	/* Ignore exit code, as an empty crontab will return error. */
>  	finish_command(&crontab_list);
>
> +	tmpedit = mks_tempfile_t(".git_cron_edit_tmpXXXXXX");
> +	if (!tmpedit)
> +		return error(_("failed to create crontab temporary file"));

It might make sense to use the same `goto out;` pattern here, to make it
easier to reason about the early exit even six years from now.

We do not even have to guard the `close_tempfile_gently()` behind an `if
(tempfile)` conditional because that function handles `NULL` parameters
gently.

> +	cron_in = fdopen_tempfile(tmpedit, "w");
> +	if (!cron_in) {
> +		result = error(_("failed to open temporary file"));
> +		goto out;
> +	}
> +
>  	/*
>  	 * Read from the .lock file, filtering out the old
>  	 * schedule while appending the new schedule.
> @@ -2086,19 +2096,6 @@ static int crontab_update_schedule(int run_maintenance, int fd)
>  	cron_list = fdopen(fd, "r");
>  	rewind(cron_list);
>
> -	strvec_split(&crontab_edit.args, cmd);
> -	crontab_edit.in = -1;
> -	crontab_edit.git_cmd = 0;
> -
> -	if (start_command(&crontab_edit))
> -		return error(_("failed to run 'crontab'; your system might not support 'cron'"));
> -
> -	cron_in = fdopen(crontab_edit.in, "w");
> -	if (!cron_in) {
> -		result = error(_("failed to open stdin of 'crontab'"));
> -		goto done_editing;
> -	}
> -
>  	while (!strbuf_getline_lf(&line, cron_list)) {
>  		if (!in_old_region && !strcmp(line.buf, BEGIN_LINE))
>  			in_old_region = 1;
> @@ -2132,14 +2129,22 @@ static int crontab_update_schedule(int run_maintenance, int fd)
>  	}
>
>  	fflush(cron_in);
> -	fclose(cron_in);
> -	close(crontab_edit.in);

This worries me a bit. I could imagine that keeping the file open and then
expecting a spawned process to read its stdin from that file won't work on
Windows.

In any case, I would consider it the correct thing to do to close
the temp file here. In other words, I would like to move the
`close_tempfile_gently()` call to this location.

>
> -done_editing:
> +	strvec_split(&crontab_edit.args, cmd);
> +	strvec_push(&crontab_edit.args, get_tempfile_path(tmpedit));
> +	crontab_edit.git_cmd = 0;
> +
> +	if (start_command(&crontab_edit)) {
> +		result = error(_("failed to run 'crontab'; your system might not support 'cron'"));
> +		goto out;
> +	}
> +
>  	if (finish_command(&crontab_edit))
>  		result = error(_("'crontab' died"));
>  	else
>  		fclose(cron_list);
> +out:
> +	close_tempfile_gently(tmpedit);

Here, I would like to call `delete_tempfile(&tmpedit);` instead. That way,
the memory is released correctly, the temporary file is deleted, and
everything is neatly cleaned up.

The way I read the code, `delete_tempfile(&tmpedit)` would return early if
`tmpedit == NULL`, and otherwise clean everything up and release the
memory, so there is no need to guard this call behind an `if (tmpedit)`
conditional.

Side note: I do notice that `delete_tempfile(&tmpedit)` seems to _not_
release memory when `tmpedit` is non-NULL when `tmpedit->active == 0`.
I consider this a bug in the `delete_tempfile()` code (in its `if
(!is_tempfile_active(tempfile))` clause, it should call
`deactivate_tempfile()` for non-NULL `tempfile`s and set `*tempfile_p =
NULL;`), but it is outside the scope of your patch to address that.

What do you think about my suggestions?

Thanks,
Dscho

>  	return result;
>  }
>
>

Re: [PATCH] gc: use temporary file for editing crontab

[Derrick Stolee]

On 8/23/2022 5:12 AM, Johannes Schindelin wrote:
> Hi brian,
> 
> On Tue, 23 Aug 2022, brian m. carlson wrote:

>> +	tmpedit = mks_tempfile_t(".git_cron_edit_tmpXXXXXX");
>> +	if (!tmpedit)
>> +		return error(_("failed to create crontab temporary file"));
> 
> It might make sense to use the same `goto out;` pattern here, to make it
> easier to reason about the early exit even six years from now.
> 
> We do not even have to guard the `close_tempfile_gently()` behind an `if
> (tempfile)` conditional because that function handles `NULL` parameters
> gently.

I don't think this is hard to reason about. It might mean that we
need to change this if block in the future to use 'goto out', if we
added another resource initialization before this one. That "future
need" is the only thing making me lean towards using the goto, but
we are just as likely to be in YAGNI territory here.
 
>> +	cron_in = fdopen_tempfile(tmpedit, "w");
>> +	if (!cron_in) {
>> +		result = error(_("failed to open temporary file"));
>> +		goto out;
>> +	}
>> +
>>  	/*
>>  	 * Read from the .lock file, filtering out the old
>>  	 * schedule while appending the new schedule.
>> @@ -2086,19 +2096,6 @@ static int crontab_update_schedule(int run_maintenance, int fd)
>>  	cron_list = fdopen(fd, "r");
>>  	rewind(cron_list);
>>
>> -	strvec_split(&crontab_edit.args, cmd);
>> -	crontab_edit.in = -1;
>> -	crontab_edit.git_cmd = 0;
>> -
>> -	if (start_command(&crontab_edit))
>> -		return error(_("failed to run 'crontab'; your system might not support 'cron'"));
>> -
>> -	cron_in = fdopen(crontab_edit.in, "w");
>> -	if (!cron_in) {
>> -		result = error(_("failed to open stdin of 'crontab'"));
>> -		goto done_editing;
>> -	}
>> -
>>  	while (!strbuf_getline_lf(&line, cron_list)) {
>>  		if (!in_old_region && !strcmp(line.buf, BEGIN_LINE))
>>  			in_old_region = 1;
>> @@ -2132,14 +2129,22 @@ static int crontab_update_schedule(int run_maintenance, int fd)
>>  	}
>>
>>  	fflush(cron_in);
>> -	fclose(cron_in);
>> -	close(crontab_edit.in);
> 
> This worries me a bit. I could imagine that keeping the file open and then
> expecting a spawned process to read its stdin from that file won't work on
> Windows.

This is focused only on the cron integration, which is not used on Windows,
so I'm not worried about that.

I was initially worried that we lost the fclose(cron_in), but of course it
is handled by the close_tempfile_gently() at the end.

> In any case, I would consider it the correct thing to do to close
> the temp file here. In other words, I would like to move the
> `close_tempfile_gently()` call to this location.
> 
>>
>> -done_editing:
>> +	strvec_split(&crontab_edit.args, cmd);
>> +	strvec_push(&crontab_edit.args, get_tempfile_path(tmpedit));
>> +	crontab_edit.git_cmd = 0;
>> +
>> +	if (start_command(&crontab_edit)) {
>> +		result = error(_("failed to run 'crontab'; your system might not support 'cron'"));
>> +		goto out;
>> +	}
>> +

Here's the crux of the matter: we are no longer using stdin but
instead passing an argument to point to a file with our desired
schedule. I tested that this worked on my machine, and I'm glad
this use is the POSIX standard.

There is something wrong with this patch: it needs to update
t/helper/test-crontab.c in order to pass t7900-maintenance.sh.

Something like this works for me:

--- >8 ---

diff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c
index e7c0137a477..29425430466 100644
--- a/t/helper/test-crontab.c
+++ b/t/helper/test-crontab.c
@@ -17,8 +17,8 @@ int cmd__crontab(int argc, const char **argv)
 		if (!from)
 			return 0;
 		to = stdout;
-	} else if (argc == 2) {
-		from = stdin;
+	} else if (argc == 3) {
+		from = fopen(argv[2], "r");
 		to = fopen(argv[1], "w");
 	} else
 		return error("unknown arguments");

--- >8 ---

>>  	if (finish_command(&crontab_edit))
>>  		result = error(_("'crontab' died"));
>>  	else
>>  		fclose(cron_list);
>> +out:
>> +	close_tempfile_gently(tmpedit);
> 
> Here, I would like to call `delete_tempfile(&tmpedit);` instead. That way,
> the memory is released correctly, the temporary file is deleted, and
> everything is neatly cleaned up.
> 
> The way I read the code, `delete_tempfile(&tmpedit)` would return early if
> `tmpedit == NULL`, and otherwise clean everything up and release the
> memory, so there is no need to guard this call behind an `if (tmpedit)`
> conditional.

While the memory release is nice, I also think it would be good to use
delete_tempfile() so the temporary file is deleted within this method,
not waiting until the end of the process to do that cleanup.

Thanks,
-Stolee

Re: [PATCH] gc: use temporary file for editing crontab

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2031 bytes --]

On 2022-08-23 at 17:06:03, Derrick Stolee wrote:
> On 8/23/2022 5:12 AM, Johannes Schindelin wrote:
> > Hi brian,
> > 
> > On Tue, 23 Aug 2022, brian m. carlson wrote:
> 
> >> +	tmpedit = mks_tempfile_t(".git_cron_edit_tmpXXXXXX");
> >> +	if (!tmpedit)
> >> +		return error(_("failed to create crontab temporary file"));
> > 
> > It might make sense to use the same `goto out;` pattern here, to make it
> > easier to reason about the early exit even six years from now.
> > 
> > We do not even have to guard the `close_tempfile_gently()` behind an `if
> > (tempfile)` conditional because that function handles `NULL` parameters
> > gently.

I can do that.  I'll need to make sure we initialize the pointer to NULL
first.

> This is focused only on the cron integration, which is not used on Windows,
> so I'm not worried about that.

Correct.  The only place this could go wrong is Cygwin, but I believe it
has the proper behaviour (and if not, lots of stuff will be broken).

> I was initially worried that we lost the fclose(cron_in), but of course it
> is handled by the close_tempfile_gently() at the end.

Yup.  I originally called fclose here and glibc screamed at me about a
double-free, so the fclose definitely should be removed.  I'll mention
this in the commit message as well.

> Here's the crux of the matter: we are no longer using stdin but
> instead passing an argument to point to a file with our desired
> schedule. I tested that this worked on my machine, and I'm glad
> this use is the POSIX standard.
> 
> There is something wrong with this patch: it needs to update
> t/helper/test-crontab.c in order to pass t7900-maintenance.sh.

Will fix.

> While the memory release is nice, I also think it would be good to use
> delete_tempfile() so the temporary file is deleted within this method,
> not waiting until the end of the process to do that cleanup.

Sounds good.  I'll include that in a v2.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH] gc: use temporary file for editing crontab

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

>> There is something wrong with this patch: it needs to update
>> t/helper/test-crontab.c in order to pass t7900-maintenance.sh.
>
> Will fix.
>
>> While the memory release is nice, I also think it would be good to use
>> delete_tempfile() so the temporary file is deleted within this method,
>> not waiting until the end of the process to do that cleanup.
>
> Sounds good.  I'll include that in a v2.

Thanks for following through the idea fell out of earlier
discussion.  I almost forgot about it, and it is very good to see it
written and reviewed quickly like this.

Thanks, all.

[PATCH v2] gc: use temporary file for editing crontab

[brian m. carlson]

While cron is specified by POSIX, there are a wide variety of
implementations in use.  On FreeBSD, the cron implementation requires a
file name argument: if the user wants to edit standard input, they must
specify "-".  However, this notation is not specified by POSIX, allowing
the possibility that making such a change may break other, less common
implementations.

Since POSIX tells us that cron must accept a file name argument, let's
solve this problem by specifying a temporary file instead.  This will
ensure that we work with the vast majority of implementations.

Note that because delete_tempfile closes the file for us, we should not
call fclose here on the handle, since doing so will introduce a double
free.

Reported-by: Renato Botelho <garga@FreeBSD.org>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
Changes from v1:

* Use `goto out;` in additional places.
* Fix broken test.
* Use `delete_tempfile`.
* Improve commit message to mention `fclose` rationale.

 builtin/gc.c            | 39 +++++++++++++++++++++++----------------
 t/helper/test-crontab.c |  4 ++--
 2 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index eeff2b760e..0d9e6dabef 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2065,6 +2065,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 	struct child_process crontab_edit = CHILD_PROCESS_INIT;
 	FILE *cron_list, *cron_in;
 	struct strbuf line = STRBUF_INIT;
+	struct tempfile *tmpedit = NULL;
 
 	get_schedule_cmd(&cmd, NULL);
 	strvec_split(&crontab_list.args, cmd);
@@ -2079,6 +2080,17 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 	/* Ignore exit code, as an empty crontab will return error. */
 	finish_command(&crontab_list);
 
+	tmpedit = mks_tempfile_t(".git_cron_edit_tmpXXXXXX");
+	if (!tmpedit) {
+		result = error(_("failed to create crontab temporary file"));
+		goto out;
+	}
+	cron_in = fdopen_tempfile(tmpedit, "w");
+	if (!cron_in) {
+		result = error(_("failed to open temporary file"));
+		goto out;
+	}
+
 	/*
 	 * Read from the .lock file, filtering out the old
 	 * schedule while appending the new schedule.
@@ -2086,19 +2098,6 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 	cron_list = fdopen(fd, "r");
 	rewind(cron_list);
 
-	strvec_split(&crontab_edit.args, cmd);
-	crontab_edit.in = -1;
-	crontab_edit.git_cmd = 0;
-
-	if (start_command(&crontab_edit))
-		return error(_("failed to run 'crontab'; your system might not support 'cron'"));
-
-	cron_in = fdopen(crontab_edit.in, "w");
-	if (!cron_in) {
-		result = error(_("failed to open stdin of 'crontab'"));
-		goto done_editing;
-	}
-
 	while (!strbuf_getline_lf(&line, cron_list)) {
 		if (!in_old_region && !strcmp(line.buf, BEGIN_LINE))
 			in_old_region = 1;
@@ -2132,14 +2131,22 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 	}
 
 	fflush(cron_in);
-	fclose(cron_in);
-	close(crontab_edit.in);
 
-done_editing:
+	strvec_split(&crontab_edit.args, cmd);
+	strvec_push(&crontab_edit.args, get_tempfile_path(tmpedit));
+	crontab_edit.git_cmd = 0;
+
+	if (start_command(&crontab_edit)) {
+		result = error(_("failed to run 'crontab'; your system might not support 'cron'"));
+		goto out;
+	}
+
 	if (finish_command(&crontab_edit))
 		result = error(_("'crontab' died"));
 	else
 		fclose(cron_list);
+out:
+	delete_tempfile(&tmpedit);
 	return result;
 }
 
diff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c
index e7c0137a47..2942543046 100644
--- a/t/helper/test-crontab.c
+++ b/t/helper/test-crontab.c
@@ -17,8 +17,8 @@ int cmd__crontab(int argc, const char **argv)
 		if (!from)
 			return 0;
 		to = stdout;
-	} else if (argc == 2) {
-		from = stdin;
+	} else if (argc == 3) {
+		from = fopen(argv[2], "r");
 		to = fopen(argv[1], "w");
 	} else
 		return error("unknown arguments");

Re: [PATCH v2] gc: use temporary file for editing crontab

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> While cron is specified by POSIX, there are a wide variety of
> implementations in use.

I would innsert: 

    "git maintenance" assumes that the "crontab" command can be fed
    from its standard input the new contents and the syntax to do so
    is not to have any filename argument, as POSIX describes.
    However,

here and downcase "O" in "On FreeBSD".

> On FreeBSD, the cron implementation
> requires a file name argument: if the user wants to edit standard
> input, they must specify "-".  

> However, this notation is not
> specified by POSIX, allowing the possibility that making such a
> change may break other, less common implementations.

And to avoid two However's in a row, perhaps

    Unfortunately, POSIX systems do not have to interpret "-" on the
    command line of crontab as a request to read from the standard
    input.  Blindly adding "-" on the command line would not work as
    a general solution.

> Since POSIX tells us that cron must accept a file name argument, let's
> solve this problem by specifying a temporary file instead.  This will
> ensure that we work with the vast majority of implementations.
>
> Note that because delete_tempfile closes the file for us, we should not
> call fclose here on the handle, since doing so will introduce a double
> free.
>
> Reported-by: Renato Botelho <garga@FreeBSD.org>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> Changes from v1:
>
> * Use `goto out;` in additional places.
> * Fix broken test.
> * Use `delete_tempfile`.
> * Improve commit message to mention `fclose` rationale.

Yup.  All nicely done.

>  builtin/gc.c            | 39 +++++++++++++++++++++++----------------
>  t/helper/test-crontab.c |  4 ++--
>  2 files changed, 25 insertions(+), 18 deletions(-)

Will queue.  Thanks.

Re: [PATCH v2] gc: use temporary file for editing crontab

[Renato Botelho]

On 28/08/22 18:41, brian m. carlson wrote:
> While cron is specified by POSIX, there are a wide variety of
> implementations in use.  On FreeBSD, the cron implementation requires a
> file name argument: if the user wants to edit standard input, they must
> specify "-".  However, this notation is not specified by POSIX, allowing
> the possibility that making such a change may break other, less common
> implementations.
> 
> Since POSIX tells us that cron must accept a file name argument, let's
> solve this problem by specifying a temporary file instead.  This will
> ensure that we work with the vast majority of implementations.
> 
> Note that because delete_tempfile closes the file for us, we should not
> call fclose here on the handle, since doing so will introduce a double
> free.
> 
> Reported-by: Renato Botelho <garga@FreeBSD.org>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>

brian,

I've tested and confirmed this fix works as expected.  This patch is now 
applied on FreeBSD ports tree.

Thanks!
-- 
Renato Botelho

Re: [PATCH v2] gc: use temporary file for editing crontab

[Derrick Stolee]

On 8/28/2022 5:41 PM, brian m. carlson wrote:
> While cron is specified by POSIX, there are a wide variety of
> implementations in use.  On FreeBSD, the cron implementation requires a
> file name argument: if the user wants to edit standard input, they must
> specify "-".  However, this notation is not specified by POSIX, allowing
> the possibility that making such a change may break other, less common
> implementations.
> 
> Since POSIX tells us that cron must accept a file name argument, let's
> solve this problem by specifying a temporary file instead.  This will
> ensure that we work with the vast majority of implementations.
> 
> Note that because delete_tempfile closes the file for us, we should not
> call fclose here on the handle, since doing so will introduce a double
> free.
> 
> Reported-by: Renato Botelho <garga@FreeBSD.org>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> Changes from v1:
> 
> * Use `goto out;` in additional places.
> * Fix broken test.
> * Use `delete_tempfile`.
> * Improve commit message to mention `fclose` rationale.

Thanks for this update. It resolves all of my concerns from v1.

Thanks,
-Stolee

[PATCH] test-crontab: minor memory and error handling fixes

[Jeff King]

On Sun, Aug 28, 2022 at 09:41:43PM +0000, brian m. carlson wrote:

> diff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c
> index e7c0137a47..2942543046 100644
> --- a/t/helper/test-crontab.c
> +++ b/t/helper/test-crontab.c
> @@ -17,8 +17,8 @@ int cmd__crontab(int argc, const char **argv)
>  		if (!from)
>  			return 0;
>  		to = stdout;
> -	} else if (argc == 2) {
> -		from = stdin;
> +	} else if (argc == 3) {
> +		from = fopen(argv[2], "r");
>  		to = fopen(argv[1], "w");
>  	} else
>  		return error("unknown arguments");

After this commit we know that argc must be 3, so that makes the "else"
in the cleanup section dead code:

  if (argc == 3)
	fclose(from);
  else
	fclose(to);

While fixing that, I noticed a ton of other small problems, so I just
lumped them all together (which I think is OK given the relative
insignificance of this program). I do have to wonder if this really
could be replaced by a call to "cp". ;)

-- >8 --
Subject: [PATCH] test-crontab: minor memory and error handling fixes

Since ee69e7884e (gc: use temporary file for editing crontab,
2022-08-28), we now insist that "argc == 3" (and otherwise return an
error). Coverity notes that this causes some dead code:

    if (argc == 3)
          fclose(from);
    else
          fclose(to);

as we will never trigger the else. This also causes a memory leak, since
we'll never close "to".

Now that all paths require 2 arguments, we can just reorganize the
function to check argc up front, and tweak the cleanup to do the right
thing for all cases.

While we're here, we can also notice some minor problems:

  - we return a negative int via error() from what is essentially a
    main() function; we should return a positive non-zero value for
    error. Or better yet, we can just use usage(), which gives a better
    message.

  - while writing the usage message, we can note the one in the comment
    was made out of date by ee69e7884e. But it also had a typo already,
    calling the subcommand "cron" and not "crontab"

  - we didn't check for an error from fopen(), meaning we would segfault
    if the to-be-read file was missing. We can use xfopen() to catch
    this.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-crontab.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c
index 2942543046..e6c1b1e22b 100644
--- a/t/helper/test-crontab.c
+++ b/t/helper/test-crontab.c
@@ -2,33 +2,34 @@
 #include "cache.h"
 
 /*
- * Usage: test-tool cron <file> [-l]
+ * Usage: test-tool crontab <file> -l|<input>
  *
  * If -l is specified, then write the contents of <file> to stdout.
- * Otherwise, write from stdin into <file>.
+ * Otherwise, copy the contents of <input> into <file>.
  */
 int cmd__crontab(int argc, const char **argv)
 {
 	int a;
 	FILE *from, *to;
 
-	if (argc == 3 && !strcmp(argv[2], "-l")) {
+	if (argc != 3)
+		usage("test-tool crontab <file> -l|<input>");
+
+	if (!strcmp(argv[2], "-l")) {
 		from = fopen(argv[1], "r");
 		if (!from)
 			return 0;
 		to = stdout;
-	} else if (argc == 3) {
-		from = fopen(argv[2], "r");
-		to = fopen(argv[1], "w");
-	} else
-		return error("unknown arguments");
+	} else {
+		from = xfopen(argv[2], "r");
+		to = xfopen(argv[1], "w");
+	}
 
 	while ((a = fgetc(from)) != EOF)
 		fputc(a, to);
 
-	if (argc == 3)
-		fclose(from);
-	else
+	fclose(from);
+	if (to != stdout)
 		fclose(to);
 
 	return 0;
-- 
2.37.3.1051.g85dc4064ac