Invalid match

[ghseb]

Hello,

i've found a potential bug in the cron implementation. The following test demonstrates the issue:

 // Asserts: Expected next match: "2022-11-01_00:00:00", actual next match: "2022-12-01_00:00:00"
check_next("0 0 0 ? 11-12 *",  "2022-10-31_00:00:00", "2022-11-01_00:00:00");

// Works: Expected and actual next match: "2022-11-01_00:00:00"
check_next("0 0 0 ? 11-12 *",  "2022-10-30_00:00:00", "2022-11-01_00:00:00");

Can anybody reproduce the issue?
What could be the reason for the invalid next match?

[slavslavov]

I can confirm this behaviour. Compiled the current master on Linux with gcc 11.3.1.

[ghseb]

I think i found the cause and probably can provide a way to fix the issue.

Given check that asserts: check_next("0 0 0 ? 11-12 *", "2021-10-30_00:00:01", "2021-11-01_00:00:00");

I realized, that when set_field is called in find_next the month is incremented from 9 (Oktober) to 10 (November). But within set_field mktime "normalizes" calendar to tm_mon being 11 and tm_mday being 1 (December the first), because mktime is fed with an invalid values in the calendar structure (tm_mday is set to 31 and tm_mon is set to 10).

See https://linux.die.net/man/3/mktime for reference.

It turns out that changing the order of incermenting month and resetting the calendar does resolve the observed issue:

	static unsigned int find_next(uint8_t* bits, unsigned int max, unsigned int value, struct tm* calendar, unsigned int field, unsigned int nextField, int* lower_orders, int* res_out) {
		int notfound = 0;
		int err = 0;
		unsigned int next_value = next_set_bit(bits, max, value, &notfound);
		/* roll over if needed */
		if (notfound) {
			err = add_to_field(calendar, nextField, 1);
			if (err) goto return_error;
			err = reset_min(calendar, field);
			if (err) goto return_error;
			notfound = 0;
			next_value = next_set_bit(bits, max, 0, &notfound);
		}
		if (notfound || next_value != value) {
+			err = reset_all_min(calendar, lower_orders);
+			if (err) goto return_error;
			err = set_field(calendar, field, next_value);
			if (err) goto return_error;
-			err = reset_all_min(calendar, lower_orders);
-			if (err) goto return_error;
		}
		return next_value;

		return_error:
		*res_out = 1;
		return 0;
	}

I am not sure if there are implications that cause other issues with the proposed change, though. The supplied tests succeed (at least on my platform).

What do you think?

[slavslavov]

@ghseb I have tested with your fix with the following test cases and all seems to work OK

    check_next("0 0 0 ? 11-12 *",   "2022-05-31_00:00:00", "2022-11-01_00:00:00");
    check_next("0 0 0 ? 11-12 *",   "2022-07-31_00:00:00", "2022-11-01_00:00:00");
    check_next("0 0 0 ? 11-12 *",   "2022-08-31_00:00:00", "2022-11-01_00:00:00");
    check_next("0 0 0 ? 11-12 *",   "2022-10-31_00:00:00", "2022-11-01_00:00:00");
    check_next("0 0 0 ? 6-7 *",     "2022-05-31_00:00:00", "2022-06-01_00:00:00");
    check_next("0 0 0 ? 8-9 *",     "2022-07-31_00:00:00", "2022-08-01_00:00:00");
    check_next("0 0 0 ? 9-10 *",    "2022-08-31_00:00:00", "2022-09-01_00:00:00");

[ghseb]

@slavslavov thanks for the feedback!
@staticlibs and @Kniggebrot maybe you also got a comment?

[Kniggebrot]

I didn't notice the issue yet, thank you for finding and fixing it! Tried it with the following test cases as well, everything works ok.

    check_next("0 0 0 ? 2-3 *",     "2022-01-31_00:00:00", "2022-02-01_00:00:00");
    check_next("0 0 0 ? 4-5 *",     "2022-03-31_00:00:00", "2022-04-01_00:00:00");

[staticlibs]

This project is no longer maintained, see its updated and extended fork in exander77/supertinycron repo, closing the issue to archive the repo.