General correctness improvements

[jpdoyle]

• Replace potentially-unsafe use of strrchr with a format string generated via snprintf
• Make symbol_t::precision unsigned to prevent potentially corrupt generated format strings.
• Replace possibly overflowing double-to-int cast with trunc()
• Change the endtm range check to exclude NaN
• Improve error message if endtm range check fails
• Enable -Wextra and -Werror, and fix the warning caused by j < LENGTH(symbols).

[jpdoyle]

I removed the if(part >= UINT_MAX) check in time_to_sec because:

• it does not prevent integer overflow in that function (part >= INT_MAX would prevent overflow)
• With trunc(), overflow protection is unnecessary.

With this version, it's also possible to remove the endtm < INT_MAX check entirely, and still be safe. However, the maximum value of time_t is not specified by the C standard, nor is its exact representation, so it's probably best to limit the range to something sensible like [0,INT_MAX).

[bitsmanent]

Thanks for your effort in improving snore.

I appreciate your aiming at correct (and secure) code but I would also keep things as pragmatic and simple as possible.

Can you give a look at HEAD to see if you find any issue? I simplified time_print and fixed if(part >= UINT_MAX) with INT_MAX (thanks for pointing out).

Please, notice that using -Wextra is undesirable due to very frequent false positives and cosequent nuisance of fixing them. This is even worse with -Werror which turns a 100% correct code which always works into a compile-time error. For example, you had to "fix" the unsigned to int comparison (j < LENGTH(symbols)) in order to be able to build snore again.

[jpdoyle]

I had forgotten about .* in format strings -- that's much better than building a format string manually!

The current code looks safe, although I'm in favor of trunc() rather than casting to int, because it is guaranteed to work even outside the range.

It looks like you haven't fixed the range check to exclude nan -- it isn't directly exploitable, but I think it could cause problems with the cast to int if anything gets changed.

I'm generally in favor of -Wextra -Werror, because I haven't found any changes it requires that make the code worse. As you mention, it is a bit stricter than necessary -- it can't tell that j >= 0 without you specifying that j is unsigned. Although it's worth keeping in mind that for example, -1 < (unsigned)1 is false, so removing any signed/unsigned comparisons generally removes footguns.

[bitsmanent]

Ok, thanks for your feedback.

I will investigate about possible issues with nan and see if there is room for improvements. Though, as stated on the README, we don't expect to make further changes so this has very low priority.