Perhaps a comment should added to describe the functionality of this function, including describing that the PRI and VERSION have already been handled by parsemsg() and the actual log printing is handled by logmsg()

According to ctime(3), the field tm.tm_gmtoff represents "offset from UTC in seconds".
Therefore, I think this equation should have "* 60" added to the end.

Does this prevent the MSG utilising the MSG-UTF8 format (starting with BOM) allowed in RFC5424?

Is this check still required for the !RemoteAddDate case, now that strptime() is being used?
Perhaps this check should be moved to the RemoteAddDate case logic, to make sure that the "msg += RFC3164_DATELEN + 1" is only skipping over a timestamp?

Could this section be simplified by making use of strptime() to perform at least some of the parsing?

This comment should be updated to reflect the reduced functionality of this function.

Is this supposed to be "timestamp_remote.tm.tm_year -= 1900;"?

What happens if the time() function fails and returns -1?
Consider checking "t_now != -1" before running localtime_r?

There are only really 3 options/iterations ever required: next year, current year, or last year.
Consider setting the for loop test expression to "year < tm_now.tm_year - 1"?

I think this line can be done before the for loop, as tm_parsed will not set the tm.tm_year or tm.tm_isdst values?

Perhaps a comment should added to describe the functionality of this function, including describing that the PRI has already been handled by parsemsg() and the actual log printing is handled by logmsg()

This comment should be updated to explain the current functionality of this function.
I.e. this function currently logs messages in the RFC3164 format, dropping any provided msgid and structured_data information.

Unfortunately, it does mean that. I am not enough of an expert to fully judge what kind of data is safe to store and display on the console, which is why I thought it made more sense to stick to the existing filtering for the time being.

I think we want to be consistent between RFC 3164 and RFC 5424 messages in this regard. As in, I think both of them should use the same parsemsg_remove_unsafe_characters() function to determine what is and what is not permitted. That way we can always translate between both flavours of messages. I've added a TODO to that function.

That's a good point. I've just changed the code to simply call strptime() unconditionally. That also keeps the external behaviour of syslogd consistent, regardless of whether RemoteAddDate is used. The strlen() call can be moved further to the bottom, so that its value is invariant.

It could, but I thought it made more sense not to down that path. With strptime() being (close to) the opposite of strftime(), one could argue that strptime() parses time in a locale and time zone dependent fashion. POSIX leaves it unspecified what happens if you pass in a timestamp that is nonexistent in the current time zone (e.g., due to daylight saving time).

This piece of code is intended to parse time in a completely time zone oblivious fashion. It is also a bit more strict, as it does not permit leap seconds, per RFC 5424.

As we already need a PARSE_NUMBER() to parse the time zone offset, it takes little effort to do the parsing of the date and time ourselves.

Well spotted. Thanks!

The syslogd code already assumes that time() never fails in some other places. I think that's a fair assumption to make on somewhat decently configured systems.

Even if this call were to fail, there's nothing failing really badly. It simply thinks it's December 31st, 1969, meaning it will generate log entries between January 7th, 1969 an January 6th, 1970.

Yes, it can only run three iterations. That said, instead of adding that as an explicit check, let me simply document this fact.

My initial version of this code did exactly this, but that's incorrect. mktime() alters its input, which in my case caused it to apply the time zone offset/DST during every loop of the iteration. This is why I'm reinitialising the entire structure during every iteration now.

Probably worth adding an “else timestamp = NULL” to specifically initialise timestamp, and for clarity.

My concern was whether there was any possibility of the for loop iterating excessively (more than 3 times). E.g. if the inputs where bad, such as time() returning -1? Whilst I haven’t played out the possibilities, is it worth adding a for loop test to prevent any possible syslogd DoS.

Indeed. Fixed!

I've added an assertion to the code.