{"thread":{"id":"58297","subject":"git maintenance broken on FreeBSD","startedAt":"2022-08-12T14:00:53Z","lastAt":"2022-08-30T20:41:04Z","messageCount":19,"participants":["Renato Botelho","Đoàn Trần Công Danh","Todd Zullinger","Junio C Hamano","brian m. carlson","Derrick Stolee","Johannes Schindelin","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"461116","messageId":"226317ba-a78f-216c-764c-52f4e393bd35@FreeBSD.org","threadId":"58297","inReplyTo":null,"subject":"git maintenance broken on FreeBSD","fromName":"Renato Botelho","fromEmail":"garga@freebsd.org","sentAt":"2022-08-12T13:51:03Z","receivedAt":"2022-08-12T14:00:53Z","isPatch":false,"sender":{"key":"garga@freebsd.org","avatar":"https://gravatar.com/avatar/695c68fb2f0629998c430204a7212aec683a3f87ec5eeaae236fd2376e790bfc?d=mp&s=160"},"body":"As reported at [1], git maintenance is not working on FreeBSD. I didn't \nfind the time to dig into it but it seems like it's calling crontab \nusing parameters not supported on FreeBSD.\n\n[1] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=260746\n-- \nRenato Botelho\n"},{"id":"461117","messageId":"YvZnQFVMZZmz9TIX@danh.dev","threadId":"58297","inReplyTo":"226317ba-a78f-216c-764c-52f4e393bd35@FreeBSD.org","subject":"Re: git maintenance broken on FreeBSD","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2022-08-12T14:44:16Z","receivedAt":"2022-08-12T14:44:22Z","isPatch":false,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2022-08-12 10:51:03-0300, Renato Botelho wrote:\n> As reported at [1], git maintenance is not working on FreeBSD. I didn't\n> find the time to dig into it but it seems like it's calling crontab using\n> parameters not supported on FreeBSD.\n> \n> [1] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=260746\n\nIt seems like FreeBSD's cron is vixie-cron which requires \npassed to crontab(1).\n\n The crontab command conforms to IEEE Std 1003.2 (“POSIX.2”) with the\n exception that the dangerous variant of calling crontab without a file\n name in the first form of the command is not allowed by this\n implementation. The pseudo-filename ‘-’ must be specified to read from\n standard input. The new command syntax differs from previous versions of\n Vixie Cron, as well as from the classic SVR3 syntax.\n\nI think other crontab implementation also accept \"-\" as filename for stdin.\nAt least cronie, fcron, dcron, and busybox's crontab both supports \"-\" as stdin.\n\nI think this patch can fix FreeBSD's problem:\n\n---- 8< -----\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex eeff2b760e..45d908def3 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2087,6 +2087,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \trewind(cron_list);\n \n \tstrvec_split(&crontab_edit.args, cmd);\n+\tstrvec_push(&crontab_edit.args, \"-\");\n \tcrontab_edit.in = -1;\n \tcrontab_edit.git_cmd = 0;\n \n---- 8< ---------\n\n\n-- \nDanh\n"},{"id":"461162","messageId":"YvcdskzUkocUv/d7@pobox.com","threadId":"58297","inReplyTo":"YvZnQFVMZZmz9TIX@danh.dev","subject":"Re: git maintenance broken on FreeBSD","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2022-08-13T03:42:42Z","receivedAt":"2022-08-13T03:46:37Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Đoàn Trần Công Danh wrote:\n> On 2022-08-12 10:51:03-0300, Renato Botelho wrote:\n>> As reported at [1], git maintenance is not working on FreeBSD. I didn't\n>> find the time to dig into it but it seems like it's calling crontab using\n>> parameters not supported on FreeBSD.\n>> \n>> [1] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=260746\n> \n> It seems like FreeBSD's cron is vixie-cron which requires \n> passed to crontab(1).\n> \n> The crontab command conforms to IEEE Std 1003.2 (“POSIX.2”) with the\n> exception that the dangerous variant of calling crontab without a file\n> name in the first form of the command is not allowed by this\n> implementation. The pseudo-filename ‘-’ must be specified to read from\n> standard input. The new command syntax differs from previous versions of\n> Vixie Cron, as well as from the classic SVR3 syntax.\n> \n> I think other crontab implementation also accept \"-\" as filename for stdin.\n> At least cronie, fcron, dcron, and busybox's crontab both supports \"-\" as stdin.\n\nA similar issue was noted in Fedora with cronie shortly\nafter the git maintenance command was released:\n\n https://bugzilla.redhat.com/show_bug.cgi?id=1939930#c1\n\nI noted that a patch just like the one below would suffice,\nbut I was concerned that it wouldn't be welcome here because\nthe behavior of crontab was specified by POSIX (even though\nit's very unfriendly and, apparently, supported by fewer and\nfewer implementations).\n\nIf a change like this is made, aren't we trading one group\nof broken users for another? It would fix users of newer\nsystems at the expense of those on older systems, I would\nsuspect.\n\n> I think this patch can fix FreeBSD's problem:\n> \n> ---- 8< -----\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index eeff2b760e..45d908def3 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -2087,6 +2087,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n> \trewind(cron_list);\n> \n> \tstrvec_split(&crontab_edit.args, cmd);\n> +\tstrvec_push(&crontab_edit.args, \"-\");\n> \tcrontab_edit.in = -1;\n> \tcrontab_edit.git_cmd = 0;\n> \n> ---- 8< ---------\n\nIn the end, cronie adjusted it's behavior, which was similar\nto that of the newer vixie-cron, in 8b0241f (Partially\nrevert the behavior of crontab command without arguments,\n2021-03-17)¹. It now behaves as required by POSIX if stdin\nis not a TTY. That seems like a reasonable compromise and\nperhaps vixie-cron would be willing to do the same?\n\n¹ https://github.com/cronie-crond/cronie/commit/8b0241f\n\n-- \nTodd\n"},{"id":"461163","messageId":"xmqqczd4ag8f.fsf@gitster.g","threadId":"58297","inReplyTo":"YvcdskzUkocUv/d7@pobox.com","subject":"Re: git maintenance broken on FreeBSD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-13T05:02:08Z","receivedAt":"2022-08-13T05:02:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Todd Zullinger writes:\n\n> If a change like this is made, aren't we trading one group\n> of broken users for another? It would fix users of newer\n> systems at the expense of those on older systems, I would\n> suspect.\n\nThanks for raising this. The description of POSIX \"crontab\"\ncommand, cf.\n\nhttps://pubs.opengroup.org/onlinepubs/9699919799/utilities/crontab.html \n\ntalks about optional \"file\", but it is explicit that it has to be a\nreal file, i.e.\n\n file\n The pathname of a file that contains specifications, in the\n format defined in the INPUT FILES section, for crontab\n entries.\n\nI would suspect that implementations may treat it as a sign to read\nthe standard input, but I do not think that is what the above\nspecifies. For example, description of \"file\" argument of another\ncommand, \"diff\", cf.\n\nhttps://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html\n\nexplicitly calls out that \"-\" stands for the standard input, i.e.\n\n diff [-c|-e|-f|-u|-C n|-U n] [-br] file1 file2\n\n file1, file2\n A pathname of a file to be compared. If either the file1 or\n file2 operand is '-', the standard input shall be used in\n its place.\n\nSo, it is fairly clear that \"crontab\" wants a real file. Somebody's\nPOSIX compliant \"crontab\" can be fed \"-\", attempt to read from a\nfile with such a name, and legitimately fail. And on such a system,\nthe proposed patch causes a regression.\n\n> In the end, cronie adjusted it's behavior, which was similar\n> to that of the newer vixie-cron, in 8b0241f (Partially\n> revert the behavior of crontab command without arguments,\n> 2021-03-17)¹. It now behaves as required by POSIX if stdin\n> is not a TTY. That seems like a reasonable compromise and\n> perhaps vixie-cron would be willing to do the same?\n\nIt indeed is a pragmatic solution to use isatty() as a hint. \n"},{"id":"461173","messageId":"YvfFUuuydtYeuvRx@danh.dev","threadId":"58297","inReplyTo":"xmqqczd4ag8f.fsf@gitster.g","subject":"Re: git maintenance broken on FreeBSD","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2022-08-13T15:37:54Z","receivedAt":"2022-08-13T15:38:02Z","isPatch":false,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2022-08-12 22:02:08-0700, Junio C Hamano wrote:\n> Todd Zullinger writes:\n> \n> > If a change like this is made, aren't we trading one group\n> > of broken users for another? It would fix users of newer\n> > systems at the expense of those on older systems, I would\n> > suspect.\n> \n> So, it is fairly clear that \"crontab\" wants a real file. Somebody's\n> POSIX compliant \"crontab\" can be fed \"-\", attempt to read from a\n> file with such a name, and legitimately fail. And on such a system,\n> the proposed patch causes a regression.\n\nThen, we are getting back to point #0, we don't have universally way\nto specify stdin as input file for crontab(1) and \"crontab -e\" is\noptional.\n\nPerhaps, FreeBSD needs to carry this patch downstream; or\nwe will invent new preprocessor, let's say CRONTAB_DASH_IS_STDIN\nwhich is defined in FreeBSD, and another config, let's say\ncrontab.dashIsStdin (in order to allow users swap their default\ncron) which is default to 1 if CRONTAB_DASH_IS_STDIN is defined,\nand 0 otherwise. So, everyone will be happy. Thought?\n\n-- \nDanh\n"},{"id":"461176","messageId":"xmqqsfm08382.fsf@gitster.g","threadId":"58297","inReplyTo":"YvfFUuuydtYeuvRx@danh.dev","subject":"Re: git maintenance broken on FreeBSD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-13T17:26:05Z","receivedAt":"2022-08-13T17:26:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Đoàn Trần Công Danh writes:\n\n> Then, we are getting back to point #0, we don't have universally way\n> to specify stdin as input file for crontab(1) and \"crontab -e\" is\n> optional.\n>\n> Perhaps, FreeBSD needs to carry this patch downstream; or\n> we will invent new preprocessor, let's say CRONTAB_DASH_IS_STDIN\n> which is defined in FreeBSD,\n\nDoes FreeBSD offer choices of cron implementations other than Vixie,\njust like some Linux distributions? If somebody on a non-FreeBSD\nplatform happens to choose to use Vixie, then they would presumably\nhave the same problem, so a compile-time switch, whose default is\nhardcoded based on the target platform, would not work very well.\nThe default will be wrong for some users, and users can later choose\nto switch between different cron implementations.\n\nConfiguration knob can be used as a workaround, but in this case, I\nam not sure if it is worth doing. What's the downside of securely\nopening a temporary file and write whatever we are currently piping\nto a spawned \"crontab\" command and then giving the path to that\ntemporary file to the \"crontab\" command? Wouldn't that give us the\nmaximal portability without that much code, no?\n\nI think this is all Derrick's code from 2fec604f (maintenance: add\nstart/stop subcommands, 2020-09-11), so let's add him to the\ndiscussion.\n"},{"id":"461177","messageId":"Yvfg7WwL8oCdxqzQ@tapette.crustytoothpaste.net","threadId":"58297","inReplyTo":"xmqqsfm08382.fsf@gitster.g","subject":"Re: git maintenance broken on FreeBSD","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-08-13T17:35:41Z","receivedAt":"2022-08-13T17:35:55Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-08-13 at 17:26:05, Junio C Hamano wrote:\n> Does FreeBSD offer choices of cron implementations other than Vixie,\n> just like some Linux distributions? If somebody on a non-FreeBSD\n> platform happens to choose to use Vixie, then they would presumably\n> have the same problem, so a compile-time switch, whose default is\n> hardcoded based on the target platform, would not work very well.\n> The default will be wrong for some users, and users can later choose\n> to switch between different cron implementations.\n\nI'm using Debian unstable, and I'm using Vixie cron. I believe that's\nthe default implementation. However, I could also well use cronie,\nsince that's available in Debian as well. So, yeah, I think this is a\nthing to consider.\n\n> Configuration knob can be used as a workaround, but in this case, I\n> am not sure if it is worth doing. What's the downside of securely\n> opening a temporary file and write whatever we are currently piping\n> to a spawned \"crontab\" command and then giving the path to that\n> temporary file to the \"crontab\" command? Wouldn't that give us the\n> maximal portability without that much code, no?\n\nI think we should try to provide an option which works across at least\nthe versions on a particular OS. The temporary file seems like a nice,\nportable option, so I think we should just do that unless there's some\npractical objection.\n\nIf Derrick doesn't get to it this next week, I can send a patch.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"461226","messageId":"1dd29f43-1a8e-eb69-3320-7f5140a0e18e@github.com","threadId":"58297","inReplyTo":"Yvfg7WwL8oCdxqzQ@tapette.crustytoothpaste.net","subject":"Re: git maintenance broken on FreeBSD","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-15T13:22:40Z","receivedAt":"2022-08-15T13:22:45Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/13/2022 1:35 PM, brian m. carlson wrote:\n> On 2022-08-13 at 17:26:05, Junio C Hamano wrote:\n>> Does FreeBSD offer choices of cron implementations other than Vixie,\n>> just like some Linux distributions? If somebody on a non-FreeBSD\n>> platform happens to choose to use Vixie, then they would presumably\n>> have the same problem, so a compile-time switch, whose default is\n>> hardcoded based on the target platform, would not work very well.\n>> The default will be wrong for some users, and users can later choose\n>> to switch between different cron implementations.\n> \n> I'm using Debian unstable, and I'm using Vixie cron. I believe that's\n> the default implementation. However, I could also well use cronie,\n> since that's available in Debian as well. So, yeah, I think this is a\n> thing to consider.\n> \n>> Configuration knob can be used as a workaround, but in this case, I\n>> am not sure if it is worth doing. What's the downside of securely\n>> opening a temporary file and write whatever we are currently piping\n>> to a spawned \"crontab\" command and then giving the path to that\n>> temporary file to the \"crontab\" command? Wouldn't that give us the\n>> maximal portability without that much code, no?\n> \n> I think we should try to provide an option which works across at least\n> the versions on a particular OS. The temporary file seems like a nice,\n> portable option, so I think we should just do that unless there's some\n> practical objection.\n> \n> If Derrick doesn't get to it this next week, I can send a patch.\n\nI agree that the tempfile approach makes the most sense in terms of\nwhat we can do within the Git codebase.\n\nI won't be able to get to this change this week, so I'd be happy to\nreview one of yours, brian. Be careful to test manually when making\nthis change, because our tests don't actually interact with the system's\ncrontab and instead verify the interaction using replacement commands.\n\nThanks,\n-Stolee\n"},{"id":"461248","messageId":"xmqqy1vp32v1.fsf@gitster.g","threadId":"58297","inReplyTo":"1dd29f43-1a8e-eb69-3320-7f5140a0e18e@github.com","subject":"Re: git maintenance broken on FreeBSD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-15T16:09:38Z","receivedAt":"2022-08-15T16:09:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> I agree that the tempfile approach makes the most sense in terms of\n> what we can do within the Git codebase.\n>\n> I won't be able to get to this change this week, so I'd be happy to\n> review one of yours, brian. Be careful to test manually when making\n> this change, because our tests don't actually interact with the system's\n> crontab and instead verify the interaction using replacement commands.\n\nThanks. I didn't mean \"it's your code, go fix it\". It was \"you\nare one of the folks who know the code well, any comments?\"\n\n"},{"id":"461822","messageId":"20220823010120.25388-1-sandals@crustytoothpaste.net","threadId":"58297","inReplyTo":"1dd29f43-1a8e-eb69-3320-7f5140a0e18e@github.com","subject":"[PATCH] gc: use temporary file for editing crontab","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-08-23T01:01:20Z","receivedAt":"2022-08-23T01:01:50Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"While cron is specified by POSIX, there are a wide variety of\nimplementations in use. On FreeBSD, the cron implementation requires a\nfile name argument: if the user wants to edit standard input, they must\nspecify \"-\". However, this notation is not specified by POSIX, allowing\nthe possibility that making such a change may break other, less common\nimplementations.\n\nSince POSIX tells us that cron must accept a file name argument, let's\nsolve this problem by specifying a temporary file instead. This will\nensure that we work with the vast majority of implementations.\n\nReported-by: Renato Botelho \nSigned-off-by: brian m. carlson \n---\nMy apologies for the delay on this. I forgot when I'd send a patch that\nI had a wedding out of town.\n\n builtin/gc.c | 37 +++++++++++++++++++++----------------\n 1 file changed, 21 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex eeff2b760e..168dbdb5d9 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2065,6 +2065,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \tstruct child_process crontab_edit = CHILD_PROCESS_INIT;\n \tFILE *cron_list, *cron_in;\n \tstruct strbuf line = STRBUF_INIT;\n+\tstruct tempfile *tmpedit;\n \n \tget_schedule_cmd(&cmd, NULL);\n \tstrvec_split(&crontab_list.args, cmd);\n@@ -2079,6 +2080,15 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \t/* Ignore exit code, as an empty crontab will return error. */\n \tfinish_command(&crontab_list);\n \n+\ttmpedit = mks_tempfile_t(\".git_cron_edit_tmpXXXXXX\");\n+\tif (!tmpedit)\n+\t\treturn error(_(\"failed to create crontab temporary file\"));\n+\tcron_in = fdopen_tempfile(tmpedit, \"w\");\n+\tif (!cron_in) {\n+\t\tresult = error(_(\"failed to open temporary file\"));\n+\t\tgoto out;\n+\t}\n+\n \t/*\n \t * Read from the .lock file, filtering out the old\n \t * schedule while appending the new schedule.\n@@ -2086,19 +2096,6 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \tcron_list = fdopen(fd, \"r\");\n \trewind(cron_list);\n \n-\tstrvec_split(&crontab_edit.args, cmd);\n-\tcrontab_edit.in = -1;\n-\tcrontab_edit.git_cmd = 0;\n-\n-\tif (start_command(&crontab_edit))\n-\t\treturn error(_(\"failed to run 'crontab'; your system might not support 'cron'\"));\n-\n-\tcron_in = fdopen(crontab_edit.in, \"w\");\n-\tif (!cron_in) {\n-\t\tresult = error(_(\"failed to open stdin of 'crontab'\"));\n-\t\tgoto done_editing;\n-\t}\n-\n \twhile (!strbuf_getline_lf(&line, cron_list)) {\n \t\tif (!in_old_region && !strcmp(line.buf, BEGIN_LINE))\n \t\t\tin_old_region = 1;\n@@ -2132,14 +2129,22 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \t}\n \n \tfflush(cron_in);\n-\tfclose(cron_in);\n-\tclose(crontab_edit.in);\n \n-done_editing:\n+\tstrvec_split(&crontab_edit.args, cmd);\n+\tstrvec_push(&crontab_edit.args, get_tempfile_path(tmpedit));\n+\tcrontab_edit.git_cmd = 0;\n+\n+\tif (start_command(&crontab_edit)) {\n+\t\tresult = error(_(\"failed to run 'crontab'; your system might not support 'cron'\"));\n+\t\tgoto out;\n+\t}\n+\n \tif (finish_command(&crontab_edit))\n \t\tresult = error(_(\"'crontab' died\"));\n \telse\n \t\tfclose(cron_list);\n+out:\n+\tclose_tempfile_gently(tmpedit);\n \treturn result;\n }\n \n"},{"id":"461843","messageId":"6428252p-ssrn-7qs7-9p26-5so10r96s3os@tzk.qr","threadId":"58297","inReplyTo":"20220823010120.25388-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] gc: use temporary file for editing crontab","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-23T09:12:21Z","receivedAt":"2022-08-23T10:53:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi brian,\n\nOn Tue, 23 Aug 2022, brian m. carlson wrote:\n\n> While cron is specified by POSIX, there are a wide variety of\n> implementations in use. On FreeBSD, the cron implementation requires a\n> file name argument: if the user wants to edit standard input, they must\n> specify \"-\". However, this notation is not specified by POSIX, allowing\n> the possibility that making such a change may break other, less common\n> implementations.\n>\n> Since POSIX tells us that cron must accept a file name argument, let's\n> solve this problem by specifying a temporary file instead. This will\n> ensure that we work with the vast majority of implementations.\n>\n> Reported-by: Renato Botelho \n> Signed-off-by: brian m. carlson \n\nBeautiful commit message. Thank you!\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index eeff2b760e..168dbdb5d9 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -2065,6 +2065,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n> \tstruct child_process crontab_edit = CHILD_PROCESS_INIT;\n> \tFILE *cron_list, *cron_in;\n> \tstruct strbuf line = STRBUF_INIT;\n> +\tstruct tempfile *tmpedit;\n>\n> \tget_schedule_cmd(&cmd, NULL);\n> \tstrvec_split(&crontab_list.args, cmd);\n> @@ -2079,6 +2080,15 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n> \t/* Ignore exit code, as an empty crontab will return error. */\n> \tfinish_command(&crontab_list);\n>\n> +\ttmpedit = mks_tempfile_t(\".git_cron_edit_tmpXXXXXX\");\n> +\tif (!tmpedit)\n> +\t\treturn error(_(\"failed to create crontab temporary file\"));\n\nIt might make sense to use the same `goto out;` pattern here, to make it\neasier to reason about the early exit even six years from now.\n\nWe do not even have to guard the `close_tempfile_gently()` behind an `if\n(tempfile)` conditional because that function handles `NULL` parameters\ngently.\n\n> +\tcron_in = fdopen_tempfile(tmpedit, \"w\");\n> +\tif (!cron_in) {\n> +\t\tresult = error(_(\"failed to open temporary file\"));\n> +\t\tgoto out;\n> +\t}\n> +\n> \t/*\n> \t * Read from the .lock file, filtering out the old\n> \t * schedule while appending the new schedule.\n> @@ -2086,19 +2096,6 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n> \tcron_list = fdopen(fd, \"r\");\n> \trewind(cron_list);\n>\n> -\tstrvec_split(&crontab_edit.args, cmd);\n> -\tcrontab_edit.in = -1;\n> -\tcrontab_edit.git_cmd = 0;\n> -\n> -\tif (start_command(&crontab_edit))\n> -\t\treturn error(_(\"failed to run 'crontab'; your system might not support 'cron'\"));\n> -\n> -\tcron_in = fdopen(crontab_edit.in, \"w\");\n> -\tif (!cron_in) {\n> -\t\tresult = error(_(\"failed to open stdin of 'crontab'\"));\n> -\t\tgoto done_editing;\n> -\t}\n> -\n> \twhile (!strbuf_getline_lf(&line, cron_list)) {\n> \t\tif (!in_old_region && !strcmp(line.buf, BEGIN_LINE))\n> \t\t\tin_old_region = 1;\n> @@ -2132,14 +2129,22 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n> \t}\n>\n> \tfflush(cron_in);\n> -\tfclose(cron_in);\n> -\tclose(crontab_edit.in);\n\nThis worries me a bit. I could imagine that keeping the file open and then\nexpecting a spawned process to read its stdin from that file won't work on\nWindows.\n\nIn any case, I would consider it the correct thing to do to close\nthe temp file here. In other words, I would like to move the\n`close_tempfile_gently()` call to this location.\n\n>\n> -done_editing:\n> +\tstrvec_split(&crontab_edit.args, cmd);\n> +\tstrvec_push(&crontab_edit.args, get_tempfile_path(tmpedit));\n> +\tcrontab_edit.git_cmd = 0;\n> +\n> +\tif (start_command(&crontab_edit)) {\n> +\t\tresult = error(_(\"failed to run 'crontab'; your system might not support 'cron'\"));\n> +\t\tgoto out;\n> +\t}\n> +\n> \tif (finish_command(&crontab_edit))\n> \t\tresult = error(_(\"'crontab' died\"));\n> \telse\n> \t\tfclose(cron_list);\n> +out:\n> +\tclose_tempfile_gently(tmpedit);\n\nHere, I would like to call `delete_tempfile(&tmpedit);` instead. That way,\nthe memory is released correctly, the temporary file is deleted, and\neverything is neatly cleaned up.\n\nThe way I read the code, `delete_tempfile(&tmpedit)` would return early if\n`tmpedit == NULL`, and otherwise clean everything up and release the\nmemory, so there is no need to guard this call behind an `if (tmpedit)`\nconditional.\n\nSide note: I do notice that `delete_tempfile(&tmpedit)` seems to _not_\nrelease memory when `tmpedit` is non-NULL when `tmpedit->active == 0`.\nI consider this a bug in the `delete_tempfile()` code (in its `if\n(!is_tempfile_active(tempfile))` clause, it should call\n`deactivate_tempfile()` for non-NULL `tempfile`s and set `*tempfile_p =\nNULL;`), but it is outside the scope of your patch to address that.\n\nWhat do you think about my suggestions?\n\nThanks,\nDscho\n\n> \treturn result;\n> }\n>\n>\n"},{"id":"461864","messageId":"9e737b4b-4a17-09d5-6452-4ca5eef3d9da@github.com","threadId":"58297","inReplyTo":"6428252p-ssrn-7qs7-9p26-5so10r96s3os@tzk.qr","subject":"Re: [PATCH] gc: use temporary file for editing crontab","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-23T17:06:03Z","receivedAt":"2022-08-23T18:42:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/23/2022 5:12 AM, Johannes Schindelin wrote:\n> Hi brian,\n> \n> On Tue, 23 Aug 2022, brian m. carlson wrote:\n\n>> +\ttmpedit = mks_tempfile_t(\".git_cron_edit_tmpXXXXXX\");\n>> +\tif (!tmpedit)\n>> +\t\treturn error(_(\"failed to create crontab temporary file\"));\n> \n> It might make sense to use the same `goto out;` pattern here, to make it\n> easier to reason about the early exit even six years from now.\n> \n> We do not even have to guard the `close_tempfile_gently()` behind an `if\n> (tempfile)` conditional because that function handles `NULL` parameters\n> gently.\n\nI don't think this is hard to reason about. It might mean that we\nneed to change this if block in the future to use 'goto out', if we\nadded another resource initialization before this one. That \"future\nneed\" is the only thing making me lean towards using the goto, but\nwe are just as likely to be in YAGNI territory here.\n \n>> +\tcron_in = fdopen_tempfile(tmpedit, \"w\");\n>> +\tif (!cron_in) {\n>> +\t\tresult = error(_(\"failed to open temporary file\"));\n>> +\t\tgoto out;\n>> +\t}\n>> +\n>> \t/*\n>> \t * Read from the .lock file, filtering out the old\n>> \t * schedule while appending the new schedule.\n>> @@ -2086,19 +2096,6 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n>> \tcron_list = fdopen(fd, \"r\");\n>> \trewind(cron_list);\n>>\n>> -\tstrvec_split(&crontab_edit.args, cmd);\n>> -\tcrontab_edit.in = -1;\n>> -\tcrontab_edit.git_cmd = 0;\n>> -\n>> -\tif (start_command(&crontab_edit))\n>> -\t\treturn error(_(\"failed to run 'crontab'; your system might not support 'cron'\"));\n>> -\n>> -\tcron_in = fdopen(crontab_edit.in, \"w\");\n>> -\tif (!cron_in) {\n>> -\t\tresult = error(_(\"failed to open stdin of 'crontab'\"));\n>> -\t\tgoto done_editing;\n>> -\t}\n>> -\n>> \twhile (!strbuf_getline_lf(&line, cron_list)) {\n>> \t\tif (!in_old_region && !strcmp(line.buf, BEGIN_LINE))\n>> \t\t\tin_old_region = 1;\n>> @@ -2132,14 +2129,22 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n>> \t}\n>>\n>> \tfflush(cron_in);\n>> -\tfclose(cron_in);\n>> -\tclose(crontab_edit.in);\n> \n> This worries me a bit. I could imagine that keeping the file open and then\n> expecting a spawned process to read its stdin from that file won't work on\n> Windows.\n\nThis is focused only on the cron integration, which is not used on Windows,\nso I'm not worried about that.\n\nI was initially worried that we lost the fclose(cron_in), but of course it\nis handled by the close_tempfile_gently() at the end.\n\n> In any case, I would consider it the correct thing to do to close\n> the temp file here. In other words, I would like to move the\n> `close_tempfile_gently()` call to this location.\n> \n>>\n>> -done_editing:\n>> +\tstrvec_split(&crontab_edit.args, cmd);\n>> +\tstrvec_push(&crontab_edit.args, get_tempfile_path(tmpedit));\n>> +\tcrontab_edit.git_cmd = 0;\n>> +\n>> +\tif (start_command(&crontab_edit)) {\n>> +\t\tresult = error(_(\"failed to run 'crontab'; your system might not support 'cron'\"));\n>> +\t\tgoto out;\n>> +\t}\n>> +\n\nHere's the crux of the matter: we are no longer using stdin but\ninstead passing an argument to point to a file with our desired\nschedule. I tested that this worked on my machine, and I'm glad\nthis use is the POSIX standard.\n\nThere is something wrong with this patch: it needs to update\nt/helper/test-crontab.c in order to pass t7900-maintenance.sh.\n\nSomething like this works for me:\n\n--- >8 ---\n\ndiff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c\nindex e7c0137a477..29425430466 100644\n--- a/t/helper/test-crontab.c\n+++ b/t/helper/test-crontab.c\n@@ -17,8 +17,8 @@ int cmd__crontab(int argc, const char **argv)\n \t\tif (!from)\n \t\t\treturn 0;\n \t\tto = stdout;\n-\t} else if (argc == 2) {\n-\t\tfrom = stdin;\n+\t} else if (argc == 3) {\n+\t\tfrom = fopen(argv[2], \"r\");\n \t\tto = fopen(argv[1], \"w\");\n \t} else\n \t\treturn error(\"unknown arguments\");\n\n--- >8 ---\n\n>> \tif (finish_command(&crontab_edit))\n>> \t\tresult = error(_(\"'crontab' died\"));\n>> \telse\n>> \t\tfclose(cron_list);\n>> +out:\n>> +\tclose_tempfile_gently(tmpedit);\n> \n> Here, I would like to call `delete_tempfile(&tmpedit);` instead. That way,\n> the memory is released correctly, the temporary file is deleted, and\n> everything is neatly cleaned up.\n> \n> The way I read the code, `delete_tempfile(&tmpedit)` would return early if\n> `tmpedit == NULL`, and otherwise clean everything up and release the\n> memory, so there is no need to guard this call behind an `if (tmpedit)`\n> conditional.\n\nWhile the memory release is nice, I also think it would be good to use\ndelete_tempfile() so the temporary file is deleted within this method,\nnot waiting until the end of the process to do that cleanup.\n\nThanks,\n-Stolee\n"},{"id":"461880","messageId":"YwVDcO/V+zx2iy4I@tapette.crustytoothpaste.net","threadId":"58297","inReplyTo":"9e737b4b-4a17-09d5-6452-4ca5eef3d9da@github.com","subject":"Re: [PATCH] gc: use temporary file for editing crontab","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-08-23T21:15:28Z","receivedAt":"2022-08-23T21:15:53Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-08-23 at 17:06:03, Derrick Stolee wrote:\n> On 8/23/2022 5:12 AM, Johannes Schindelin wrote:\n> > Hi brian,\n> > \n> > On Tue, 23 Aug 2022, brian m. carlson wrote:\n> \n> >> +\ttmpedit = mks_tempfile_t(\".git_cron_edit_tmpXXXXXX\");\n> >> +\tif (!tmpedit)\n> >> +\t\treturn error(_(\"failed to create crontab temporary file\"));\n> > \n> > It might make sense to use the same `goto out;` pattern here, to make it\n> > easier to reason about the early exit even six years from now.\n> > \n> > We do not even have to guard the `close_tempfile_gently()` behind an `if\n> > (tempfile)` conditional because that function handles `NULL` parameters\n> > gently.\n\nI can do that. I'll need to make sure we initialize the pointer to NULL\nfirst.\n\n> This is focused only on the cron integration, which is not used on Windows,\n> so I'm not worried about that.\n\nCorrect. The only place this could go wrong is Cygwin, but I believe it\nhas the proper behaviour (and if not, lots of stuff will be broken).\n\n> I was initially worried that we lost the fclose(cron_in), but of course it\n> is handled by the close_tempfile_gently() at the end.\n\nYup. I originally called fclose here and glibc screamed at me about a\ndouble-free, so the fclose definitely should be removed. I'll mention\nthis in the commit message as well.\n\n> Here's the crux of the matter: we are no longer using stdin but\n> instead passing an argument to point to a file with our desired\n> schedule. I tested that this worked on my machine, and I'm glad\n> this use is the POSIX standard.\n> \n> There is something wrong with this patch: it needs to update\n> t/helper/test-crontab.c in order to pass t7900-maintenance.sh.\n\nWill fix.\n\n> While the memory release is nice, I also think it would be good to use\n> delete_tempfile() so the temporary file is deleted within this method,\n> not waiting until the end of the process to do that cleanup.\n\nSounds good. I'll include that in a v2.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"461900","messageId":"xmqqpmgp8w2x.fsf@gitster.g","threadId":"58297","inReplyTo":"YwVDcO/V+zx2iy4I@tapette.crustytoothpaste.net","subject":"Re: [PATCH] gc: use temporary file for editing crontab","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-24T16:06:46Z","receivedAt":"2022-08-24T16:06:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n>> There is something wrong with this patch: it needs to update\n>> t/helper/test-crontab.c in order to pass t7900-maintenance.sh.\n>\n> Will fix.\n>\n>> While the memory release is nice, I also think it would be good to use\n>> delete_tempfile() so the temporary file is deleted within this method,\n>> not waiting until the end of the process to do that cleanup.\n>\n> Sounds good. I'll include that in a v2.\n\nThanks for following through the idea fell out of earlier\ndiscussion. I almost forgot about it, and it is very good to see it\nwritten and reviewed quickly like this.\n\nThanks, all.\n"},{"id":"462064","messageId":"20220828214143.754759-1-sandals@crustytoothpaste.net","threadId":"58297","inReplyTo":"20220823010120.25388-1-sandals@crustytoothpaste.net","subject":"[PATCH v2] gc: use temporary file for editing crontab","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-08-28T21:41:43Z","receivedAt":"2022-08-28T21:42:26Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"While cron is specified by POSIX, there are a wide variety of\nimplementations in use. On FreeBSD, the cron implementation requires a\nfile name argument: if the user wants to edit standard input, they must\nspecify \"-\". However, this notation is not specified by POSIX, allowing\nthe possibility that making such a change may break other, less common\nimplementations.\n\nSince POSIX tells us that cron must accept a file name argument, let's\nsolve this problem by specifying a temporary file instead. This will\nensure that we work with the vast majority of implementations.\n\nNote that because delete_tempfile closes the file for us, we should not\ncall fclose here on the handle, since doing so will introduce a double\nfree.\n\nReported-by: Renato Botelho \nSigned-off-by: brian m. carlson \n---\nChanges from v1:\n\n* Use `goto out;` in additional places.\n* Fix broken test.\n* Use `delete_tempfile`.\n* Improve commit message to mention `fclose` rationale.\n\n builtin/gc.c | 39 +++++++++++++++++++++++----------------\n t/helper/test-crontab.c | 4 ++--\n 2 files changed, 25 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex eeff2b760e..0d9e6dabef 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2065,6 +2065,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \tstruct child_process crontab_edit = CHILD_PROCESS_INIT;\n \tFILE *cron_list, *cron_in;\n \tstruct strbuf line = STRBUF_INIT;\n+\tstruct tempfile *tmpedit = NULL;\n \n \tget_schedule_cmd(&cmd, NULL);\n \tstrvec_split(&crontab_list.args, cmd);\n@@ -2079,6 +2080,17 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \t/* Ignore exit code, as an empty crontab will return error. */\n \tfinish_command(&crontab_list);\n \n+\ttmpedit = mks_tempfile_t(\".git_cron_edit_tmpXXXXXX\");\n+\tif (!tmpedit) {\n+\t\tresult = error(_(\"failed to create crontab temporary file\"));\n+\t\tgoto out;\n+\t}\n+\tcron_in = fdopen_tempfile(tmpedit, \"w\");\n+\tif (!cron_in) {\n+\t\tresult = error(_(\"failed to open temporary file\"));\n+\t\tgoto out;\n+\t}\n+\n \t/*\n \t * Read from the .lock file, filtering out the old\n \t * schedule while appending the new schedule.\n@@ -2086,19 +2098,6 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \tcron_list = fdopen(fd, \"r\");\n \trewind(cron_list);\n \n-\tstrvec_split(&crontab_edit.args, cmd);\n-\tcrontab_edit.in = -1;\n-\tcrontab_edit.git_cmd = 0;\n-\n-\tif (start_command(&crontab_edit))\n-\t\treturn error(_(\"failed to run 'crontab'; your system might not support 'cron'\"));\n-\n-\tcron_in = fdopen(crontab_edit.in, \"w\");\n-\tif (!cron_in) {\n-\t\tresult = error(_(\"failed to open stdin of 'crontab'\"));\n-\t\tgoto done_editing;\n-\t}\n-\n \twhile (!strbuf_getline_lf(&line, cron_list)) {\n \t\tif (!in_old_region && !strcmp(line.buf, BEGIN_LINE))\n \t\t\tin_old_region = 1;\n@@ -2132,14 +2131,22 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \t}\n \n \tfflush(cron_in);\n-\tfclose(cron_in);\n-\tclose(crontab_edit.in);\n \n-done_editing:\n+\tstrvec_split(&crontab_edit.args, cmd);\n+\tstrvec_push(&crontab_edit.args, get_tempfile_path(tmpedit));\n+\tcrontab_edit.git_cmd = 0;\n+\n+\tif (start_command(&crontab_edit)) {\n+\t\tresult = error(_(\"failed to run 'crontab'; your system might not support 'cron'\"));\n+\t\tgoto out;\n+\t}\n+\n \tif (finish_command(&crontab_edit))\n \t\tresult = error(_(\"'crontab' died\"));\n \telse\n \t\tfclose(cron_list);\n+out:\n+\tdelete_tempfile(&tmpedit);\n \treturn result;\n }\n \ndiff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c\nindex e7c0137a47..2942543046 100644\n--- a/t/helper/test-crontab.c\n+++ b/t/helper/test-crontab.c\n@@ -17,8 +17,8 @@ int cmd__crontab(int argc, const char **argv)\n \t\tif (!from)\n \t\t\treturn 0;\n \t\tto = stdout;\n-\t} else if (argc == 2) {\n-\t\tfrom = stdin;\n+\t} else if (argc == 3) {\n+\t\tfrom = fopen(argv[2], \"r\");\n \t\tto = fopen(argv[1], \"w\");\n \t} else\n \t\treturn error(\"unknown arguments\");\n"},{"id":"462070","messageId":"xmqqmtbnr1hc.fsf@gitster.g","threadId":"58297","inReplyTo":"20220828214143.754759-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2] gc: use temporary file for editing crontab","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-29T06:46:23Z","receivedAt":"2022-08-29T06:46:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> While cron is specified by POSIX, there are a wide variety of\n> implementations in use.\n\nI would innsert: \n\n \"git maintenance\" assumes that the \"crontab\" command can be fed\n from its standard input the new contents and the syntax to do so\n is not to have any filename argument, as POSIX describes.\n However,\n\nhere and downcase \"O\" in \"On FreeBSD\".\n\n> On FreeBSD, the cron implementation\n> requires a file name argument: if the user wants to edit standard\n> input, they must specify \"-\". \n\n> However, this notation is not\n> specified by POSIX, allowing the possibility that making such a\n> change may break other, less common implementations.\n\nAnd to avoid two However's in a row, perhaps\n\n Unfortunately, POSIX systems do not have to interpret \"-\" on the\n command line of crontab as a request to read from the standard\n input. Blindly adding \"-\" on the command line would not work as\n a general solution.\n\n> Since POSIX tells us that cron must accept a file name argument, let's\n> solve this problem by specifying a temporary file instead. This will\n> ensure that we work with the vast majority of implementations.\n>\n> Note that because delete_tempfile closes the file for us, we should not\n> call fclose here on the handle, since doing so will introduce a double\n> free.\n>\n> Reported-by: Renato Botelho \n> Signed-off-by: brian m. carlson \n> ---\n> Changes from v1:\n>\n> * Use `goto out;` in additional places.\n> * Fix broken test.\n> * Use `delete_tempfile`.\n> * Improve commit message to mention `fclose` rationale.\n\nYup. All nicely done.\n\n> builtin/gc.c | 39 +++++++++++++++++++++++----------------\n> t/helper/test-crontab.c | 4 ++--\n> 2 files changed, 25 insertions(+), 18 deletions(-)\n\nWill queue. Thanks.\n"},{"id":"462079","messageId":"d2b63f68-4463-63cd-065f-0902fc8be4e3@FreeBSD.org","threadId":"58297","inReplyTo":"20220828214143.754759-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2] gc: use temporary file for editing crontab","fromName":"Renato Botelho","fromEmail":"garga@freebsd.org","sentAt":"2022-08-29T10:52:46Z","receivedAt":"2022-08-29T10:53:05Z","isPatch":true,"sender":{"key":"garga@freebsd.org","avatar":"https://gravatar.com/avatar/695c68fb2f0629998c430204a7212aec683a3f87ec5eeaae236fd2376e790bfc?d=mp&s=160"},"body":"On 28/08/22 18:41, brian m. carlson wrote:\n> While cron is specified by POSIX, there are a wide variety of\n> implementations in use. On FreeBSD, the cron implementation requires a\n> file name argument: if the user wants to edit standard input, they must\n> specify \"-\". However, this notation is not specified by POSIX, allowing\n> the possibility that making such a change may break other, less common\n> implementations.\n> \n> Since POSIX tells us that cron must accept a file name argument, let's\n> solve this problem by specifying a temporary file instead. This will\n> ensure that we work with the vast majority of implementations.\n> \n> Note that because delete_tempfile closes the file for us, we should not\n> call fclose here on the handle, since doing so will introduce a double\n> free.\n> \n> Reported-by: Renato Botelho \n> Signed-off-by: brian m. carlson \n\nbrian,\n\nI've tested and confirmed this fix works as expected. This patch is now \napplied on FreeBSD ports tree.\n\nThanks!\n-- \nRenato Botelho\n\n"},{"id":"462188","messageId":"e851e646-1daa-a77c-5d27-fd7c4445c2ce@github.com","threadId":"58297","inReplyTo":"20220828214143.754759-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2] gc: use temporary file for editing crontab","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-30T13:27:23Z","receivedAt":"2022-08-30T13:28:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/28/2022 5:41 PM, brian m. carlson wrote:\n> While cron is specified by POSIX, there are a wide variety of\n> implementations in use. On FreeBSD, the cron implementation requires a\n> file name argument: if the user wants to edit standard input, they must\n> specify \"-\". However, this notation is not specified by POSIX, allowing\n> the possibility that making such a change may break other, less common\n> implementations.\n> \n> Since POSIX tells us that cron must accept a file name argument, let's\n> solve this problem by specifying a temporary file instead. This will\n> ensure that we work with the vast majority of implementations.\n> \n> Note that because delete_tempfile closes the file for us, we should not\n> call fclose here on the handle, since doing so will introduce a double\n> free.\n> \n> Reported-by: Renato Botelho \n> Signed-off-by: brian m. carlson \n> ---\n> Changes from v1:\n> \n> * Use `goto out;` in additional places.\n> * Fix broken test.\n> * Use `delete_tempfile`.\n> * Improve commit message to mention `fclose` rationale.\n\nThanks for this update. It resolves all of my concerns from v1.\n\nThanks,\n-Stolee\n"},{"id":"462238","messageId":"Yw512HXz/SV50ckc@coredump.intra.peff.net","threadId":"58297","inReplyTo":"20220828214143.754759-1-sandals@crustytoothpaste.net","subject":"[PATCH] test-crontab: minor memory and error handling fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-30T20:40:56Z","receivedAt":"2022-08-30T20:41:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 28, 2022 at 09:41:43PM +0000, brian m. carlson wrote:\n\n> diff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c\n> index e7c0137a47..2942543046 100644\n> --- a/t/helper/test-crontab.c\n> +++ b/t/helper/test-crontab.c\n> @@ -17,8 +17,8 @@ int cmd__crontab(int argc, const char **argv)\n> \t\tif (!from)\n> \t\t\treturn 0;\n> \t\tto = stdout;\n> -\t} else if (argc == 2) {\n> -\t\tfrom = stdin;\n> +\t} else if (argc == 3) {\n> +\t\tfrom = fopen(argv[2], \"r\");\n> \t\tto = fopen(argv[1], \"w\");\n> \t} else\n> \t\treturn error(\"unknown arguments\");\n\nAfter this commit we know that argc must be 3, so that makes the \"else\"\nin the cleanup section dead code:\n\n if (argc == 3)\n\tfclose(from);\n else\n\tfclose(to);\n\nWhile fixing that, I noticed a ton of other small problems, so I just\nlumped them all together (which I think is OK given the relative\ninsignificance of this program). I do have to wonder if this really\ncould be replaced by a call to \"cp\". ;)\n\n-- >8 --\nSubject: [PATCH] test-crontab: minor memory and error handling fixes\n\nSince ee69e7884e (gc: use temporary file for editing crontab,\n2022-08-28), we now insist that \"argc == 3\" (and otherwise return an\nerror). Coverity notes that this causes some dead code:\n\n if (argc == 3)\n fclose(from);\n else\n fclose(to);\n\nas we will never trigger the else. This also causes a memory leak, since\nwe'll never close \"to\".\n\nNow that all paths require 2 arguments, we can just reorganize the\nfunction to check argc up front, and tweak the cleanup to do the right\nthing for all cases.\n\nWhile we're here, we can also notice some minor problems:\n\n - we return a negative int via error() from what is essentially a\n main() function; we should return a positive non-zero value for\n error. Or better yet, we can just use usage(), which gives a better\n message.\n\n - while writing the usage message, we can note the one in the comment\n was made out of date by ee69e7884e. But it also had a typo already,\n calling the subcommand \"cron\" and not \"crontab\"\n\n - we didn't check for an error from fopen(), meaning we would segfault\n if the to-be-read file was missing. We can use xfopen() to catch\n this.\n\nSigned-off-by: Jeff King \n---\n t/helper/test-crontab.c | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c\nindex 2942543046..e6c1b1e22b 100644\n--- a/t/helper/test-crontab.c\n+++ b/t/helper/test-crontab.c\n@@ -2,33 +2,34 @@\n #include \"cache.h\"\n \n /*\n- * Usage: test-tool cron [-l]\n+ * Usage: test-tool crontab -l|\n *\n * If -l is specified, then write the contents of to stdout.\n- * Otherwise, write from stdin into .\n+ * Otherwise, copy the contents of into .\n */\n int cmd__crontab(int argc, const char **argv)\n {\n \tint a;\n \tFILE *from, *to;\n \n-\tif (argc == 3 && !strcmp(argv[2], \"-l\")) {\n+\tif (argc != 3)\n+\t\tusage(\"test-tool crontab -l|\");\n+\n+\tif (!strcmp(argv[2], \"-l\")) {\n \t\tfrom = fopen(argv[1], \"r\");\n \t\tif (!from)\n \t\t\treturn 0;\n \t\tto = stdout;\n-\t} else if (argc == 3) {\n-\t\tfrom = fopen(argv[2], \"r\");\n-\t\tto = fopen(argv[1], \"w\");\n-\t} else\n-\t\treturn error(\"unknown arguments\");\n+\t} else {\n+\t\tfrom = xfopen(argv[2], \"r\");\n+\t\tto = xfopen(argv[1], \"w\");\n+\t}\n \n \twhile ((a = fgetc(from)) != EOF)\n \t\tfputc(a, to);\n \n-\tif (argc == 3)\n-\t\tfclose(from);\n-\telse\n+\tfclose(from);\n+\tif (to != stdout)\n \t\tfclose(to);\n \n \treturn 0;\n-- \n2.37.3.1051.g85dc4064ac\n\n"}]}