crontab: don't reject non-string-like shell commands by benknoble · Pull Request #47 · yo8192/fcron

benknoble · 2026-03-15T15:05:15Z

The use of get_string to parse and assign shell commands to jobs
requires that shell commands starting with quotes also terminate with
quotes, like

* * * * * "/root/my script"

This improperly rejects commands generated by Git maintenance, which
look like (just one example)

32 1-23 * * * "/usr/libexec/git-core/git" --exec-path="/usr/libexec/git-core" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=hourly

Simpler commands with internal quotes are of course unaffected.

But why are we interpreting shell syntax anyway? That requires a fairly
complex parser.

Instead, do what the documentation says and take the rest of the
(logical) line as the shell command to execute, leaving it to the
shell's parser to interpret the commands. In case we want to later add
some validation of our own, hide this new parsing behind the abstraction
"get_command". Keep removing blanks, since they should be ignored by the
shell anyway.

While at it, add (some) test coverage for the various read_* functions
that parse job lines; these tests are only interested in the shell
parsing for now. Note that the compilation recipe for these tests is a
bit different than other recipes, since it builds based on fcrontab
(where fileconf.o is used). We also have to expose the functions to test
via the header.

Fix: #46

Notes: On my (amd64) system, cmocka-2.0.2 was required (current in testing on Gentoo). It might be worth writing that down somewhere, and I think it's a similar issue to https://gitlab.com/cmocka/cmocka/-/work_items/38 in that it was fixed but I can't compile use the Gentoo-stable 1.1.8. I've reported that to Gentoo, so not something you need to worry about here. Just wanted to leave a breadcrumb.

yo8192

Thanks for raising this and preparing this!

The use of get_string to parse and assign shell commands to jobs requires that shell commands starting with quotes also terminate with quotes, like * * * * * "/root/my script" This improperly rejects commands generated by Git maintenance, which look like (just one example) 32 1-23 * * * "/usr/libexec/git-core/git" --exec-path="/usr/libexec/git-core" -c credential.interactive=false -c core.askPass=true for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=hourly Simpler commands with internal quotes are of course unaffected. But why are we interpreting shell syntax anyway? That requires a fairly complex parser. Instead, do what the documentation says and take the rest of the (logical) line as the shell command to execute, leaving it to the shell's parser to interpret the commands. In case we want to later add some validation of our own, hide this new parsing behind the abstraction "get_command". Keep removing blanks, since they should be ignored by the shell anyway. While at it, add (some) test coverage for the various read_* functions that parse job lines; these tests are only interested in the shell parsing for now. Note that the compilation recipe for these tests is a bit different than other recipes, since it builds based on fcrontab (where fileconf.o is used). We also have to expose the functions to test via the header. Fix: yo8192#46

benknoble · 2026-03-16T22:04:56Z

range-diff (git range-diff origin/master benknoble/bk/fix-46 bk/fix-46): add tests and keep removing blanks

