Compiler warning about sprintf

[chfast]

Hi there,

I get the following compiler warning:

duk_logging.c:184:64: warning: ‘Z’ directive writing 1 byte into a region of size between 0 and 9 [-Wformat-overflow=]
  sprintf((char *) date_buf, "%04d-%02d-%02dT%02d:%02d:%02d.%03dZ",
                                                                ^
In file included from /usr/include/stdio.h:867,
                 from duk_logging.c:5:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:36:10: note: ‘__builtin___sprintf_chk’ output between 25 and 85 bytes into a destination of size 32
   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       __bos (__s), __fmt, __va_arg_pack ());
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Obviously you can fix it by increasing the date_buf size to 85 in https://github.com/svaarala/duktape/blob/master/extras/logging/duk_logging.c#L138.

I wander if you have any opinions on that.

[svaarala]

That format ending with %03dZ is trying to format a 3-digit integer, followed by a literal Z. I don't know why it would be interpreted as a Z format string - and if you run the actual code it produces a Z literal as expected. So possibly the warning is just wrong?

[chfast]

Maybe it's a false positive. Let me check the compiler version and what flag Go is using and I will try to reproduce this with a smaller example.

[karalabe]

The warning is a bit misleading, it's not the Z that it has a problem with, rather the entire formatting string. The formatting string specifies the minimum sizes of the fields (e.g. %04d for year), but the data type the method puts into that slot might marshal into more than 4 characters (same for the other fields).

[svaarala]

The values are range checked before the call, but the compiler probably doesn't carry that range information far enough. It would need to carry that information through the public API, as ECMAScript datetime components have known ranges.

[svaarala]

I don't see how the compiler could be (reliably) made aware of the actual ranges of the values, so the solution would be either to suppress the warning (the buffer is large enough so it is bogus) or to switch to snprintf().

The annoying thing about snprintf() is that it's not portable below C99 (which Duktape also targets) so it's going to need an ifdef at least for older MSVC (as in https://github.com/svaarala/duktape/blob/master/extras/module-duktape/duk_module_duktape.c#L8-L14). Inside Duktape itself there are platform/compiler workarounds for this already, but the logging module is external so it doesn't have access to these. Another non-MSVC-specific alternative is to check for C99 and use snprintf() if C99 is available, and fall back to sprintf() otherwise (and allow the warning).