fmt::sprintf() is triggering the libc sprintf() rule

[markgalassi]

The fmt library in C++ offers a streamlined implementation of the boost::format libraries. Among other things it offers an sprintf() function. This function safely generates an std::string without buffer overflow issues.

nsiqcppstyle reports:
Do not use burfferoverflow risky function(sprintf [RULE_10_1_A_do_not_use_bufferoverflow_risky_function_for_unix]

for this line of code:

string s = fmt::sprintf("%2s%012.8f", year, doy_fraction);

The simple solution might be to check if sprintf( is preceded by a "::" (two colons), although that would run in to problems if you have "using namespace fmt;" and thus drop the fmt::. Still, I think it would be quite OK.

[kunaltyagi]

Checking for fmt::sprintf in the main rule would imply checking for library specific quirks (despite making it generic). That's because there's std::sprintf which will escape this.

I understand the rationale behind your suggestion, just that a good solution will not be scalable for this project. Maybe you could implement a different rule for your{self, organization} and make it an alternative to this one (available via opt-in). I welcome such contributions.

Or you could use some other checker that brings a lot more of semantic knowledge power to the rules such as clang-tidy.

[markgalassi]

[I put an edit below]
I just wrote a patch which would, as you say, not work for std::sprintf. The lexer does not immediately give easy access to the "prev of the prev" token, but if it did then we could check for ::sprintf() that is not std::sprintf().

But if I may propose a different way of thinking about "library specific quirks": the whole existence of RULE_10_1_A is all about quirks: the existence of legacy unsafe functions is a quirk of the standard library, and it is one of the most useful rules (lots of our contributors contribute code with these sad functions). I think it would be worth a bit of work and I'll have a go at it, starting from the trivial patch which is subject to the problem you mentioned:

--- a/rules/RULE_10_1_A_do_not_use_bufferoverflow_risky_function_for_unix.py
+++ b/rules/RULE_10_1_A_do_not_use_bufferoverflow_risky_function_for_unix.py
@@ -42,6 +42,10 @@ def RunRule(lexer, contextStack):
             t2 = lexer.PeekNextTokenSkipWhiteSpaceAndComment()
             if t2 is not None and t2.type == "LPAREN":
                 t3 = lexer.PeekPrevTokenSkipWhiteSpaceAndComment()
+                if t3.type == "DOUBLECOLON":
+                    # if :: comes first then we are not using
+                    # dangerous C functions, so we can get out
+                    return
                 if t3 is None or t3.type != "PERIOD":
                     nsiqcppstyle_reporter.Error(t, __name__,
                                                 "Do not use burfferoverflow risky function(%s)" % t.value)

EDIT: I have submitted a pull request with the following slightly more elaborate patch:

diff --git a/rules/RULE_10_1_A_do_not_use_bufferoverflow_risky_function_for_unix.py b/rules/RULE_10_1_A_do_not_use_bufferoverflow_risky_function_for_unix.py
index 00e4d8b..f525d09 100644
--- a/rules/RULE_10_1_A_do_not_use_bufferoverflow_risky_function_for_unix.py
+++ b/rules/RULE_10_1_A_do_not_use_bufferoverflow_risky_function_for_unix.py
@@ -42,6 +42,13 @@ def RunRule(lexer, contextStack):
             t2 = lexer.PeekNextTokenSkipWhiteSpaceAndComment()
             if t2 is not None and t2.type == "LPAREN":
                 t3 = lexer.PeekPrevTokenSkipWhiteSpaceAndComment()
+                # if :: comes first then we are not using dangerous C
+                # functions, so we can get out
+                if t3.type == "DOUBLECOLON":
+                    # check out the namespace that gives us this ::
+                    prev_namespace_token = lexer.GetPrevTokenInType("ID", keepCur=True)
+                    if prev_namespace_token.value != "std":
+                        return
                 if t3 is None or t3.type != "PERIOD":
                     nsiqcppstyle_reporter.Error(t, __name__,
                                                 "Do not use burfferoverflow risky function(%s)" % t.value)

[kunaltyagi]

Note: I'll take a look at the patch sometime next week. I'll be traveling soon and can't promise any action til next Wednesday at minimum.

the whole existence of RULE_10_1_A is all about quirks

That's a nice way (too nice) to look at it, but sadly, C doesn't consider them as quirks. It'd be easy to mark functions as deprecated, but C just wouldn't. And in turn we the programmers have to do "hacks" and tooling around the language.

it is one of the most useful rules

Thanks for your kind words. May I ask how you use this tool in your current setup?

[markgalassi]

You asked

May I ask how you use this tool in your current setup?

In Los Alamos we have a large simulation program related to space research. We have somewhere between 1/3 and 1/2 a million lines of C++ code. We are using nsiqcppstyle to enforce a uniform style, so that people joining the project don't get whiplash going from one chunk of code to another. We have found nsiqcppstyle quite valuable, although hard to pronounce when we talk about it :-)

[kunaltyagi]

Thanks for that. I've been struggling to see the scope of the project due to lack of ways to connect with the community.