alexfh added inline comments.
Herald added a subscriber: jdoerfert.
Herald added a project: LLVM.
Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:31-33
+ static int ProtWrite;
+ static int ProtExec;
+ static int ProtRead;
Can these b
NoQ added a comment.
Sorry, busy days>< Done.
Repository:
rL LLVM
https://reviews.llvm.org/D42645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL326405: [analyzer] Add a checker for mmap()s which are both
writable and executable. (authored by dergachev, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://rev
devnexen added a comment.
ping :)
https://reviews.llvm.org/D42645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
Thank you! I'll try to commit again.
https://reviews.llvm.org/D42645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-
devnexen updated this revision to Diff 135258.
devnexen added a comment.
Rephrasing Checkers.td comment
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
devnexen added inline comments.
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:417-419
+// Operating systems specific PROT_READ/PROT_WRITE values is not implemented,
+// thus ought to be overriden with the proper analyser-config variables
+// remain in alpha until
emaste added inline comments.
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:417-419
+// Operating systems specific PROT_READ/PROT_WRITE values is not implemented,
+// thus ought to be overriden with the proper analyser-config variables
+// remain in alpha until th
devnexen added inline comments.
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:417-419
+// Operating systems specific PROT_READ/PROT_WRITE values is not implemented,
+// thus ought to be overriden with the proper analyser-config variables
+// remain in alpha until
emaste added inline comments.
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:417-419
+// Operating systems specific PROT_READ/PROT_WRITE values is not implemented,
+// thus ought to be overriden with the proper analyser-config variables
+// remain in alpha until th
a.sidorin accepted this revision.
a.sidorin added a comment.
Looks good to me as an 'alpha' checker. Thank you David!
I'd prefer this patch to be accepted by somebody else as well before committing
it.
https://reviews.llvm.org/D42645
___
cfe-commit
devnexen updated this revision to Diff 135067.
devnexen added a comment.
Updating tests accordingly
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
te
devnexen updated this revision to Diff 135027.
devnexen added a comment.
Moving back the checker to alpha.security level.
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWri
a.sidorin added a comment.
Hello David,
There are still some items for improvement. However, if we move this checker
into 'alpha' category, as Artem pointed, I think it can be accepted for merge
in its current state and improved later.
Repository:
rC Clang
https://reviews.llvm.org/D42645
NoQ added a comment.
I believe we should relocate this checker into `alpha.security` in order to
indicate that this is still in development, so that you (or anyone else) could
provide auto-detection for the macro values later as an incremental
improvement, and then it will be back in `security`
devnexen added a comment.
ping
Repository:
rC Clang
https://reviews.llvm.org/D42645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
devnexen updated this revision to Diff 133564.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
test/Analysis/mmap-writeexec.c
I
NoQ added a comment.
Hmm, maybe it'd also be a good idea to disable the check completely when a
likely-correct value for the macro cannot be determined.
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:74-75
+ mgr.registerChecker();
+ Mwec->ProtExecOv =
+
devnexen added a comment.
None of the possible solutions are ideal, but I think I chose the least complex
(e.g. via analyzer-config), less edgy one, and 4 is the most common value I ve
found so far for PROT_EXEC.
Repository:
rC Clang
https://reviews.llvm.org/D42645
__
devnexen updated this revision to Diff 133164.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
test/Analysis/mmap-writeexec.c
I
NoQ added a comment.
> What I propose is "two" separated checkers one using same but the value of
> PROT_EXEC will depend on the checker name e.g. security.MmapWriteExecGen vs
> security.MmapWriteExecGlibc ... or by default we keep PROT_EXEC as is and we
> would allow to override the value via
devnexen added a comment.
Repository:
rC Clang
https://reviews.llvm.org/D42645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
devnexen updated this revision to Diff 133010.
devnexen added a comment.
Will work on most modern Linux/Glibc versions, BSD variants and Illumos.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/Driver/ToolChains/FreeBSD.cp
devnexen updated this revision to Diff 133008.
devnexen added a comment.
Both Linux/Darwin unit tests passed.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/Driver/ToolChains/FreeBSD.cpp
lib/StaticAnalyzer/Checkers/CMak
devnexen updated this revision to Diff 132969.
Herald added a subscriber: emaste.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/Driver/ToolChains/FreeBSD.cpp
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyze
devnexen added a comment.
In https://reviews.llvm.org/D42645#998732, @a.sidorin wrote:
> Hello David,
>
> I have looked into mmap constant definitions in different implementations and
> found them pretty inconsistent. For example, MMAP_EXEC can be 0x01, 0x04 and
> I even found 0x00 in some file
a.sidorin added a comment.
Hello David,
I have looked into mmap constant definitions in different implementations and
found them pretty inconsistent. For example, MMAP_EXEC can be 0x01, 0x04 and I
even found 0x00 in some file
(https://www.cs.cmu.edu/~dga/crypto/priveth/libethash/mmap.h). There
devnexen updated this revision to Diff 132873.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
test/Analysis/mmap-writeexec.c
I
a.sidorin added a comment.
Hi David! The patch looks almost OK.
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:65
+ *BT, "Both PROT_WRITE and PROT_EXEC flags are set. This can "
+ "lead to exploitable memory regions, which could be overw
devnexen added a comment.
Correcting last typos in unit test.
Repository:
rC Clang
https://reviews.llvm.org/D42645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
devnexen updated this revision to Diff 132850.
devnexen edited the summary of this revision.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecC
devnexen updated this revision to Diff 132829.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
test/Analysis/mmap-writeexec.c
I
devnexen updated this revision to Diff 132826.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
test/Analysis/mmap-writeexec.c
I
devnexen added a comment.
Your remarks make sense. Ok will update the general "tone" accordingly.
Repository:
rC Clang
https://reviews.llvm.org/D42645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/l
jroelofs added inline comments.
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:399
+ def MmapWriteExecChecker : Checker<"MmapWriteExec">,
+HelpText<"Check if mmap() call is not both writable and executable">,
+DescFile<"MmapWriteExecChecker.cpp">;
devnexen added a comment.
In https://reviews.llvm.org/D42645#997062, @NoQ wrote:
> This failed on the buildbots (for example
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/14910 -
> committer gets notified of those), so i reverted it for now as r324167.
>
> I guess the reason is
devnexen updated this revision to Diff 132728.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
test/Analysis/mmap-writeexec.c
I
devnexen updated this revision to Diff 132727.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
test/Analysis/mmap-writeexec.c
I
NoQ added a comment.
This failed on the buildbots (for example
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/14910 - committer
gets notified of those), so i reverted it for now as r324167.
I guess the reason is that buildbots have different triples, and even though
you check the
This revision was automatically updated to reflect the committed changes.
Closed by commit rC324166: [analyzer] Add a checker for mmap()s which are both
writable and executable. (authored by dergachev, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D42645?vs=132715&id=132719#
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
Fantastic, thanks! I'll commit.
Repository:
rC Clang
https://reviews.llvm.org/D42645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://
devnexen updated this revision to Diff 132715.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
test/Analysis/mmap-writeexec.c
I
NoQ added a comment.
Wonderful! Could you also add a `// no-warning` kind of test, i.e. when the
respective flags are not set, no warning is emitted?
Repository:
rC Clang
https://reviews.llvm.org/D42645
___
cfe-commits mailing list
cfe-commits@l
devnexen added a comment.
In https://reviews.llvm.org/D42645#996668, @NoQ wrote:
> All right, so the code looks good now, but in order to commit this, we also
> need tests. We've got those automatic tests of ours in `test/Analysis/` that
> feed small code snippets into the analyzer and verify t
devnexen updated this revision to Diff 132713.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
test/Analysis/mmap-writeexec.c
I
NoQ added a comment.
All right, so the code looks good now, but in order to commit this, we also
need tests. We've got those automatic tests of ours in `test/Analysis/` that
feed small code snippets into the analyzer and verify that it does (or doesn't)
emit warnings in certain cases, so you'd
devnexen updated this revision to Diff 132578.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
Index: lib/StaticAnalyzer/Checkers
devnexen added a comment.
Updated. I ve tried (with few personal code) called as f/ptr, prots set via
variable as well. Might sounds obvious to you though this is my first
contribution to this :-) Any chance it get pushed ? Just asking I test
constantly/carry patches between 3 machines :-)
Re
devnexen updated this revision to Diff 132459.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
Index: lib/StaticAnalyzer/Checkers
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:62
+
+ ExplodedNode *N = C.generateErrorNode();
+ if (!N)
You should also use `generateNonFatalErrorNode()` here so that not to prevent
the analyzer from findin
devnexen added a comment.
Another example with pcre2, more specifically its JIT engine
In file included from
/home/dcarlier/Contribs/php-src/ext/pcre/pcre2lib/pcre2_jit_compile.c:78:
In file included from
/home/dcarlier/Contribs/php-src/ext/pcre/pcre2lib/sljit/sljitLir.c:261:
/home/dcarl
devnexen added a comment.
In https://reviews.llvm.org/D42645#990771, @a.sidorin wrote:
> Hello David,
>
> Do you have any results of this checker on the real code? If yes, could you
> please share them?
> There are also some inline comments regarding implementation.
I did a quick test on the
devnexen updated this revision to Diff 132054.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
Index: lib/StaticAnalyzer/Checkers
NoQ edited reviewers, added: NoQ; removed: dergachev.a.
NoQ added a comment.
Actually, most of our `security` package is off by default (in the driver) for
that very reason, so please never mind and just keep it in `security` :)
You can also put the checker straight into `security` bypassing `al
NoQ added a comment.
> `// It's somehow an optional checker since for example in JIT libraries it is
> pretty common.`
Dunno, it seems that most of our security checks are something like that, and
i'm not sure how to deal with it. This one sounds like it would probably go to
some sort of `opti
devnexen updated this revision to Diff 132024.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
Index: lib/StaticAnalyzer/Checkers
a.sidorin added a comment.
Hello David,
Do you have any results of this checker on the real code? If yes, could you
please share them?
There are also some inline comments regarding implementation.
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:42
+
+void Mma
jroelofs added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:1
+// MmapWriteExecChecker.cpp - Check for the prot argument
+//
jroelofs wrote:
> Needs one of these at the top:
>
> ```
> //===- MmapWriteExecChecker.cpp - Check t
devnexen added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:10
+//
+// This checker detects a common memory allocation security flaw.
+// Suppose 'unsigned int n' comes from an untrusted source. If the
jroelofs wrote:
> This c
devnexen updated this revision to Diff 131813.
Repository:
rC Clang
https://reviews.llvm.org/D42645
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
Index: lib/StaticAnalyzer/Checkers
jroelofs added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:1
+// MmapWriteExecChecker.cpp - Check for the prot argument
+//
Needs one of these at the top:
```
//===- MmapWriteExecChecker.cpp - Check the mmap prot argument
-
devnexen created this revision.
devnexen added reviewers: dcoughlin, dergachev.a.
devnexen created this object with visibility "All Users".
Herald added subscribers: cfe-commits, hintonda, mgorny.
This new checker tests if both PROT_WRITE and PROT_EXEC have been set.
Repository:
rC Clang
http
62 matches
Mail list logo
