* git maintenance broken on FreeBSD @ 2022-08-12 13:51 Renato Botelho 2022-08-12 14:44 ` Đoàn Trần Công Danh 0 siblings, 1 reply; 19+ messages in thread From: Renato Botelho @ 2022-08-12 13:51 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: git maintenance broken on FreeBSD 2022-08-12 13:51 git maintenance broken on FreeBSD Renato Botelho @ 2022-08-12 14:44 ` Đoàn Trần Công Danh 2022-08-13 3:42 ` Todd Zullinger 0 siblings, 1 reply; 19+ messages in thread From: Đoàn Trần Công Danh @ 2022-08-12 14:44 UTC (permalink / raw) To: Renato Botelho; +Cc: git 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: git maintenance broken on FreeBSD 2022-08-12 14:44 ` Đoàn Trần Công Danh @ 2022-08-13 3:42 ` Todd Zullinger 2022-08-13 5:02 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Todd Zullinger @ 2022-08-13 3:42 UTC (permalink / raw) To: Đoàn Trần Công Danh; +Cc: Renato Botelho, git Đ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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: git maintenance broken on FreeBSD 2022-08-13 3:42 ` Todd Zullinger @ 2022-08-13 5:02 ` Junio C Hamano 2022-08-13 15:37 ` Đoàn Trần Công Danh 0 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2022-08-13 5:02 UTC (permalink / raw) To: Todd Zullinger Cc: Đoàn Trần Công Danh, Renato Botelho, git 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: git maintenance broken on FreeBSD 2022-08-13 5:02 ` Junio C Hamano @ 2022-08-13 15:37 ` Đoàn Trần Công Danh 2022-08-13 17:26 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Đoàn Trần Công Danh @ 2022-08-13 15:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: Todd Zullinger, Renato Botelho, git 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: git maintenance broken on FreeBSD 2022-08-13 15:37 ` Đoàn Trần Công Danh @ 2022-08-13 17:26 ` Junio C Hamano 2022-08-13 17:35 ` brian m. carlson 0 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2022-08-13 17:26 UTC (permalink / raw) To: Đoàn Trần Công Danh, Derrick Stolee Cc: Todd Zullinger, Renato Botelho, git Đ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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: git maintenance broken on FreeBSD 2022-08-13 17:26 ` Junio C Hamano @ 2022-08-13 17:35 ` brian m. carlson 2022-08-15 13:22 ` Derrick Stolee 0 siblings, 1 reply; 19+ messages in thread From: brian m. carlson @ 2022-08-13 17:35 UTC (permalink / raw) To: Junio C Hamano Cc: Đoàn Trần Công Danh, Derrick Stolee, Todd Zullinger, Renato Botelho, git [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: git maintenance broken on FreeBSD 2022-08-13 17:35 ` brian m. carlson @ 2022-08-15 13:22 ` Derrick Stolee 2022-08-15 16:09 ` Junio C Hamano 2022-08-23 1:01 ` [PATCH] gc: use temporary file for editing crontab brian m. carlson 0 siblings, 2 replies; 19+ messages in thread From: Derrick Stolee @ 2022-08-15 13:22 UTC (permalink / raw) To: brian m. carlson, Junio C Hamano, Đoàn Trần Công Danh, Todd Zullinger, Renato Botelho, git 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: git maintenance broken on FreeBSD 2022-08-15 13:22 ` Derrick Stolee @ 2022-08-15 16:09 ` Junio C Hamano 2022-08-23 1:01 ` [PATCH] gc: use temporary file for editing crontab brian m. carlson 1 sibling, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2022-08-15 16:09 UTC (permalink / raw) To: Derrick Stolee Cc: brian m. carlson, Đoàn Trần Công Danh, Todd Zullinger, Renato Botelho, git 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?" ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] gc: use temporary file for editing crontab 2022-08-15 13:22 ` Derrick Stolee 2022-08-15 16:09 ` Junio C Hamano @ 2022-08-23 1:01 ` brian m. carlson 2022-08-23 9:12 ` Johannes Schindelin 2022-08-28 21:41 ` [PATCH v2] " brian m. carlson 1 sibling, 2 replies; 19+ messages in thread From: brian m. carlson @ 2022-08-23 1:01 UTC (permalink / raw) To: git Cc: Junio C Hamano, Derrick Stolee, Renato Botelho, Todd Zullinger, Đoàn Trần Công Danh 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; } ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] gc: use temporary file for editing crontab 2022-08-23 1:01 ` [PATCH] gc: use temporary file for editing crontab brian m. carlson @ 2022-08-23 9:12 ` Johannes Schindelin 2022-08-23 17:06 ` Derrick Stolee 2022-08-28 21:41 ` [PATCH v2] " brian m. carlson 1 sibling, 1 reply; 19+ messages in thread From: Johannes Schindelin @ 2022-08-23 9:12 UTC (permalink / raw) To: brian m. carlson Cc: git, Junio C Hamano, Derrick Stolee, Renato Botelho, Todd Zullinger, Đoàn Trần Công Danh 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; > } > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] gc: use temporary file for editing crontab 2022-08-23 9:12 ` Johannes Schindelin @ 2022-08-23 17:06 ` Derrick Stolee 2022-08-23 21:15 ` brian m. carlson 0 siblings, 1 reply; 19+ messages in thread From: Derrick Stolee @ 2022-08-23 17:06 UTC (permalink / raw) To: Johannes Schindelin, brian m. carlson Cc: git, Junio C Hamano, Derrick Stolee, Renato Botelho, Todd Zullinger, Đoàn Trần Công Danh 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] gc: use temporary file for editing crontab 2022-08-23 17:06 ` Derrick Stolee @ 2022-08-23 21:15 ` brian m. carlson 2022-08-24 16:06 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: brian m. carlson @ 2022-08-23 21:15 UTC (permalink / raw) To: Derrick Stolee Cc: Johannes Schindelin, git, Junio C Hamano, Derrick Stolee, Renato Botelho, Todd Zullinger, Đoàn Trần Công Danh [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] gc: use temporary file for editing crontab 2022-08-23 21:15 ` brian m. carlson @ 2022-08-24 16:06 ` Junio C Hamano 0 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2022-08-24 16:06 UTC (permalink / raw) To: brian m. carlson Cc: Derrick Stolee, Johannes Schindelin, git, Derrick Stolee, Renato Botelho, Todd Zullinger, Đoàn Trần Công Danh "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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] gc: use temporary file for editing crontab 2022-08-23 1:01 ` [PATCH] gc: use temporary file for editing crontab brian m. carlson 2022-08-23 9:12 ` Johannes Schindelin @ 2022-08-28 21:41 ` brian m. carlson 2022-08-29 6:46 ` Junio C Hamano ` (3 more replies) 1 sibling, 4 replies; 19+ messages in thread From: brian m. carlson @ 2022-08-28 21:41 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Johannes Schindelin, Derrick Stolee, Renato Botelho 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"); ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] gc: use temporary file for editing crontab 2022-08-28 21:41 ` [PATCH v2] " brian m. carlson @ 2022-08-29 6:46 ` Junio C Hamano 2022-08-29 10:52 ` Renato Botelho ` (2 subsequent siblings) 3 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2022-08-29 6:46 UTC (permalink / raw) To: brian m. carlson; +Cc: git, Johannes Schindelin, Derrick Stolee, Renato Botelho "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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] gc: use temporary file for editing crontab 2022-08-28 21:41 ` [PATCH v2] " brian m. carlson 2022-08-29 6:46 ` Junio C Hamano @ 2022-08-29 10:52 ` Renato Botelho 2022-08-30 13:27 ` Derrick Stolee 2022-08-30 20:40 ` [PATCH] test-crontab: minor memory and error handling fixes Jeff King 3 siblings, 0 replies; 19+ messages in thread From: Renato Botelho @ 2022-08-29 10:52 UTC (permalink / raw) To: brian m. carlson, git; +Cc: Junio C Hamano, Johannes Schindelin, Derrick Stolee 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] gc: use temporary file for editing crontab 2022-08-28 21:41 ` [PATCH v2] " brian m. carlson 2022-08-29 6:46 ` Junio C Hamano 2022-08-29 10:52 ` Renato Botelho @ 2022-08-30 13:27 ` Derrick Stolee 2022-08-30 20:40 ` [PATCH] test-crontab: minor memory and error handling fixes Jeff King 3 siblings, 0 replies; 19+ messages in thread From: Derrick Stolee @ 2022-08-30 13:27 UTC (permalink / raw) To: brian m. carlson, git Cc: Junio C Hamano, Johannes Schindelin, Derrick Stolee, Renato Botelho 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] test-crontab: minor memory and error handling fixes 2022-08-28 21:41 ` [PATCH v2] " brian m. carlson ` (2 preceding siblings ...) 2022-08-30 13:27 ` Derrick Stolee @ 2022-08-30 20:40 ` Jeff King 3 siblings, 0 replies; 19+ messages in thread From: Jeff King @ 2022-08-30 20:40 UTC (permalink / raw) To: brian m. carlson Cc: git, Junio C Hamano, Johannes Schindelin, Derrick Stolee, Renato Botelho 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-08-30 20:41 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-12 13:51 git maintenance broken on FreeBSD Renato Botelho 2022-08-12 14:44 ` Đoàn Trần Công Danh 2022-08-13 3:42 ` Todd Zullinger 2022-08-13 5:02 ` Junio C Hamano 2022-08-13 15:37 ` Đoàn Trần Công Danh 2022-08-13 17:26 ` Junio C Hamano 2022-08-13 17:35 ` brian m. carlson 2022-08-15 13:22 ` Derrick Stolee 2022-08-15 16:09 ` Junio C Hamano 2022-08-23 1:01 ` [PATCH] gc: use temporary file for editing crontab brian m. carlson 2022-08-23 9:12 ` Johannes Schindelin 2022-08-23 17:06 ` Derrick Stolee 2022-08-23 21:15 ` brian m. carlson 2022-08-24 16:06 ` Junio C Hamano 2022-08-28 21:41 ` [PATCH v2] " brian m. carlson 2022-08-29 6:46 ` Junio C Hamano 2022-08-29 10:52 ` Renato Botelho 2022-08-30 13:27 ` Derrick Stolee 2022-08-30 20:40 ` [PATCH] test-crontab: minor memory and error handling fixes Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).