Possible null pointer dereference in time_string()

[gvranken]

In time_string() (otime.c):

buf_printf(&out, "%s", ctime(&t));

buf_printf call vsnprintf which tries to print the string returned ctime.
However, ctime may return NULL if 't' is invalid. On Linux/glibc and FreeBSD (these systems I've tested so far), vsnprintf will print the string "(null)". But as far as I can confirm, the behavior of vsnprintf is undefined in this case (relevant standards make no mention of this particular situation), so a NULL pointer dereference may occur.

Is there any code path in which 't' is defined (sent) by the peer?
Can the case vsnprintf with format string "%s" and argument NULL be tested on other systems than the ones I mentioned? I'd be interesting to see what the maximum impact, and on which system, may be.

Guido

[Gert Döring]

in time_string(), t is a local variable, so &t will always be valid.

(The caller can influence the number stored in t, but as there are no restrictions on the numerical value wrt "seconds since the begin of the epoch", it can not be invalid either).

[gvranken]

Yes it can be invalid

#include <string.h>
#include <stdio.h>
#include <time.h>
int main(void)
{
    time_t t;
    memset(&t, 0x01, sizeof(t));
    printf("%p\n", ctime(&t));
}

[plaisthos]

Yes, a time_t can be invalid, but the time_t in our code is always valid.

[Gert Döring]

The example prints "0x800bc7340" here... and printing the string instead of the pointer, int prints "Thu Aug 9 07:17:53 -2002596506", which might not be very useful, but is most certainly not NULL.

But otherwise, what plaistos says - we either pass in "0", or time values fully under our control (mi->created, route->last_reference, pin->time (0 or time()), or DHCP lease stuff also set ourselves, or e->tv via tv_string_abs(), which is our event stuff).

[gvranken]

Ok, I get a NULL ptr on Debian 64 bit for my example.

It still might be worth the effort to do something like

timestr = ctime(&t);
if ( timestr )
   buf_printf(&out, "%s", ctime(&t));
else
   buf_printf(&out, "(null"));

Theoretically the preceding gettimeofday() can fail and leave 'tv' undefined. Definitely a corner case but who knows. And maybe in the future paths to time_string() may be created that handle arbitrary user time_t values.

Other than that, I have no further comments and you may close this report.

[gvranken]

Added a proper patch

[Gert Döring]

I do not see this is a good use of our collective time - there's more pressing things right now, and the otime.c code is due for a rewrite (to produce proper ISO timestamps) anyway, there's an existing trac ticket for that.

Given that all platforms that we support *either* just accept arbitrary values passed as time_t *or* just return "(null)" if feeding a NULL pointer to "%s", and we never pass in anything remotely undefined today (and there is no reason why we should in future, as there are no timestamps in the protocol we might be tempted to print), this is far too obscure to worry about.