baloghadamsoftware closed this revision.
baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added a comment.
https://reviews.llvm.org/rL318906 and https://reviews.llvm.org/rL318907
https://reviews.llvm.org/D39121
___
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Comment at: test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.c:1
+// RUN: %check_clang_tidy %s
baloghadamsoftware marked 12 inline comments as done.
baloghadamsoftware added inline comments.
Comment at: test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.c:1
+// RUN: %check_clang_tidy %s bugprone-misplaced-operator-in-strlen-in-alloc %t
+
baloghadamsoftware updated this revision to Diff 121690.
baloghadamsoftware added a comment.
Updated according to the comments.
https://reviews.llvm.org/D39121
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
aaron.ballman added a comment.
In https://reviews.llvm.org/D39121#915441, @baloghadamsoftware wrote:
> In https://reviews.llvm.org/D39121#915123, @aaron.ballman wrote:
>
> > Out of curiosity, were there any true positives, either?
>
>
> No, in a release version there should be no true positives
baloghadamsoftware added a comment.
In https://reviews.llvm.org/D39121#915123, @aaron.ballman wrote:
> Out of curiosity, were there any true positives, either?
No, in a release version there should be no true positives of this kind, I
think.
https://reviews.llvm.org/D39121
aaron.ballman added a comment.
In https://reviews.llvm.org/D39121#914871, @baloghadamsoftware wrote:
> I tested it on a C project (Postgres) and found no false positives.
Out of curiosity, were there any true positives, either?
https://reviews.llvm.org/D39121
baloghadamsoftware added a comment.
In https://reviews.llvm.org/D39121#914875, @Abpostelnicu wrote:
> I can test this on our repo, Mozilla, since it's a large code-base I think we
> you will have a better understanding of the false-positive ratio.
Thank you in advance!
Abpostelnicu added a comment.
I can test this on our repo, Mozilla, since it's a large code-base I think we
you will have a better understanding of the false-positive ratio.
https://reviews.llvm.org/D39121
___
cfe-commits mailing list
baloghadamsoftware added a comment.
I tested it on a C project (Postgres) and found no false positives.
https://reviews.llvm.org/D39121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
baloghadamsoftware added a comment.
OK, I will test it on some real code tomorrow, but today is a holiday here.
https://reviews.llvm.org/D39121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
baloghadamsoftware updated this revision to Diff 121102.
baloghadamsoftware added a comment.
Diagnostic message changed.
https://reviews.llvm.org/D39121
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
aaron.ballman added a comment.
In https://reviews.llvm.org/D39121#911745, @aaron.ballman wrote:
> Hmm, this is a good point -- I was thinking of the generic +N case with the
> original example, but with an explicit +1, you can't run into that situation
> with Win32 APIs. I will think on this a
aaron.ballman added a comment.
In https://reviews.llvm.org/D39121#911744, @baloghadamsoftware wrote:
> In https://reviews.llvm.org/D39121#911741, @aaron.ballman wrote:
>
> > As I pointed out earlier in the thread, it is common to have
> > double-null-terminated strings in Win32 APIs. This is a
baloghadamsoftware added a comment.
In https://reviews.llvm.org/D39121#911741, @aaron.ballman wrote:
> As I pointed out earlier in the thread, it is common to have
> double-null-terminated strings in Win32 APIs. This is a case where strlen(s +
> N) is valid. Since 1-byte strings would also be
aaron.ballman added a comment.
In https://reviews.llvm.org/D39121#911736, @baloghadamsoftware wrote:
> Can you give me please an example where `malloc(strlen(s+1))` is intentional.
> It allocates `2` byte less than needed to store `s`. If it is really the goal
> (e.g. `s` has a `2` character
baloghadamsoftware added a comment.
Can you give me please an example where `malloc(strlen(s+1))` is intentional.
It allocates `2` byte less than needed to store `s`. If it is really the goal
(e.g. `s` has a `2` character prefix which we do not want to copy) then the
correct solution is to use
aaron.ballman added a comment.
In https://reviews.llvm.org/D39121#910802, @baloghadamsoftware wrote:
> I thought on something like this, but I still do not like my phrasing:
>
> "Addition operator is applied to the argument of strlen(). instead of its
> result; move the '+ 1' outside of the
baloghadamsoftware added a comment.
I thought on something like this, but I still do not like my phrasing:
"Addition operator is applied to the argument of strlen(). instead of its
result; move the '+ 1' outside of the call. (Or, if it is intentional then
surround the addition subexpression
aaron.ballman added a comment.
In https://reviews.llvm.org/D39121#910735, @baloghadamsoftware wrote:
> In https://reviews.llvm.org/D39121#910715, @aaron.ballman wrote:
>
> > The diagnostic tells the user that you surround the arg to strlen with
> > parens to silence the diagnostic, but the
baloghadamsoftware added a comment.
In https://reviews.llvm.org/D39121#910715, @aaron.ballman wrote:
> The diagnostic tells the user that you surround the arg to strlen with parens
> to silence the diagnostic, but the fixit doesn't do that -- it moves the
> addition to the result. That's
aaron.ballman added a comment.
In https://reviews.llvm.org/D39121#910440, @baloghadamsoftware wrote:
> In https://reviews.llvm.org/D39121#909255, @aaron.ballman wrote:
>
> > My only remaining concern is with the diagnostic message/fixit interaction
> > itself. Let's see if @alexfh has any
baloghadamsoftware added a comment.
In https://reviews.llvm.org/D39121#909255, @aaron.ballman wrote:
> My only remaining concern is with the diagnostic message/fixit interaction
> itself. Let's see if @alexfh has any suggestions there, or we think of an
> improvement ourselves.
What do you
aaron.ballman added a comment.
My only remaining concern is with the diagnostic message/fixit interaction
itself. Let's see if @alexfh has any suggestions there, or we think of an
improvement ourselves.
https://reviews.llvm.org/D39121
___
baloghadamsoftware updated this revision to Diff 120593.
baloghadamsoftware added a comment.
Docs rephrased according to the comments.
https://reviews.llvm.org/D39121
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
aaron.ballman added a comment.
This upload looks considerably better, thank you!
Comment at: docs/ReleaseNotes.rst:63
+
+ Finds cases where ``1`` is added to the string in the parameter of
+ ``strlen()``, ``strnlen()``, ``strnlen_s()``, ``wcslen()``, ``wcsnlen()`` and
baloghadamsoftware updated this revision to Diff 120582.
baloghadamsoftware added a comment.
Second try to upload the correct diff...
https://reviews.llvm.org/D39121
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
baloghadamsoftware added a comment.
In https://reviews.llvm.org/D39121#909172, @aaron.ballman wrote:
> Many of the comments are marked done, but the code does not reflect that it
> actually is done, so I'm not certain what's happened there.
Neither do I. I will try to re-upload the diff.
aaron.ballman added a comment.
Many of the comments are marked done, but the code does not reflect that it
actually is done, so I'm not certain what's happened there.
Comment at: clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp:86
+ diag(Alloc->getLocStart(),
+
baloghadamsoftware updated this revision to Diff 120545.
baloghadamsoftware added a comment.
Updated according to the comments.
https://reviews.llvm.org/D39121
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
aaron.ballman added inline comments.
Comment at: clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp:64-67
+ const auto StrLenText = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(StrLen->getSourceRange()),
+ *Result.SourceManager, getLangOpts());
+
baloghadamsoftware updated this revision to Diff 120408.
baloghadamsoftware added a comment.
Updated according to comments.
https://reviews.llvm.org/D39121
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
aaron.ballman added a comment.
I'm still a bit concerned about how to silence this diagnostic if the code is
actually correct. Would it make sense to diagnose `malloc(strlen(s + 1))` but
silence the diagnostic if the argument to `strlen()` is explicitly
parenthesized? That means a user could
baloghadamsoftware added a comment.
In https://reviews.llvm.org/D39121#902145, @Eugene.Zelenko wrote:
> Same mistake could be made with new[] operator.
Yes! However, it seems that I need a new matcher for it so I would do it in a
follow-up patch.
https://reviews.llvm.org/D39121
baloghadamsoftware added a comment.
In https://reviews.llvm.org/D39121#902121, @martong wrote:
> Consider the use of a function pointer:
>
> void* malloc(int);
> int strlen(char*);
> auto fp = malloc;
> void bad_malloc(char *str) { char *c = (char *)fp(strlen(str + 1)); }
>
>
> I think,
baloghadamsoftware marked 4 inline comments as done.
baloghadamsoftware added a comment.
In https://reviews.llvm.org/D39121#902728, @alexfh wrote:
> Apart from all other comments, I think, this check would better be placed
> under bugprone/.
I moved it there.
https://reviews.llvm.org/D39121
baloghadamsoftware marked 5 inline comments as done.
baloghadamsoftware added inline comments.
Comment at: clang-tidy/misc/MisplacedOperatorInStrlenInAllocCheck.cpp:30
+ Finder->addMatcher(
+ callExpr(callee(functionDecl(hasName("malloc"))),
+ hasArgument(0,
baloghadamsoftware updated this revision to Diff 120214.
baloghadamsoftware added a comment.
Updated according to comments.
https://reviews.llvm.org/D39121
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
alexfh added a comment.
Apart from all other comments, I think, this check would better be placed under
bugprone/.
Repository:
rL LLVM
https://reviews.llvm.org/D39121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Eugene.Zelenko added a comment.
Same mistake could be made with new[] operator.
Comment at: docs/clang-tidy/checks/list.rst:10
android-cloexec-creat
+ android-cloexec-dup
android-cloexec-epoll-create
I think will be better to fix order of other
aaron.ballman added inline comments.
Comment at:
docs/clang-tidy/checks/misc-misplaced-operator-in-strlen-in-alloc.rst:16
+void bad_malloc(char *str) {
+ char *c = (char*) malloc(strlen(str + 1));
+}
xazax.hun wrote:
> What if this code is
martong added a comment.
We might get false positives in case of certain substring operations.
Consider the case of copying a substring, pseudo code below:
const char * s = "abcdefg";
int offset = my_find('d', s);
// I want to copy "defg"
char *new_subststring = (char*) malloc(strlen(s +
xazax.hun added inline comments.
Comment at: clang-tidy/misc/MisplacedOperatorInStrlenInAllocCheck.cpp:30
+ Finder->addMatcher(
+ callExpr(callee(functionDecl(hasName("malloc"))),
+ hasArgument(0, anyOf(hasDescendant(BadUse), BadUse)))
Maybe
martong added a comment.
Consider the use of a function pointer:
void* malloc(int);
int strlen(char*);
auto fp = malloc;
void bad_malloc(char *str) { char *c = (char *)fp(strlen(str + 1)); }
I think, the checker will not match in this case.
One might use allocation functions via a
baloghadamsoftware created this revision.
Herald added subscribers: mgorny, srhines.
A possible error is to write ``malloc(strlen(s+1))`` instead of
``malloc(strlen(s)+1)``. Unfortunately the former is also valid syntactically,
but allocates less memory by two bytes (if ``s`` is at least one
45 matches
Mail list logo
