[tz] Minor (unimportant really) technical UB bug in strftime() ?

[Paul Eggert]

On 2022-11-10 16:11, Tom Lane wrote:

> simple modifications of the
> strftime() case can provide pretty good examples.
I'm not sure I'm following this argument, but if I understand it 
correctly I disagree. I very much much want implementations to reject 
attempts to use uninitialized data, and I don't want the C standard to 
prohibit such implementations. Use of uninitialized data is a serious 
problem in real applications - several CVEs have arisen from it - and 
the C standard shouldn't require implementations to sweep it under the rug.

Let me try to make this more concrete. Suppose we are on a platform with 
no trap representations (which is the typical case), and suppose we make 
the following patch to tzcode's implementation of strftime with "%s":

	--- a/strftime.c
	+++ b/strftime.c
	@@ -319,16 +319,7 @@
					time_t) + 1];
		time_t		mkt;

	-	tm.tm_sec = t->tm_sec;
	-	tm.tm_min = t->tm_min;
	-	tm.tm_hour = t->tm_hour;
	-	tm.tm_mday = t->tm_mday;
	-	tm.tm_mon = t->tm_mon;
	-	tm.tm_year = t->tm_year;
	-	tm.tm_isdst = t->tm_isdst;
	-#if defined TM_GMTOFF && ! UNINIT_TRAP
	-	tm.TM_GMTOFF = t->TM_GMTOFF;
	-#endif
	+	memcpy(&tm, t, sizeof tm);
		mkt = mktime(&tm);
		/* If mktime fails, %s expands to the
		   value of (time_t) -1 as a failure

and suppose someone calls tzcode strftime this way:

   struct buf[1000];
   char *f(void)
   {
     struct tm uninitialized;
     return strftime(buf, sizeof buf, "%s", &tm);
   }

This call is obviously buggy since it is passing an uninitialized struct 
tm to strftime. However, the C standard says the patched strftime's 
memcpy initializes strftime's local variable tm, and therefore mktime's 
use of tm's contents is defined and the implementation allow the 
program's use of uninitialized data.

Whatever the reason for this peculiarity of the C standard (and nobody 
seems to remember why it's there), it's counterproductive. In general C 
programs shouldn't use uninitialized data. If they do, it's useful to 
have a debugging implementation that reports any such use the instant 
that it happens as this will help catch real bugs. And since common 
debugging implementations like valgrind and UBSan are not following the 
C standard here, this suggests that the C standard is mistaken, not the 
implementations.


> That leads either to strftime() knowing
> much more about mktime()'s innards than it ought to

It's not a problem for strftime, since strftime's has to know what's 
needed in the struct tm; this follows from strftime's API.

It might be a problem for other scenarios, such as the one you vaguely 
suggested though I don't know the details. However, even assuming these 
other scenarios exist, they don't justify this peculiarity of the C 
standard. The standard should not say that the ordinary meaning of 
"don't use uninitialized variables" is wrong and that there's some 
trickier interpretation that hardly any programmer knows and that was 
wrong even in C17 and so needed rewording (and perhaps still needs 
rewording) and that nevertheless must be supported - even though 
important debugging implementations disagree.