| Message ID | 20220828214143.754759-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ee69e7884e0cae3d2feabd5fcce8d3dfb44dda3a |
| Headers | show |
| Series | [v2] gc: use temporary file for editing crontab | expand |
"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.
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!
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
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");
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(-)