| Message ID | 20220823010120.25388-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gc: use temporary file for editing crontab | expand |
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; > } > >
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
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" <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.
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; }
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(-)