Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jimklimov opened this issue May 22, 2024 · 3 comments · May be fixed by #2460
Open
Assignees
Labels
CI Entries related to continuous integration infrastructure (historically also recipes like Makefiles) refactor/fightwarn PR or issue proposal to improve code maintainability without functional changes, or to fix warnings
Milestone

Comments

@jimklimov
Copy link
Member

jimklimov commented May 22, 2024

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):

  • create a macro (one with a number argument, or a set of macros with different amount of args) just to pass a hint about amount of expected formatting arguments, and somehow parse (pycparser as used elsewhere? simpler perl magic?) to statically check that the amount of unescaped percent characters in the resolved first argument (at least when fixed from tables) matches the macro hint amount;
  • do same in runtime to at least throw fatalx() or similar and not do dangerous things - at least in our methods like upsdebugx() or dstate_setinfo(), we can at least control the varargc vs. amount of percents in the actual formatting string.

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...

@jimklimov jimklimov added CI Entries related to continuous integration infrastructure (historically also recipes like Makefiles) refactor/fightwarn PR or issue proposal to improve code maintainability without functional changes, or to fix warnings labels May 22, 2024
@jimklimov jimklimov added this to the 2.8.4 milestone May 22, 2024
@clepple clepple removed their assignment May 23, 2024
jimklimov added a commit to jimklimov/nut that referenced this issue May 31, 2024
…ng() [networkupstools#2450]

...and minimize_formatting_string_staticbuf() for one-shot use-cases.

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue May 31, 2024
…roduce snprintf_dynamic() and related methods [networkupstools#2450]

Mitigate the inherent insecurity of dynamically constructed formatting
strings vs. a fixed vararg list with its amounts and types of variables
printed by this or that method and pre-compiled in the program.

* minimize_formatting_string() with caller-specified buffer;
* minimize_formatting_string_staticbuf() for one-shot use-cases;
* validate_formatting_string() to compare a dynamic and expected formatting strings;
* vsnprintf_dynamic(), vsnprintfcat_dynamic() for practical applications (with fixed va_list argument);
* snprintf_dynamic(), snprintfcat_dynamic(), mkstr_dynamic() for practical applications (with ... variadic arguments);
* added vsnprintfcat() with fixed va_list argument, for good measure.

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue May 31, 2024
…rsions for hardened dynamic format string support [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue May 31, 2024
jimklimov added a commit to jimklimov/nut that referenced this issue May 31, 2024
…oc: introduce snprintf_dynamic() and related methods [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue May 31, 2024
…nt formats for same data type into same char to ease sanity-check comparisons [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 1, 2024
…nt formats for same data type into same char to ease sanity-check comparisons [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 1, 2024
…llapse known different formats for same data type into same char to ease sanity-check comparisons [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
…roduce snprintf_dynamic() and related methods [networkupstools#2450]

Mitigate the inherent insecurity of dynamically constructed formatting
strings vs. a fixed vararg list with its amounts and types of variables
printed by this or that method and pre-compiled in the program.

* minimize_formatting_string() with caller-specified buffer;
* minimize_formatting_string_staticbuf() for one-shot use-cases;
* validate_formatting_string() to compare a dynamic and expected formatting strings;
* vsnprintf_dynamic(), vsnprintfcat_dynamic() for practical applications (with fixed va_list argument);
* snprintf_dynamic(), snprintfcat_dynamic(), mkstr_dynamic() for practical applications (with ... variadic arguments);
* added vsnprintfcat() with fixed va_list argument, for good measure.

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
…rsions for hardened dynamic format string support [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
…llapse known different formats for same data type into same char to ease sanity-check comparisons [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
….c: introduce verbosity option to validate_formatting_string() and minimize_formatting_string() [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
…ch to snprintf_dynamic() instead of hushing potential flaws with macros [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
…ad of hushing potential flaws with macros [networkupstools#2450]

Found by pragmas to clean up, with

    :; git grep -En 'Wformat-(sec|nonlit)'

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
…operly cast the value, and harden with snprintf_dynamic() [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
…t the odd conversion, and harden with snprintf_dynamic() [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
….battery.start" might vary by applicable formatting strings [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
…witching to snprintf_dynamic() instead of hushing potential flaws with macros [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
…may produce invalid printf-style strings and not complain (garbage in = garbage out) [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
…lerate dynamic formats that are sub-strings and beginnings of reference (wasteful but survivable) [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
jimklimov added a commit to jimklimov/nut that referenced this issue Jun 2, 2024
…t-extra-args" in pragmas to quiesce "bogus-looking" test cases [networkupstools#2450]

Signed-off-by: Jim Klimov <[email protected]>
@ntd
Copy link
Contributor

ntd commented Jun 3, 2024

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
Copy link
Contributor

ntd commented Jun 3, 2024

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
Copy link
Member Author

jimklimov commented Jun 3, 2024

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).

jimklimov added a commit to jimklimov/nut that referenced this issue Jun 4, 2024
…x_blazer-common.c: define macros for minimize_formatting_string() and validate_formatting_string() verbosity argument values [networkupstools#2450]

Custom builds that do not want to require setting driver/tool debug levels
can re-define their NUT_DYNAMICFORMATTING_DEBUG_LEVEL to e.g. 0 and see
any formatting discrepancies instantly.

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jul 15, 2024
…nt [networkupstools#2511, networkupstools#2431, networkupstools#2450 et al]

Earlier bump went to "patch" component, but here we actually have
an API expansion (and more exported symbols), so the more important
component should be bumped.

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this issue Jul 15, 2024
…nt [networkupstools#2511, networkupstools#2431, networkupstools#2450 et al]

Earlier bump went to "patch" component, but here we actually have
an API expansion (and more exported symbols), so the more important
component should be bumped.

Signed-off-by: Jim Klimov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Entries related to continuous integration infrastructure (historically also recipes like Makefiles) refactor/fightwarn PR or issue proposal to improve code maintainability without functional changes, or to fix warnings
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants