Page MenuHomePhabricator

[clang-tidy] Add a close-on-exec check on pipe() in Android module.
ClosedPublic

Authored by jcai19 on May 15 2019, 3:43 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jcai19 created this revision.May 15 2019, 3:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2019, 3:43 PM

Thanks for this!

I don't have great context on tidy, so I can't stamp this, but I do have a few drive-by nits for you.

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp
22 ↗(On Diff #199692)

We probably don't want to commit commented out code

27 ↗(On Diff #199692)

simplicity nit: can this be a std::string?

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h
18 ↗(On Diff #199692)

nit: should probably update this comment

clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp
9 ↗(On Diff #199692)

(Do we have a CHECK-FIXES-NOT or CHECK-MESSAGES-NOT to apply to the things below? Or are the CHECKs here meant to be complete like clang's -verify?)

Eugene.Zelenko added inline comments.
clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst
6 ↗(On Diff #199692)

Please make first sentence same as in Release Notes.

8 ↗(On Diff #199692)

I think will be good idea to highlight fork and exec with double back-ticks.

Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added inline comments.
clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst
4 ↗(On Diff #199692)

Please make same length as name above.

jcai19 updated this revision to Diff 199861.May 16 2019, 11:16 AM

Add fixes based on comments

jcai19 updated this revision to Diff 199870.May 16 2019, 11:34 AM
jcai19 marked 4 inline comments as done.

Fix format

jcai19 marked 4 inline comments as done.May 16 2019, 11:35 AM
jcai19 added inline comments.
clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp
27 ↗(On Diff #199692)

replaceFunc takes a llvm::StringRef as the third parameter. StringRef is "expected to be used in situations where the character data resides in some other buffer, whose lifetime extends past that of the StringRef", which is true in this case, so I think it should be fine.

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h
18 ↗(On Diff #199692)

Good catch!

clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst
4 ↗(On Diff #199692)

Thanks for the comments. I have fixed them accordingly.

jcai19 marked 2 inline comments as done.May 16 2019, 12:01 PM
jcai19 added inline comments.
clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp
9 ↗(On Diff #199692)

Based on Testing Checks https://clang.llvm.org/extra/clang-tidy/Contributing.html, CHECK-MASSAGES and CHECK-FIXES are sufficient for clang-tidy checks. I have double checked the tests for similar checks and they don't seem to have additional FileCheck invocations other than these two.

srhines added a subscriber: jmgao.May 16 2019, 1:14 PM
clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp
27 ↗(On Diff #199692)

Both should be functionally equivalent in this code. My point was that it's not common to rely on temporary lifetime extension when writing the non-const& type would be equivalent (barring maybe cases with auto, but that's not applicable), and I don't see why we should break with that here.

jcai19 updated this revision to Diff 199926.May 16 2019, 4:03 PM

Use non-const& type for readbility.

jcai19 marked 2 inline comments as done.May 16 2019, 4:04 PM
jcai19 added inline comments.
clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp
27 ↗(On Diff #199692)

Make sense.

Eugene.Zelenko added inline comments.May 16 2019, 6:23 PM
clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h
1 ↗(On Diff #199926)

Please extend to 80 characters. Same in source file.

jcai19 updated this revision to Diff 200068.May 17 2019, 11:11 AM
jcai19 marked an inline comment as done.

Update based on comments.

jcai19 marked an inline comment as done.May 17 2019, 11:12 AM
jcai19 updated this revision to Diff 202052.Wed, May 29, 1:59 PM

Add CHECK-MESSAGES-NOT checks.

hokein accepted this revision.Fri, May 31, 2:23 AM
hokein added inline comments.
clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp
31 ↗(On Diff #202052)

the message doesn't seem to explain the reason, especially to the people who are not familiar with the pipe API, maybe prefer pipe2() to pipe() to avoid file descriptor leakage is clearer?

Ah, it looks like you are following the existing CloexecCreatCheck check, no need to do it in this patch.

clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp
17 ↗(On Diff #202052)

nit: no need to do it explicitly, if a warning is shown unexpectedly, the test will fail.

This revision is now accepted and ready to land.Fri, May 31, 2:23 AM
gribozavr added inline comments.
clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp
31 ↗(On Diff #202052)

"prefer pipe2() with O_CLOEXEC to avoid leaking file descriptors to child processes"

No need to change in this patch if you're following an existing pattern, but please do update the message in all checks in a separate patch to explain things better.

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h
18 ↗(On Diff #202052)

"Suggests to replace calls to pipe() with calls to pipe2()."

clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst
6 ↗(On Diff #202052)

WDYT about this rewrite?

"""
This check detects usage of pipe(). Using pipe() is not recommended, pipe2() is the suggested replacement.

The check also adds the O_CLOEXEC flag that marks the file descriptor to be closed in child processes. Without this flag a sensitive file descriptor can be leaked to a child process, potentially into a lower-privileged SELinux domain.
"""

16 ↗(On Diff #202052)

"Suggested replacement"

Also use a separate code block for the replacement.

gribozavr requested changes to this revision.Fri, May 31, 9:50 AM
gribozavr edited reviewers, added: gribozavr; removed: alexfh.
This revision now requires changes to proceed.Fri, May 31, 9:50 AM
srhines added inline comments.Fri, May 31, 1:04 PM
clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp
17 ↗(On Diff #202052)

I somehow never realized this (and there seem to be several places that also don't realize it). Thanks for letting us know.

jcai19 updated this revision to Diff 202625.Sun, Jun 2, 3:38 PM
jcai19 marked an inline comment as done.

Remove CHECK-MESSAGES-NOT and update description based on comments

jcai19 marked 6 inline comments as done.Sun, Jun 2, 3:41 PM
jcai19 added inline comments.
clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp
31 ↗(On Diff #202052)

Thanks for the comments. I have updated the text.

clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp
17 ↗(On Diff #202052)

Thanks for the clarification.

gribozavr accepted this revision.Mon, Jun 3, 1:05 AM
gribozavr added inline comments.
clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp
5 ↗(On Diff #202052)

Please give the tests informative names instead of f, g etc.

This revision is now accepted and ready to land.Mon, Jun 3, 1:05 AM
jcai19 updated this revision to Diff 202835.Mon, Jun 3, 6:15 PM

Update the test function names.

jcai19 marked an inline comment as done.Mon, Jun 3, 6:17 PM

Thanks for all the comments. george.burgess.iv will submit this change for me.

Will submit once gribozavr indicates that they're happy with the new test names. Thanks again for working on this!

gribozavr accepted this revision.Tue, Jun 4, 1:40 AM
gribozavr added inline comments.
clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp
14 ↗(On Diff #202835)

noWarningForPipeInNamespace ?

23 ↗(On Diff #202835)

noWarningForPipeMemberFunction ?

jcai19 updated this revision to Diff 202984.Tue, Jun 4, 10:53 AM

Update test function names.

jcai19 marked 2 inline comments as done.Tue, Jun 4, 10:54 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jun 5, 10:19 PM