Fix: libcrmcommon: Don't assert in time_to_hr.

[clumens]

This just allows time_to_hr and pcmk__time_hr_now to return NULL, which can then be handled higher up in the call stack. This is just to address an automatically filed fuzzer bug report - it can't figure out how to deal with the fact that a function it's fuzzing can assert.

Fixes https://issues.oss-fuzz.com/issues/456526118

[nrwahl2]

it can't figure out how to deal with the fact that a function it's fuzzing can assert.

A lot of our functions can assert, so I'd like to see this addressed at the fuzzer level if at all possible. This is going to lead to more problems as people fuzz more of our code.

With that said, I'm not tied to asserting in time_to_hr(). In pcmk__time_hr_new(), we pass it the return value from parse_date(date_time), where date_time is the argument to pcmk__time_hr_new():

pcmk__time_hr_t *
pcmk__time_hr_new(const char *date_time)
{
    pcmk__time_hr_t *hr_dt = NULL;

    if (date_time == NULL) {
        hr_dt = pcmk__time_hr_now(NULL);
    } else {
        crm_time_t *dt = parse_date(date_time);

        hr_dt = time_to_hr(dt);
        crm_time_free(dt);
    }
    return hr_dt;
}

That means we're relying on that string argument being parsable as a date-time, and asserting if not. I don't think I like that.

---

HOWEVER, the sole caller of pcmk__time_hr_new() (besides the fuzzer) is the TEST_TIME macro in pcmk__time_format_hr_test.c. We could simply drop pcmk__time_hr_new() altogether and construct the pcmk__time_hr_t some other way for the unit test.

[nrwahl2]

HOWEVER, the sole caller of pcmk__time_hr_new() (besides the fuzzer) is the TEST_TIME macro in pcmk__time_format_hr_test.c. We could simply drop pcmk__time_hr_new() altogether and construct the pcmk__time_hr_t some other way for the unit test.

I misread -- there is a pcmk__time_hr_now() and a pcmk__time_hr_new(). So I need to look again.

[nrwahl2]

I'm unable to view the OSS Fuzz bug. Access denied. I sent you a Slack message about it.

Anyway, I don't know if OSS Fuzz is complaining about pcmk__time_hr_now() or pcmk__time_hr_new() (it fuzzes both of them), or what.

I would like to see us drop pcmk__time_hr_new() and construct the pcmk__time_hr_t object another way in the sole caller besides the fuzzer (a unit test).

When pcmk__time_hr_now() calls time_to_hr(), the dt argument is clearly guaranteed to be non-NULL. So it should be totally safe to assert that within time_to_hr(). But at that point, pcmk__time_hr_now() would be the sole remaining caller of time_to_hr(). So we could either inline time_to_hr() or simply drop the assertion. In Pacemaker, we usually don't NULL-check arguments in a static function with a single caller. We often assert when there are multiple callers because it becomes easier to lose track of things.