[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
yawanng added a comment. https://reviews.llvm.org/rL310669 is the latest and complete version. It fixes an issue in windows platform. https://reviews.llvm.org/D35372 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
yawanng updated this revision to Diff 110606.
yawanng marked 3 inline comments as done.
https://reviews.llvm.org/D35372
Files:
clang-tidy/android/AndroidTidyModule.cpp
clang-tidy/android/CMakeLists.txt
clang-tidy/android/CloexecCheck.cpp
clang-tidy/android/CloexecCheck.h
clang-tidy/android/CloexecMemfdCreateCheck.cpp
clang-tidy/android/CloexecMemfdCreateCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/android-cloexec-memfd-create.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/android-cloexec-memfd-create.cpp
Index: test/clang-tidy/android-cloexec-memfd-create.cpp
===
--- /dev/null
+++ test/clang-tidy/android-cloexec-memfd-create.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s android-cloexec-memfd-create %t
+
+#define MFD_ALLOW_SEALING 1
+#define __O_CLOEXEC 3
+#define MFD_CLOEXEC __O_CLOEXEC
+#define TEMP_FAILURE_RETRY(exp) \
+ ({\
+int _rc;\
+do {\
+ _rc = (exp); \
+} while (_rc == -1);\
+ })
+#define NULL 0
+
+extern "C" int memfd_create(const char *name, unsigned int flags);
+
+void a() {
+ memfd_create(NULL, MFD_ALLOW_SEALING);
+ // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: 'memfd_create' should use MFD_CLOEXEC where possible [android-cloexec-memfd-create]
+ // CHECK-FIXES: memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC)
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+ // CHECK-MESSAGES: :[[@LINE-1]]:58: warning: 'memfd_create'
+ // CHECK-FIXES: TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC))
+}
+
+void f() {
+ memfd_create(NULL, 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'memfd_create'
+ // CHECK-FIXES: memfd_create(NULL, 3 | MFD_CLOEXEC)
+ TEMP_FAILURE_RETRY(memfd_create(NULL, 3));
+ // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: 'memfd_create'
+ // CHECK-FIXES: TEMP_FAILURE_RETRY(memfd_create(NULL, 3 | MFD_CLOEXEC))
+
+ int flag = 3;
+ memfd_create(NULL, flag);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, flag));
+}
+
+namespace i {
+int memfd_create(const char *name, unsigned int flags);
+
+void d() {
+ memfd_create(NULL, MFD_ALLOW_SEALING);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+}
+
+} // namespace i
+
+void e() {
+ memfd_create(NULL, MFD_CLOEXEC);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_CLOEXEC));
+ memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC));
+}
+
+class G {
+public:
+ int memfd_create(const char *name, unsigned int flags);
+ void d() {
+memfd_create(NULL, MFD_ALLOW_SEALING);
+TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+ }
+};
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -6,6 +6,7 @@
.. toctree::
android-cloexec-creat
android-cloexec-fopen
+ android-cloexec-memfd-create
android-cloexec-open
android-cloexec-socket
boost-use-to-string
Index: docs/clang-tidy/checks/android-cloexec-memfd-create.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/android-cloexec-memfd-create.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - android-cloexec-memfd-create
+
+android-cloexec-memfd-create
+
+
+``memfd_create()`` should include ``MFD_CLOEXEC`` in its type argument to avoid
+the file descriptor leakage. Without this flag, an opened sensitive file would
+remain open across a fork+exec to a lower-privileged SELinux domain.
+
+Examples:
+
+.. code-block:: c++
+
+ memfd_create(name, MFD_ALLOW_SEALING);
+
+ // becomes
+
+ memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -73,6 +73,12 @@
Checks if the required mode ``e`` exists in the mode argument of ``fopen()``.
+- New `android-cloexec-memfd_create
+ `_ check
+
+ Checks if the required file flag ``MFD_CLOEXEC`` is present in the argument of
+ ``memfd_create()``.
+
- New `android-cloexec-socket
`_ check
Index: clang-tidy/android/CloexecMemfdCreateCheck.h
===
--- /dev/null
+++ clang-tidy/android/CloexecMemfdCreateCheck.h
@@ -0,0 +1,35 @@
+//===--- CloexecMemfdCreateCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. LG with a few nits. Comment at: clang-tidy/android/CloexecCheck.cpp:49 + Finder->addMatcher( + ast_matchers::callExpr( + ast_matchers::callee( No need to qualify names in `ast_matchers::`, since there's a using directive above. Comment at: clang-tidy/android/CloexecCheck.h:38 + ast_matchers::internal::Matcher Function); + /// Currently, we have three types of fixes. + /// nit: Please add an empty line before this comment. Comment at: clang-tidy/android/CloexecCheck.h:54 + void insertMacroFlag(const ast_matchers::MatchFinder::MatchResult , + const StringRef MarcoFlag, const int ArgPos); + Please remove top-level const from the last two arguments. It has no effect in declaration (and definition can still use top-level const, if needed, since it is not a part of the function signature). Same below. https://reviews.llvm.org/D35372 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
yawanng updated this revision to Diff 110443.
yawanng marked 5 inline comments as done.
https://reviews.llvm.org/D35372
Files:
clang-tidy/android/AndroidTidyModule.cpp
clang-tidy/android/CMakeLists.txt
clang-tidy/android/CloexecCheck.cpp
clang-tidy/android/CloexecCheck.h
clang-tidy/android/CloexecMemfdCreateCheck.cpp
clang-tidy/android/CloexecMemfdCreateCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/android-cloexec-memfd-create.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/android-cloexec-memfd-create.cpp
Index: test/clang-tidy/android-cloexec-memfd-create.cpp
===
--- /dev/null
+++ test/clang-tidy/android-cloexec-memfd-create.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s android-cloexec-memfd-create %t
+
+#define MFD_ALLOW_SEALING 1
+#define __O_CLOEXEC 3
+#define MFD_CLOEXEC __O_CLOEXEC
+#define TEMP_FAILURE_RETRY(exp) \
+ ({\
+int _rc;\
+do {\
+ _rc = (exp); \
+} while (_rc == -1);\
+ })
+#define NULL 0
+
+extern "C" int memfd_create(const char *name, unsigned int flags);
+
+void a() {
+ memfd_create(NULL, MFD_ALLOW_SEALING);
+ // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: 'memfd_create' should use MFD_CLOEXEC where possible [android-cloexec-memfd-create]
+ // CHECK-FIXES: memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC)
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+ // CHECK-MESSAGES: :[[@LINE-1]]:58: warning: 'memfd_create'
+ // CHECK-FIXES: TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC))
+}
+
+void f() {
+ memfd_create(NULL, 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'memfd_create'
+ // CHECK-FIXES: memfd_create(NULL, 3 | MFD_CLOEXEC)
+ TEMP_FAILURE_RETRY(memfd_create(NULL, 3));
+ // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: 'memfd_create'
+ // CHECK-FIXES: TEMP_FAILURE_RETRY(memfd_create(NULL, 3 | MFD_CLOEXEC))
+
+ int flag = 3;
+ memfd_create(NULL, flag);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, flag));
+}
+
+namespace i {
+int memfd_create(const char *name, unsigned int flags);
+
+void d() {
+ memfd_create(NULL, MFD_ALLOW_SEALING);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+}
+
+} // namespace i
+
+void e() {
+ memfd_create(NULL, MFD_CLOEXEC);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_CLOEXEC));
+ memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC));
+}
+
+class G {
+public:
+ int memfd_create(const char *name, unsigned int flags);
+ void d() {
+memfd_create(NULL, MFD_ALLOW_SEALING);
+TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+ }
+};
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -6,6 +6,7 @@
.. toctree::
android-cloexec-creat
android-cloexec-fopen
+ android-cloexec-memfd-create
android-cloexec-open
android-cloexec-socket
boost-use-to-string
Index: docs/clang-tidy/checks/android-cloexec-memfd-create.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/android-cloexec-memfd-create.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - android-cloexec-memfd-create
+
+android-cloexec-memfd-create
+
+
+``memfd_create()`` should include ``MFD_CLOEXEC`` in its type argument to avoid
+the file descriptor leakage. Without this flag, an opened sensitive file would
+remain open across a fork+exec to a lower-privileged SELinux domain.
+
+Examples:
+
+.. code-block:: c++
+
+ memfd_create(name, MFD_ALLOW_SEALING);
+
+ // becomes
+
+ memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -73,6 +73,12 @@
Checks if the required mode ``e`` exists in the mode argument of ``fopen()``.
+- New `android-cloexec-memfd_create
+ `_ check
+
+ Checks if the required file flag ``MFD_CLOEXEC`` is present in the argument of
+ ``memfd_create()``.
+
- New `android-cloexec-socket
`_ check
Index: clang-tidy/android/CloexecMemfdCreateCheck.h
===
--- /dev/null
+++ clang-tidy/android/CloexecMemfdCreateCheck.h
@@ -0,0 +1,35 @@
+//===--- CloexecMemfdCreateCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
alexfh added a comment.
I'm not sure this approach is the best to deal with the boilerplate, but it
seems like an improvement compared to repeating code. See a few comments inline.
Comment at: clang-tidy/android/CloexecCheck.cpp:66
+
+StringRef CloexecCheck::getSpellingArg(const MatchFinder::MatchResult ,
+ int N) const {
1. Should this be a class method?
2. Is this ever used? (if it is going to be used in a different check, let's
postpone its addition to that moment)
Comment at: clang-tidy/android/CloexecCheck.h:43
+ast_matchers::callExpr(
+ast_matchers::callee(FuncDecl().bind(FuncDeclBindingStr)))
+.bind(FuncBindingStr),
Can we make this method non-template and put its implementation to the .cpp
file? It could accept a Matcher or something like that and just
apply it:
void registerForCall(MatchFinder *Finder, Matcher Function) {
Finder->addMatcher(calExpr(callee(functionDecl(isExternC(),
Function).bind(...))).bind(...), this);
}
Then you could make `FuncDeclBindingStr` and `FuncBindingStr` local to the file.
Comment at: clang-tidy/android/CloexecCheck.h:47
+ }
+ /// Currently, we has three types of fixes.
+ ///
s/we has/we have/
Comment at: clang-tidy/android/CloexecCheck.h:102-137
+ ast_matchers::internal::Matcher hasIntegerParameter(int N) {
+return ast_matchers::hasParameter(
+N, ast_matchers::hasType(ast_matchers::isInteger()));
+ }
+
+ ast_matchers::internal::Matcher
+ hasCharPointerTypeParameter(int N) {
These methods don't add much value: `hasParameter(N, hasType(isInteger()))` is
not much longer or harder to read than `hasIntegerParameter(N)`, etc. Also
having these utilities as non-static methods doesn't make much sense. If you
want a shorter way to describe function signatures, I would suggest a single
utility function `hasParameters()` that would take a list of type matchers and
apply them to the corresponding parameters. That one could be useful more
broadly than in this specific check, so it could be placed in utils/ (and later
in ASTMatchers.h, if we find enough potential users).
Comment at: clang-tidy/android/CloexecCheck.h:140
+private:
+ const static char *FuncDeclBindingStr;
+ const static char *FuncBindingStr;
You're missing one more const: `static const char * const FuncDeclBindingStr;`
https://reviews.llvm.org/D35372
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
yawanng added a comment. In https://reviews.llvm.org/D35372#834238, @hokein wrote: > Looks good to me, a few nits. Thanks for improving it continuously. > > I'd hold it for a while to see whether @alexfh has further comments before > submitting it. Thank you for the reviewing :-) https://reviews.llvm.org/D35372 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
yawanng updated this revision to Diff 110065.
yawanng marked 4 inline comments as done.
https://reviews.llvm.org/D35372
Files:
clang-tidy/android/AndroidTidyModule.cpp
clang-tidy/android/CMakeLists.txt
clang-tidy/android/CloexecCheck.cpp
clang-tidy/android/CloexecCheck.h
clang-tidy/android/CloexecMemfdCreateCheck.cpp
clang-tidy/android/CloexecMemfdCreateCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/android-cloexec-memfd-create.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/android-cloexec-memfd-create.cpp
Index: test/clang-tidy/android-cloexec-memfd-create.cpp
===
--- /dev/null
+++ test/clang-tidy/android-cloexec-memfd-create.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s android-cloexec-memfd-create %t
+
+#define MFD_ALLOW_SEALING 1
+#define __O_CLOEXEC 3
+#define MFD_CLOEXEC __O_CLOEXEC
+#define TEMP_FAILURE_RETRY(exp) \
+ ({\
+int _rc;\
+do {\
+ _rc = (exp); \
+} while (_rc == -1);\
+ })
+#define NULL 0
+
+extern "C" int memfd_create(const char *name, unsigned int flags);
+
+void a() {
+ memfd_create(NULL, MFD_ALLOW_SEALING);
+ // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: 'memfd_create' should use MFD_CLOEXEC where possible [android-cloexec-memfd-create]
+ // CHECK-FIXES: memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC)
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+ // CHECK-MESSAGES: :[[@LINE-1]]:58: warning: 'memfd_create'
+ // CHECK-FIXES: TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC))
+}
+
+void f() {
+ memfd_create(NULL, 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'memfd_create'
+ // CHECK-FIXES: memfd_create(NULL, 3 | MFD_CLOEXEC)
+ TEMP_FAILURE_RETRY(memfd_create(NULL, 3));
+ // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: 'memfd_create'
+ // CHECK-FIXES: TEMP_FAILURE_RETRY(memfd_create(NULL, 3 | MFD_CLOEXEC))
+
+ int flag = 3;
+ memfd_create(NULL, flag);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, flag));
+}
+
+namespace i {
+int memfd_create(const char *name, unsigned int flags);
+
+void d() {
+ memfd_create(NULL, MFD_ALLOW_SEALING);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+}
+
+} // namespace i
+
+void e() {
+ memfd_create(NULL, MFD_CLOEXEC);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_CLOEXEC));
+ memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC));
+}
+
+class G {
+public:
+ int memfd_create(const char *name, unsigned int flags);
+ void d() {
+memfd_create(NULL, MFD_ALLOW_SEALING);
+TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+ }
+};
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -6,6 +6,7 @@
.. toctree::
android-cloexec-creat
android-cloexec-fopen
+ android-cloexec-memfd-create
android-cloexec-open
android-cloexec-socket
boost-use-to-string
Index: docs/clang-tidy/checks/android-cloexec-memfd-create.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/android-cloexec-memfd-create.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - android-cloexec-memfd-create
+
+android-cloexec-memfd-create
+
+
+``memfd_create()`` should include ``MFD_CLOEXEC`` in its type argument to avoid
+the file descriptor leakage. Without this flag, an opened sensitive file would
+remain open across a fork+exec to a lower-privileged SELinux domain.
+
+Examples:
+
+.. code-block:: c++
+
+ memfd_create(name, MFD_ALLOW_SEALING);
+
+ // becomes
+
+ memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -73,6 +73,12 @@
Checks if the required mode ``e`` exists in the mode argument of ``fopen()``.
+- New `android-cloexec-memfd_create
+ `_ check
+
+ Checks if the required file flag ``MFD_CLOEXEC`` is present in the argument of
+ ``memfd_create()``.
+
- New `android-cloexec-socket
`_ check
Index: clang-tidy/android/CloexecMemfdCreateCheck.h
===
--- /dev/null
+++ clang-tidy/android/CloexecMemfdCreateCheck.h
@@ -0,0 +1,35 @@
+//===--- CloexecMemfdCreateCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
hokein accepted this revision.
hokein added a comment.
Looks good to me, a few nits. Thanks for improving it continuously.
I'd hold it for a while to see whether @alexfh has further comments before
submitting it.
Comment at: clang-tidy/android/CloexecCheck.h:28
+/// prevent the file descriptor leakage on fork+exec and this class provides
+/// utilities to identify and fix these C functions.
+class CloexecCheck : public ClangTidyCheck {
remove an extra blank before `and`.
Comment at: clang-tidy/android/CloexecCheck.h:45
+ /// open(file, O_RDONLY | O_CLOEXE);
+ /// \endcode
+ void insertMacroFlag(const ast_matchers::MatchFinder::MatchResult ,
Nit: you are missing the parameter part in the comment, the same below.
```
/// \param Result XXX
/// \param MacroFlag XXX
/// \param ArgPos
```
Comment at: clang-tidy/android/CloexecCheck.h:47
+ void insertMacroFlag(const ast_matchers::MatchFinder::MatchResult ,
+ const StringRef MarcoFlag, const int ArgPos);
+
Is the `ArgPos` 0-based or 1-based? I know it is 0-based, but we'd better
mention in the document part.
Comment at: clang-tidy/android/CloexecCheck.h:116
+ template
+ void registerMatchersImpl(ast_matchers::MatchFinder *Finder,
+Types... Params) {
nit: I'd put this method as the first protected method.
https://reviews.llvm.org/D35372
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
yawanng added inline comments. Comment at: clang-tidy/android/CloexecCheck.h:72 + StringRef getSpellingArg(const ast_matchers::MatchFinder::MatchResult , + int n); + hokein wrote: > `n` should be capital `N`. I think we can make it a const member method. And > I can't see any usage of this method in this patch. It's not used in this one, but in others. https://reviews.llvm.org/D35372 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
yawanng updated this revision to Diff 109778.
yawanng marked 13 inline comments as done.
https://reviews.llvm.org/D35372
Files:
clang-tidy/android/AndroidTidyModule.cpp
clang-tidy/android/CMakeLists.txt
clang-tidy/android/CloexecCheck.cpp
clang-tidy/android/CloexecCheck.h
clang-tidy/android/CloexecMemfdCreateCheck.cpp
clang-tidy/android/CloexecMemfdCreateCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/android-cloexec-memfd-create.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/android-cloexec-memfd-create.cpp
Index: test/clang-tidy/android-cloexec-memfd-create.cpp
===
--- /dev/null
+++ test/clang-tidy/android-cloexec-memfd-create.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s android-cloexec-memfd-create %t
+
+#define MFD_ALLOW_SEALING 1
+#define __O_CLOEXEC 3
+#define MFD_CLOEXEC __O_CLOEXEC
+#define TEMP_FAILURE_RETRY(exp) \
+ ({\
+int _rc;\
+do {\
+ _rc = (exp); \
+} while (_rc == -1);\
+ })
+#define NULL 0
+
+extern "C" int memfd_create(const char *name, unsigned int flags);
+
+void a() {
+ memfd_create(NULL, MFD_ALLOW_SEALING);
+ // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: 'memfd_create' should use MFD_CLOEXEC where possible [android-cloexec-memfd-create]
+ // CHECK-FIXES: memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC)
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+ // CHECK-MESSAGES: :[[@LINE-1]]:58: warning: 'memfd_create'
+ // CHECK-FIXES: TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC))
+}
+
+void f() {
+ memfd_create(NULL, 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'memfd_create'
+ // CHECK-FIXES: memfd_create(NULL, 3 | MFD_CLOEXEC)
+ TEMP_FAILURE_RETRY(memfd_create(NULL, 3));
+ // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: 'memfd_create'
+ // CHECK-FIXES: TEMP_FAILURE_RETRY(memfd_create(NULL, 3 | MFD_CLOEXEC))
+
+ int flag = 3;
+ memfd_create(NULL, flag);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, flag));
+}
+
+namespace i {
+int memfd_create(const char *name, unsigned int flags);
+
+void d() {
+ memfd_create(NULL, MFD_ALLOW_SEALING);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+}
+
+} // namespace i
+
+void e() {
+ memfd_create(NULL, MFD_CLOEXEC);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_CLOEXEC));
+ memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC));
+}
+
+class G {
+public:
+ int memfd_create(const char *name, unsigned int flags);
+ void d() {
+memfd_create(NULL, MFD_ALLOW_SEALING);
+TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+ }
+};
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -6,6 +6,7 @@
.. toctree::
android-cloexec-creat
android-cloexec-fopen
+ android-cloexec-memfd-create
android-cloexec-open
android-cloexec-socket
boost-use-to-string
Index: docs/clang-tidy/checks/android-cloexec-memfd-create.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/android-cloexec-memfd-create.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - android-cloexec-memfd-create
+
+android-cloexec-memfd-create
+
+
+``memfd_create()`` should include ``MFD_CLOEXEC`` in its type argument to avoid
+the file descriptor leakage. Without this flag, an opened sensitive file would
+remain open across a fork+exec to a lower-privileged SELinux domain.
+
+Examples:
+
+.. code-block:: c++
+
+ memfd_create(name, MFD_ALLOW_SEALING);
+
+ // becomes
+
+ memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -73,6 +73,12 @@
Checks if the required mode ``e`` exists in the mode argument of ``fopen()``.
+- New `android-cloexec-memfd_create
+ `_ check
+
+ Checks if the required file flag ``MFD_CLOEXEC`` is present in the argument of
+ ``memfd_create()``.
+
- New `android-cloexec-socket
`_ check
Index: clang-tidy/android/CloexecMemfdCreateCheck.h
===
--- /dev/null
+++ clang-tidy/android/CloexecMemfdCreateCheck.h
@@ -0,0 +1,35 @@
+//===--- CloexecMemfdCreateCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
hokein added inline comments.
Comment at: clang-tidy/android/CloexecCheck.cpp:66
+ // Check if the may be in the mode string.
+ const auto ModeStr = dyn_cast(ModeArg->IgnoreParenCasts());
+ if (!ModeStr || (ModeStr->getString().find(Mode) != StringRef::npos))
`const auto*`
Comment at: clang-tidy/android/CloexecCheck.h:11
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_H
+#include "../ClangTidy.h"
A blank line here.
Comment at: clang-tidy/android/CloexecCheck.h:23
+class CloexecCheck : public ClangTidyCheck {
+ const static constexpr char *FuncDeclBindingStr = "funcDecl";
+ const static constexpr char *FuncBindingStr = "func";
Normally, the style should like:
```
class Foo {
public:
...
protected:
...
privated:
...
};
```
BTW, in C++11 these are declarations, you still need to define them in the .cc
file like
```
const static constexpr char* CloexecCheck::FuncDeclBindingStr;
```
Comment at: clang-tidy/android/CloexecCheck.h:32
+ template
+ void registerMatchersImpl(ast_matchers::MatchFinder *Finder,
+Types... Params) {
I think this method (and below) can be `protected` member.
Comment at: clang-tidy/android/CloexecCheck.h:51
+ void insertMacroFlag(const ast_matchers::MatchFinder::MatchResult ,
+ const StringRef Flag, const int ArgPos);
+
I'd name it `MacroFlag`.
Comment at: clang-tidy/android/CloexecCheck.h:59
+ void replaceFunc(const ast_matchers::MatchFinder::MatchResult ,
+ StringRef Msg, StringRef FixMsg);
+
It is not obvious to see what these parameter mean. `Msg` is a message for
warning, maybe `WarningMsg`?
I'd suggest using the formal documentation for this class
(https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments).
Comment at: clang-tidy/android/CloexecCheck.h:63
+ // the flag is some string rather than a macro. For example, 'fopen' needs
+ // string 'e' in its mode argument, so the fix should be:
+ // fpen(in_file, "r");
`e` is not a string, is a char.
Comment at: clang-tidy/android/CloexecCheck.h:64
+ // string 'e' in its mode argument, so the fix should be:
+ // fpen(in_file, "r");
+ //^~~
s/fpen/fopen.
Comment at: clang-tidy/android/CloexecCheck.h:68
+ void insertStringFlag(const ast_matchers::MatchFinder::MatchResult ,
+const StringRef Mode, const int ArgPos);
+
Is `char` for `Mode` sufficient? Seems to me that only `fopen` check is using
it, or there will be more checks using this in the future?
Comment at: clang-tidy/android/CloexecCheck.h:72
+ StringRef getSpellingArg(const ast_matchers::MatchFinder::MatchResult
,
+ int n);
+
`n` should be capital `N`. I think we can make it a const member method. And I
can't see any usage of this method in this patch.
Comment at: clang-tidy/android/CloexecCheck.h:75
+ // Helper function to form the correct string mode for Type3.
+ std::string buildFixMsgForStringFlag(const Expr *Arg, const SourceManager
,
+ const LangOptions ,
Looks like this method is used internally, do we need to expose it? or just
define it as an anonymous function in the cc file?
Comment at: clang-tidy/android/CloexecCheck.h:79
+
+ inline ast_matchers::internal::Matcher
+ hasIntegerParameter(int N) {
The `inline` is redundant.
Comment at: clang-tidy/android/CloexecMemfdCreateCheck.cpp:24
+void CloexecMemfdCreateCheck::check(const MatchFinder::MatchResult ) {
+ insertMacroFlag(Result, "MFD_CLOEXEC", 1);
+}
nit: add `/*ArgPos=*/` before parameter 1 to make it more readable.
https://reviews.llvm.org/D35372
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
yawanng added inline comments.
Comment at: clang-tidy/android/CloexecCheck.h:35
+
+ // This issue has three types.
+ // Type1 is to insert the necessary macro flag in the flag argument.
hokein wrote:
> It is unclear to me what the "issue" means here? I assume there are three
> types of fixes?
Yes. It means three types of fixes :-)
Comment at: clang-tidy/android/CloexecCheck.h:36
+ // This issue has three types.
+ // Type1 is to insert the necessary macro flag in the flag argument.
+ void
hokein wrote:
> Deserve a document on the behavior of the method, would be better to give an
> example. The same below.
>
> Is `Flag` required to be a macro flag? looks like it could be any string.
Yes, the flag is expected to be a string of the required macro name.
Comment at: clang-tidy/android/CloexecCheck.h:75
+ inline ast_matchers::internal::Matcher
+ hasSockAddrPointerTypeParameter(int n) {
+return ast_matchers::hasParameter(
hokein wrote:
> should we put this method and below in the base class? They seem to be
> check-specific, I think?
Only the 'mode_t' one is currently only used by one check. The reason for
putting them all together here is to be kind of uniform and maybe facilitate
possible future checks that may contain this type.
https://reviews.llvm.org/D35372
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
yawanng updated this revision to Diff 109589.
yawanng marked 9 inline comments as done.
https://reviews.llvm.org/D35372
Files:
clang-tidy/android/AndroidTidyModule.cpp
clang-tidy/android/CMakeLists.txt
clang-tidy/android/CloexecCheck.cpp
clang-tidy/android/CloexecCheck.h
clang-tidy/android/CloexecMemfdCreateCheck.cpp
clang-tidy/android/CloexecMemfdCreateCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/android-cloexec-memfd-create.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/android-cloexec-memfd-create.cpp
Index: test/clang-tidy/android-cloexec-memfd-create.cpp
===
--- /dev/null
+++ test/clang-tidy/android-cloexec-memfd-create.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s android-cloexec-memfd-create %t
+
+#define MFD_ALLOW_SEALING 1
+#define __O_CLOEXEC 3
+#define MFD_CLOEXEC __O_CLOEXEC
+#define TEMP_FAILURE_RETRY(exp) \
+ ({\
+int _rc;\
+do {\
+ _rc = (exp); \
+} while (_rc == -1);\
+ })
+#define NULL 0
+
+extern "C" int memfd_create(const char *name, unsigned int flags);
+
+void a() {
+ memfd_create(NULL, MFD_ALLOW_SEALING);
+ // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: 'memfd_create' should use MFD_CLOEXEC where possible [android-cloexec-memfd-create]
+ // CHECK-FIXES: memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC)
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+ // CHECK-MESSAGES: :[[@LINE-1]]:58: warning: 'memfd_create'
+ // CHECK-FIXES: TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC))
+}
+
+void f() {
+ memfd_create(NULL, 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'memfd_create'
+ // CHECK-FIXES: memfd_create(NULL, 3 | MFD_CLOEXEC)
+ TEMP_FAILURE_RETRY(memfd_create(NULL, 3));
+ // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: 'memfd_create'
+ // CHECK-FIXES: TEMP_FAILURE_RETRY(memfd_create(NULL, 3 | MFD_CLOEXEC))
+
+ int flag = 3;
+ memfd_create(NULL, flag);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, flag));
+}
+
+namespace i {
+int memfd_create(const char *name, unsigned int flags);
+
+void d() {
+ memfd_create(NULL, MFD_ALLOW_SEALING);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+}
+
+} // namespace i
+
+void e() {
+ memfd_create(NULL, MFD_CLOEXEC);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_CLOEXEC));
+ memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC);
+ TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING | MFD_CLOEXEC));
+}
+
+class G {
+public:
+ int memfd_create(const char *name, unsigned int flags);
+ void d() {
+memfd_create(NULL, MFD_ALLOW_SEALING);
+TEMP_FAILURE_RETRY(memfd_create(NULL, MFD_ALLOW_SEALING));
+ }
+};
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -6,6 +6,7 @@
.. toctree::
android-cloexec-creat
android-cloexec-fopen
+ android-cloexec-memfd-create
android-cloexec-open
android-cloexec-socket
boost-use-to-string
Index: docs/clang-tidy/checks/android-cloexec-memfd-create.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/android-cloexec-memfd-create.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - android-cloexec-memfd-create
+
+android-cloexec-memfd-create
+
+
+``memfd_create()`` should include ``MFD_CLOEXEC`` in its type argument to avoid
+the file descriptor leakage. Without this flag, an opened sensitive file would
+remain open across a fork+exec to a lower-privileged SELinux domain.
+
+Examples:
+
+.. code-block:: c++
+
+ memfd_create(name, MFD_ALLOW_SEALING);
+
+ // becomes
+
+ memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -73,6 +73,12 @@
Checks if the required mode ``e`` exists in the mode argument of ``fopen()``.
+- New `android-cloexec-memfd_create
+ `_ check
+
+ Checks if the required file flag ``MFD_CLOEXEC`` is present in the argument of
+ ``memfd_create()``.
+
- New `android-cloexec-socket
`_ check
Index: clang-tidy/android/CloexecMemfdCreateCheck.h
===
--- /dev/null
+++ clang-tidy/android/CloexecMemfdCreateCheck.h
@@ -0,0 +1,35 @@
+//===--- CloexecMemfdCreateCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
hokein added a comment.
Sorry for the delay, and thanks for refining it.
In general, I'm fine with the current design, a few comments below.
Comment at: clang-tidy/android/CloexecCheck.cpp:62
+const int ArgPos) {
+ const auto MatchedCall = Result.Nodes.getNodeAs("func");
+ const auto FD = Result.Nodes.getNodeAs("funcDecl");
Use `const auto*`, obviously indicates this is a pointer, the same to other
places.
Comment at: clang-tidy/android/CloexecCheck.cpp:94
+ StringRef SR = cast(Arg->IgnoreParenCasts())->getString();
+ return ("\"" + SR + Twine(Mode) + "\"").str();
+}
I think there will be a use-after-free issue here.
The method returns a StringRef (which doesn't own the "std::string"), the
temporary `std::string` will be destructed after the method finished.
Comment at: clang-tidy/android/CloexecCheck.h:18
+
+/// The base class for all close-on-exec checks.
+class CloexecCheck : public ClangTidyCheck {
This base class will be used in a bunch of cloase-on-exec checks.
It deserves a better and more detailed documentation (e.g. the intended usage).
Comment at: clang-tidy/android/CloexecCheck.h:26
+ template
+ void doRegisterMatchers(ast_matchers::MatchFinder *Finder, Types... Params) {
+auto FuncDecl = std::bind(ast_matchers::functionDecl,
I think we can drop the `do` for most methods, how about
`doRegisterMatchers` => `registerMatchers`
`doMacroFlagInsertion` => `insertMacroFlag`
`doFuncReplacement` => `replaceFunc`
`doStringFlagInsertion` => `insertStringFlag`?
Comment at: clang-tidy/android/CloexecCheck.h:30
+Finder->addMatcher(ast_matchers::callExpr(
+ ast_matchers::callee(FuncDecl().bind("funcDecl")))
+ .bind("func"),
Instead of using the magic string `funDecl` anywhere, I would define a static
const member for it. The same to `func`.
Comment at: clang-tidy/android/CloexecCheck.h:35
+
+ // This issue has three types.
+ // Type1 is to insert the necessary macro flag in the flag argument.
It is unclear to me what the "issue" means here? I assume there are three types
of fixes?
Comment at: clang-tidy/android/CloexecCheck.h:36
+ // This issue has three types.
+ // Type1 is to insert the necessary macro flag in the flag argument.
+ void
Deserve a document on the behavior of the method, would be better to give an
example. The same below.
Is `Flag` required to be a macro flag? looks like it could be any string.
Comment at: clang-tidy/android/CloexecCheck.h:53
+ // Helper function to get the spelling of a particular argument.
+ StringRef getArgSpelling(const ast_matchers::MatchFinder::MatchResult
,
+ int n);
Maybe getSpellingArg which sounds more natural?
Comment at: clang-tidy/android/CloexecCheck.h:54
+ StringRef getArgSpelling(const ast_matchers::MatchFinder::MatchResult
,
+ int n);
+
s/n/N
Comment at: clang-tidy/android/CloexecCheck.h:75
+ inline ast_matchers::internal::Matcher
+ hasSockAddrPointerTypeParameter(int n) {
+return ast_matchers::hasParameter(
should we put this method and below in the base class? They seem to be
check-specific, I think?
https://reviews.llvm.org/D35372
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
yawanng added a comment. Ping. https://reviews.llvm.org/D35372 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
