networkupstools / nut

The Network UPS Tools repository. UPS management protocol Informational RFC 9271 published by IETF at https://www.rfc-editor.org/info/rfc9271 Please star NUT on GitHub, this helps with sponsorships!
https://networkupstools.org/
Other
2.04k stars 349 forks source link

For the few cases where we use variables as formatting strings, find a way to ensure it is safe #2450

Open jimklimov opened 5 months ago

jimklimov commented 5 months ago

In a few cases we use formatting strings as variables (e.g. coming from some tables or even constructed at run-time) which is error-prone with regard to interpretation of subsequent memory stack when calling a printf-related method. While this is done only for strings defined in NUT codebase, it has a potential to regress if someone modifies the table value in some later revision. Currently we hush a warning like format not a string literal and no format arguments and a few similar others with pragmas, but there gotta be some better way.

Some first ideas (more welcome):

Either way, I think we lose the facility of modern compilers to also statically check the types (that a %i refers to an int-sized number, and not a long or char*, etc.) in these cases, so some error-proneness remains even if the amount of args remains but their type changes.

Maybe the solution to get the best of all worlds could be in fact to specify the runtime method to return a string (so callers would go like dstate_setinfo("ups.model", "%s", checked_format(variableFormat, checkingFormat, ...)); and avoid hushing pragmas altogether) with checkingFormat being a real formatting string like "%s%s%i%"PRIuSIZE"%f" according to the types of subsequent vararg parameters, and the preceding variableFormat argument specifying the actual formatting string (expected/checked to mention exactly the same set of percent-formats in same order).

This way we could have compile-time checks that varargs conform in amount and type to some contrived formatting string, and run-time assertions that whatever variable string we actually use to produce the checked_format() string is compatible with those expectations.

Is there some (library?) method to strip non-formatting characters (plain text, format beautification with sizes/alignments like %.01f => %f or whatever) so we could directly strcmp() the expected pattern vs. the stripped dynamic formatting string? If not, we have a fallback printf() implementation that I guess could be wrangled into such a helper method...

ntd commented 4 months ago

Not sure if relevant, but on GCC you can add typechecking by adding the following attribute to the function declaration:

__attribute__ ((format (printf, <string-index>, <first-to-check>)));

In GLib it is used extensively through the G_GNUC_PRINTF macro.

ntd commented 4 months ago

Not sure if relevant, but on GCC you can add typechecking by adding the following attribute to the function declaration:

__attribute__ ((format (printf, <string-index>, <first-to-check>)));

In GLib it is used extensively through the G_GNUC_PRINTF macro.

Sorry, I just seen you are already using it.

jimklimov commented 4 months ago

Thanks! Not sure how portable this is, but the attribute is actually used in include/common.h (and some other files with static methods) to mark the varargs support, and all compilers currently used in the NUT CI farm do not complain at least.

Probably that helps clang and gcc raise the compile-time warnings, and this is what one part of this solution relies on with the "reference" formatting strings (the fallback being that they can be checked by a human to match any subsequent actual string/numeric/... arguments).