Any suggestion to improve NanoLog type safety?

[wizwx]

I learned in the hard way that NanoLog can behave very tricky when you have a typo in the format string, and the decompressor may not pick up properly from the encoded byte stream.

Here is one example:
NANO_LOG(NOTICE, "Test1 v=%4.2f", 4);
the above will print out "Test1 v=0.00"

Here is another one:
const char * str = "Hello World"; NANO_LOG(NOTICE, "Test2 v=%4.2f s=%.*s", 4, 5, str);
This will trigger assert to abort. If I change (*in) += sizeof(T); in unpack function for floating point values in Packer.h to (*in) += bytes;, then it will not abort and print out "Test2 v=0.00 s=Hello".

First question: is the change above in unpack function correct? Since unpack reads "bytes" of bytes from the memory to the destination variable, it makes sense to forward the memory location by bytes.

Second question/comment:
There may be a lot of other similar cases out there.
One suggestion is: right now, you only differentiate STRING from NON_STRING. Any chance you can take one step further and extend ParamType to tell different value type from NON_STRING, such as whether the non-string is an integer or a float? In this way, you can convert the passed in argument to the right value type (based on ParamType) in the store_argument function before you store the value.

Thank you.

[syang0]

Hi wizwx,

I'm sorry this issue has caused you some headache. C++17 NanoLog actually does have built-in type-safety through the use of GNU's function attributes to perform format checking at compile-time. However, it needs to be enabled by passing the -Werror=format parameter to the compiler. It's my fault for both not explicitly mentioning this in the README and not adding this to the sample application's GNUmakefile.

I'll create a commit to fix this.

As for your other question, I think your change is correct. However I suspect it fixed the issue by chance rather than addressing the root cause format error. I will still make the change as it's technically more correct after I verify the unit tests still pass.

Best Regards,
Stephen Yang

[wizwx]

Thank you for the reply. It is very helpful.

The sample code NANO_LOG(NOTICE, "Test1 v=%4.2f", 4); is a simple case to illustrate that type safety is critical for NanoLog.

If my understanding is correct, NanoLog encodes (save the value) by the static type of the passed in variable, while decoding is based on the specification in the format string. The sample code illustrated that if you have mismatch between the type specifier in the format string and the passed in value type, you can have different issues: 1. you can read back wrong value (simply because of wrong type), and 2. you can corrupt memory (decoding consumes different amount of memory than the encoding).

Yes, compiler warnings are very helpful in this case and will enforce users to provide the right type.
I would suggest that you always turn on format checking, instead of relying on users in doing so. In gcc you can achieve this with the following:

#pragma GCC diagnostic push
#pragma GCC diagnostic error "-Wformat"
if (false) { NanoLogInternal::checkFormat(format, ##VA_ARGS); }
#pragma GCC diagnostic pop

[syang0]

Hey wizwx,

I like your suggestion! However, I gave it a quick try in NanoLogCpp17.h and got compilation errors. I suspect this happens because the NANO_LOG macro only spans 1 line (due to the '\' characters), and the #pragmas need to be on their own line.

In file included from main.cc:24:
../runtime/NanoLogCpp17.h:1075:39: error: '#' is not followed by a macro parameter
 #define NANO_LOG(severity, format, ...) do { \

Let me know if you can figure a way around this constraint. Otherwise, -Werror=format may be the only solution to this problem for now.

Best Regards,
Stephen Yang

[wizwx]

Also tried _Pragma instead of #pragma.
The standard is unclear on where a _Pragma operator can appear.
From my testing, _Pragma does not have any compile errors when used in a macro but it also does not seem to function at all (no warnings for wrong format).

[wizwx]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55578
so this seems a bug in gcc and has not been fixed after 8 yrs...