1:  1c3ebf7 ! 1:  4c399bb crontab: don't reject non-string-like shell commands
    @@ Commit message
         (logical) line as the shell command to execute, leaving it to the
         shell's parser to interpret the commands. In case we want to later add
         some validation of our own, hide this new parsing behind the abstraction
    -    "get_command". This does, by the by, also mean we don't strip trailing
    -    blanks from the command. But the shell will do so anyway unless the
    -    blanks are protected (such as via a quote), in which case they're no
    -    longer trailing.
    +    "get_command". Keep removing blanks, since they should be ignored by the
    +    shell anyway.
    +
    +    While at it, add (some) test coverage for the various read_* functions
    +    that parse job lines; these tests are only interested in the shell
    +    parsing for now. Note that the compilation recipe for these tests is a
    +    bit different than other recipes, since it builds based on fcrontab
    +    (where fileconf.o is used). We also have to expose the functions to test
    +    via the header.
     
         Fix: https://github.com/yo8192/fcron/issues/46
     
    @@ Commit message
      ## Notes ##
         On my (amd64) system, cmocka-2.0.2 was required (current in testing on Gentoo)
     
    + ## Makefile.in ##
    +@@ Makefile.in: OBJCONV       := convert-fcrontab.o cl.o subs.o mem.o save.o log.o u_list.o env_
    + OBJSIG        := fcronsighup.o subs.o mem.o log.o allow.o fcronconf.o mail.o filesubs.o
    + HEADERSALL    := config.h $(SRCDIR)/global.h $(SRCDIR)/cl.h $(SRCDIR)/log.h $(SRCDIR)/subs.h $(SRCDIR)/mem.h $(SRCDIR)/save.h $(SRCDIR)/option.h $(SRCDIR)/dyncom.h $(SRCDIR)/mail.h
    + TESTS_DIR     := tests
    +-TESTS         := $(addprefix ${TESTS_DIR}/,fcrondyn_svr_test exe_list_test mail_test)
    ++TESTS         := $(addprefix ${TESTS_DIR}/,fcrondyn_svr_test exe_list_test mail_test parse_job_test)
    + 
    + # this is a regular expression :
    + # do not ci automaticaly generated files and doc (done by doc's Makefile)
    +@@ Makefile.in: $(TESTS_DIR)/mail_test: $(OBJSD_NO_MAIN) $(TESTS_DIR)/fcron.test.o $(TESTS_DIR)/
    + 	$(CC) $(CFLAGS) -o $@  $^ $(LIBS) -lcmocka -Wno-implicit-function-declaration
    + 	$@
    + 
    ++$(TESTS_DIR)/parse_job_test: $(filter-out fcrontab.o,$(OBJSTAB)) $(TESTS_DIR)/fcrontab.test.o $(TESTS_DIR)/parse_job_test.o
    ++	$(CC) $(CFLAGS) -o $@  $^ $(LIBS) -lcmocka -Wno-implicit-function-declaration
    ++	$@
    ++
    + tests: $(TESTS)
    + 
    + $(TESTS_DIR)/%.o: $(TESTS_DIR)/%.c
    +
      ## fileconf.c ##
     @@
      #include "fileconf.h"
    @@ fileconf.c: get_string(char *ptr)
      
     +char *
     +get_command(char *ptr)
    -+    /* read a shell command from ptr, to the end of the (logical) line */
    ++    /* read a shell command from ptr to the end of the (logical) line,
    ++     * removing trailing blanks */
     +{
    ++    remove_blanks(ptr);
     +    return strdup2(ptr);
     +}
      
    @@ fileconf.c: read_period(char *ptr, cf_t *cf)
          if (strcmp(cl->cl_shell, "\0") == 0) {
              fprintf(stderr, "%s:%d: No shell command: skipping line.\n",
                      file_name, line);
    +
    + ## fileconf.h ##
    +@@
    +  *  `LICENSE' that comes with the fcron source distribution.
    +  */
    + 
    ++#include "global.h"
    + 
    + #ifndef __FILECONF_H__
    + #define __FILECONF_H__
    +@@ fileconf.h: extern int read_file(char *filename, int fd);
    + extern void delete_file(const char *user_name);
    + extern int save_file(char *path);
    + 
    ++/* for tests */
    ++int read_shortcut(char *, cf_t *);
    ++void read_arys(char *, cf_t *);
    ++void read_freq(char *, cf_t *);
    ++void read_period(char *, cf_t *);
    ++
    + #endif                          /* __FILECONF_H__ */
    +
    + ## tests/parse_job_test.c (new) ##
    +@@
    ++#include <cmocka.h>
    ++
    ++#include "../mem.h"
    ++#include "../fileconf.h"
    ++
    ++#define TestParser(PARSER, DESC, LINE, EXPECTED_COMMAND) \
    ++{ \
    ++    cf_t cf; \
    ++    char *input = strdup2(LINE); \
    ++    PARSER(input, &cf); \
    ++    assert_string_equal(cf.cf_line_base->cl_shell, EXPECTED_COMMAND); \
    ++    Free_safe(input); \
    ++}
    ++
    ++#define TestArysParser(D, L, E) TestParser(read_arys, D, L, E)
    ++#define TestPeriodParser(D, L, E) TestParser(read_period, D, L, E)
    ++#define TestFreqParser(D, L, E) TestParser(read_freq, D, L, E)
    ++#define TestShortcutParser(D, L, E) TestParser(read_shortcut, D, L, E)
    ++
    ++static void
    ++test_read_arys_shell_parser(void **state)
    ++{
    ++    (void)state; /* unused */
    ++    TestArysParser("example 1",
    ++                   "& 05,35 12-14 * * * mycommand -u me -o file ",
    ++                   "mycommand -u me -o file");
    ++    TestArysParser("quoted command",
    ++                   "* * * * * \"/root/my script\"",
    ++                   "\"/root/my script\"");
    ++    TestArysParser("Hourly Git Maintenance",
    ++                   "32 1-23 * * * \"/usr/libexec/git-core/git\" --exec-path=\"/usr/libexec/git-core\" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=hourly",
    ++                   "\"/usr/libexec/git-core/git\" --exec-path=\"/usr/libexec/git-core\" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=hourly");
    ++    TestArysParser("Daily Git Maintenance",
    ++                   "32 0 * * 1-6 \"/usr/libexec/git-core/git\" --exec-path=\"/usr/libexec/git-core\" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=daily",
    ++                   "\"/usr/libexec/git-core/git\" --exec-path=\"/usr/libexec/git-core\" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=daily");
    ++    TestArysParser("Weekly Git Maintenance",
    ++                   "32 0 * * 0 \"/usr/libexec/git-core/git\" --exec-path=\"/usr/libexec/git-core\" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=weekly",
    ++                   "\"/usr/libexec/git-core/git\" --exec-path=\"/usr/libexec/git-core\" -c credential.interactive=false -c core.askPass=true  for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=weekly");
    ++}
    ++
    ++static void
    ++test_read_freq_shell_parser(void **state)
    ++{
    ++    (void)state; /* unused */
    ++    TestFreqParser("example 1",
    ++                   "@ 30 getmails -all",
    ++                   "getmails -all");
    ++    TestFreqParser("example 2",
    ++                   "@mailto(root),forcemail 2d /etc/security/msec/cron-sh/security.sh",
    ++                   "/etc/security/msec/cron-sh/security.sh");
    ++    TestFreqParser("quoted command",
    ++                   "@ 12h02 \"/root/my script\"",
    ++                   "\"/root/my script\"");
    ++    TestFreqParser("internal quotes",
    ++                   "@ 3d echo \"hi\"",
    ++                   "echo \"hi\"");
    ++    TestFreqParser("blanks",
    ++                   "@ 3w2d5h1 true  ",
    ++                   "true");
    ++}
    ++
    ++static void
    ++test_read_period_shell_parser(void **state)
    ++{
    ++    (void)state; /* unused */
    ++    TestPeriodParser("example 1",
    ++                     "%nightly,mail(no) * 21-23,3-5 echo \"a nigthly entry\"",
    ++                     "echo \"a nigthly entry\"");
    ++    TestPeriodParser("example 2",
    ++                     "%hours * 0-22 * * * echo \"Ok.\"",
    ++                     "echo \"Ok.\"");
    ++    TestPeriodParser("quoted command",
    ++                     "%hourly 31 \"/root/my script\"",
    ++                     "\"/root/my script\"");
    ++    TestPeriodParser("internal quotes",
    ++                     "%middaily 21 7-10 echo \"hi\"",
    ++                     "echo \"hi\"");
    ++    TestPeriodParser("blanks",
    ++                     "%monthly 59 4 12 true  ",
    ++                     "true");
    ++}
    ++
    ++static void
    ++test_read_shortcut_shell_parser(void **state)
    ++{
    ++    (void)state; /* unused */
    ++    TestShortcutParser("example 1",
    ++                       "@hourly check_laptop_logs.sh",
    ++                       "check_laptop_logs.sh");
    ++    TestShortcutParser("example 2",
    ++                       "@daily check_web_server.sh",
    ++                       "check_web_server.sh");
    ++    TestShortcutParser("example 3",
    ++                       "@daily check_file_server.sh",
    ++                       "check_file_server.sh");
    ++    TestShortcutParser("example 4",
    ++                       "@monthly compress_home_made_app_log_files.sh",
    ++                       "compress_home_made_app_log_files.sh");
    ++    TestShortcutParser("quoted command",
    ++                       "@weekly \"/root/my script\"",
    ++                       "\"/root/my script\"");
    ++    TestShortcutParser("internal quotes",
    ++                       "@reboot echo \"hi\"",
    ++                       "echo \"hi\"");
    ++    TestShortcutParser("quoted command",
    ++                       "@yearly true   ",
    ++                       "true");
    ++}
    ++
    ++int
    ++main(void)
    ++{
    ++    const struct CMUnitTest tests[] = {
    ++        cmocka_unit_test(test_read_arys_shell_parser),
    ++        cmocka_unit_test(test_read_freq_shell_parser),
    ++        cmocka_unit_test(test_read_period_shell_parser),
    ++        cmocka_unit_test(test_read_shortcut_shell_parser),
    ++    };
    ++    return cmocka_run_group_tests(tests, NULL, NULL);
    ++}

I found it somewhat difficult to get the compilation recipe setup correctly, and the way state is communicated between some functions left me scratching my head 😅 Hopefully I've hit what you were aiming for, though!

benknoble · 2026-03-20T19:21:27Z

Salut @yo8192 , just a nudge in case you find time soon to give this a look :)

…d function descriptions.

benknoble · 2026-03-23T22:48:33Z

+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>             /* setjmp.h is needed by cmocka.h */
 #include <cmocka.h>


Hm. FWIW, the cmocka docs state (and on my system they were right!) that the first 3 are implied by #include <cmocka.h>.

benknoble · 2026-03-23T22:49:09Z

 int read_shortcut(char *, cf_t *);
 void read_arys(char *, cf_t *);
 void read_freq(char *, cf_t *);
 void read_period(char *, cf_t *);


I'm still not sure what provides cf_t in this header, without global.h

benknoble · 2026-03-27T20:56:56Z

Any chance we'll see a new release with this fix soon?

yo8192 · 2026-05-17T10:23:25Z

I'll try to find a bit of time to cut a new release in the coming weeks.

…

On Fri, 27 Mar 2026 at 20:57, D. Ben Knoble ***@***.***> wrote: *benknoble* left a comment (yo8192/fcron#47) <#47?email_source=notifications&email_token=ABPHBIQ5V4E64OU7YIW556L4S3TK3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJUGUZDCNJVG4ZKM4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4145215572> Any chance we'll see a new release with this fix soon? — Reply to this email directly, view it on GitHub <#47?email_source=notifications&email_token=ABPHBIQ5V4E64OU7YIW556L4S3TK3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJUGUZDCNJVG4ZKM4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4145215572>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABPHBIXTL62ID3FTYGEK5H34S3TK3AVCNFSM6AAAAACWSQR4MGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCNBVGIYTKNJXGI> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

yo8192 requested changes Mar 15, 2026

View reviewed changes

Comment thread fileconf.c

benknoble force-pushed the bk/fix-46 branch from 1c3ebf7 to 4c399bb Compare March 16, 2026 22:03

benknoble requested a review from yo8192 March 16, 2026 22:05

yo8192 reviewed Mar 20, 2026

View reviewed changes

Comment thread fileconf.h Outdated

Fixed compilation of parse_job_test, applied indentation, and improve…

648948d

…d function descriptions.

yo8192 approved these changes Mar 22, 2026

View reviewed changes

yo8192 merged commit ec25ba2 into yo8192:master Mar 22, 2026
1 check failed

benknoble deleted the bk/fix-46 branch March 22, 2026 16:48

benknoble commented Mar 23, 2026

View reviewed changes

Navigation Menu

crontab: don't reject non-string-like shell commands - #47

crontab: don't reject non-string-like shell commands#47
yo8192 merged 2 commits into
yo8192:masterfrom
benknoble:bk/fix-46

benknoble commented Mar 15, 2026 •

edited

Loading

Uh oh!

yo8192 left a comment

Uh oh!

Uh oh!

benknoble commented Mar 16, 2026

Uh oh!

benknoble commented Mar 20, 2026

Uh oh!

Uh oh!

Uh oh!

benknoble Mar 23, 2026

Uh oh!

benknoble Mar 23, 2026

Uh oh!

benknoble commented Mar 27, 2026

Uh oh!

yo8192 commented May 17, 2026 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

benknoble commented Mar 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yo8192 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benknoble commented Mar 16, 2026

Uh oh!

benknoble commented Mar 20, 2026

Uh oh!

Uh oh!

Uh oh!

benknoble Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

benknoble Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

benknoble commented Mar 27, 2026

Uh oh!

yo8192 commented May 17, 2026 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

benknoble commented Mar 15, 2026 •

edited

Loading