Page MenuHomePhabricator

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

Authored by jcai19 on May 16 2019, 5:52 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jcai19 created this revision.May 16 2019, 5:52 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/android/CloexecPipe2Check.h
1 ↗(On Diff #199951)

Please extend to 80 characters. Same in source file.

Eugene.Zelenko added a project: Restricted Project.
jcai19 updated this revision to Diff 200066.May 17 2019, 11:09 AM

Update based on comments.

jcai19 marked an inline comment as done.May 17 2019, 11:09 AM
jcai19 added a reviewer: chh.May 20 2019, 4:52 PM
srhines added inline comments.May 21 2019, 1:00 AM
clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe2.rst
19 ↗(On Diff #200066)

Shouldn't this be "O_NONBLOCK | O_CLOEXEC" instead? Why drop the O_NONBLOCK?

jcai19 updated this revision to Diff 200547.May 21 2019, 11:05 AM

Fix a typo.

jcai19 marked 2 inline comments as done.May 21 2019, 11:06 AM
jcai19 added inline comments.
clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe2.rst
19 ↗(On Diff #200066)

Good catch!

srhines added inline comments.May 21 2019, 11:21 AM
clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
52 ↗(On Diff #200066)

I'm not all that familiar with writing clang-tidy-specific tests, but should these tests here denote that a diagnostic should NOT be issued? That is usually the convention in regular Clang tests, so I assume the test runner here should be equally supportive of ensuring that the contents passed through without any other diagnostics related to pipe2 and/or O_CLOEXEC.

jcai19 marked 2 inline comments as done.May 21 2019, 11:43 AM
jcai19 added inline comments.
clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
52 ↗(On Diff #200066)

That makes sense, and I have seem tests for similar checks with (e.g. android-cloexec-open) and without (e.g. android-cloexec-accep4 and android-cloexec-socket) additional CHECK-MESSAGES-NOT check. But based on the Testing Checks section of https://clang.llvm.org/extra/clang-tidy/Contributing.html, it seems typically CHECK-MASSAGES and CHECK-FIXES are sufficient for clang-tidy checks. Please let me know what you think.

srhines added inline comments.May 23 2019, 11:06 AM
clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
52 ↗(On Diff #200066)

If you look in test/clang-tidy/android-cloexec-creat.cpp, you will see that there are "CHECK-MESSAGES-NOT" checks that ensure the diagnostic is not issued in correct cases. You can put the checks on lines 39, 58, and 67, which will ensure that there are no additional diagnostics being generated.

jcai19 updated this revision to Diff 202030.May 29 2019, 1:00 PM

Add CHECK-MESSAGES-NOT checks.

jcai19 marked 2 inline comments as done.May 29 2019, 1:01 PM
jcai19 added inline comments.
clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
52 ↗(On Diff #200066)

Sounds good! Thanks for the reference.

srhines added inline comments.May 29 2019, 3:09 PM
clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
48 ↗(On Diff #202030)

Much like line 39 (which covers both lines 37 and 38), you can delete this CHECK-MESSAGES-NOT. The one on line 50 will cover both of these.

64 ↗(On Diff #202030)

Only keep this CHECK-MESSAGES-NOT line.

75 ↗(On Diff #202030)

Only keep this CHECK-MESSAGES-NOT line.

jcai19 updated this revision to Diff 202072.May 29 2019, 3:52 PM
jcai19 marked an inline comment as done.

Remove redundant CHECK-MESSAGES-NOT checks.

jcai19 marked 3 inline comments as done.May 29 2019, 3:53 PM
srhines accepted this revision.May 29 2019, 3:59 PM

Everything looks great. Thanks for adding these improvements. While it's probably safe to commit this, perhaps you should give it 24 hours in case some of the other clang-tidy folks have different style or testing concerns.

This revision is now accepted and ready to land.May 29 2019, 3:59 PM

Everything looks great. Thanks for adding these improvements. While it's probably safe to commit this, perhaps you should give it 24 hours in case some of the other clang-tidy folks have different style or testing concerns.

Sounds good! Thanks for all the comments.

hokein accepted this revision.May 31 2019, 2:26 AM

LGTM.

clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
60 ↗(On Diff #202072)

no need to verify it explicitly.

gribozavr requested changes to this revision.May 31 2019, 10:01 AM
gribozavr added a subscriber: gribozavr.
gribozavr added inline comments.
clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe2.rst
6 ↗(On Diff #202072)

This checks ensures that pipe2() is called with the O_CLOEXEC flag. This flag helps to avoid accidentally leaking file descriptors to child processes.

17 ↗(On Diff #202072)

"Suggested replacement"

Also use a separate code block for the replacement.

clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
20 ↗(On Diff #202072)

Same comment about the message as in D61967 -- the message should briefly explain why the user should make this change. Users tend to ignore warnings they don't understand.

"pipe2 should be called with O_CLOEXEC to avoid leaking file descriptors to child processes"

27 ↗(On Diff #202072)

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

54 ↗(On Diff #202072)

How is e different from a?

48 ↗(On Diff #202030)

Not done -- you don't need any CHECK-MESSAGES-NOT at all.

This revision now requires changes to proceed.May 31 2019, 10:01 AM
srhines added inline comments.May 31 2019, 1:07 PM
clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
54 ↗(On Diff #202072)

e uses O_CLOEXEC properly with pipe2() and makes sure that we don't issue additional diagnostics/fixes that are unnecessary.

gribozavr added inline comments.May 31 2019, 3:05 PM
clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
54 ↗(On Diff #202072)

Thanks -- I see it now. I think a descriptive test name would help here.

jcai19 updated this revision to Diff 202830.Jun 3 2019, 5:29 PM

Update description based on comments and remove CHECK-MESSAGES-NOT

jcai19 marked 4 inline comments as done.Jun 3 2019, 5:42 PM
jcai19 added inline comments.
clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
20 ↗(On Diff #202072)

It appeared to me that there was no easy way to change the warning message due to the way insertMacroFlag is implemented (called in CloexecPipe2Check::check(...) to add O_CLOEXEC flag when necessary). The function always issue a warning in the format of "%0 should use %1 where possible". So unless we parameterize the warning string (which could be solved by a different code change), we might end up implementing a similar function with different warning message, which creates extra work if we need to modify insertMacroFlag in the future. I am new to Clang so please let me know if there is a better way to udpate the warning message.

jcai19 updated this revision to Diff 202832.Jun 3 2019, 5:51 PM

Fix a typo.

gribozavr added inline comments.Jun 4 2019, 1:46 AM
clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
17 ↗(On Diff #202832)

warning1, warning2 is better than a and b, but we can do even better.

warningForPipeCall
warningForPipeCallInMacroArgument

Similarly in the rest of the patch.

20 ↗(On Diff #202072)

I don't see an issue with parameterizing insertMacroFlag -- the current message that it produces is not descriptive enough anyway, users tend to ignore such messages.

jcai19 marked an inline comment as done.Jun 4 2019, 10:19 AM
jcai19 added inline comments.
clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
20 ↗(On Diff #202072)

I agree. But my point is insertMacroFlag is used by several other checks. Shouldn't we parameterize the warning message and update these checks together in a separate change?

gribozavr marked an inline comment as done.Jun 4 2019, 2:58 PM
gribozavr added inline comments.
clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
20 ↗(On Diff #202072)

SGTM!

jcai19 updated this revision to Diff 203050.Jun 4 2019, 5:19 PM

Update test function names.

jcai19 marked an inline comment as done.Jun 4 2019, 5:24 PM
jcai19 added inline comments.
clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp
17 ↗(On Diff #202832)

Sounds good. I have updated the function names, although I feel the "ForPipeCall" part is already implied in the test so I did not include it to keep the names shorter. Let me know if you think it is better to have it included.

gribozavr accepted this revision.Jun 5 2019, 12:29 AM
This revision is now accepted and ready to land.Jun 5 2019, 12:29 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2019, 10:19 PM