[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:226
+    DiagnosticBuilder &Diag) {
+  if (getLangOpts().CPlusPlus11) {
+    StringRef NewFuncName = (Name[0] != 'w') ? "strncpy_s" : "wcsncpy_s";
----------------
Charusso wrote:
> aaron.ballman wrote:
> > Charusso wrote:
> > > aaron.ballman wrote:
> > > > What about C?
> > > The `else` part would fire.
> > I think this comment got moved to an unrelated area. I was talking about `NotNullTerminatedResultCheck::memmoveFix()`, where there is no `else` clause. However, I'm also not entirely certain why you are assuming the _s versions of those functions are available in C++ (they're in Annex K for C, which is an optional annex).
> The `else` clause is missing, according to the documentation: 
> 
> > - ``memmove``, ``wmemmove``:
> >   - C11: New function is ``memmove_s``/``wmemmove_s``, it has four arguments,
> >   - the new second argument is the first argument's length, and
> >   - the third argument will be moved as the fourth, where ``+ 1`` needed.
> > 
> >   - Before C11: The third argument gets a ``+ 1`` operation.
> 
> So the third argument as a token remains third if I add an extra second argument as a string, that's why it's work well on both options.
I don't think you should assume the _s versions are available in C++, because they're not available everywhere. In fact, they're not available everywhere in C either (for instance, there is no Annex K implementation in glibc).

My recommendation would be to either gate this on the presence of a definition of the `__STDC_LIB_EXT1__` macro and that the ` __STDC_WANT_LIB_EXT1__` macro is defined but not defined to 0, or to make it a configurable option. I have a slight preference for the config option because not all implementations implement all of Annex K but still have a reasonable _s implementation of the Annex K APIs (like Microsoft's implementation) so I can picture users wanting a way to opt-in even without the macros.


https://reviews.llvm.org/D45050