Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding process_open_pipes table #6142

Merged
merged 8 commits into from
Jan 17, 2020
Merged

Conversation

scoders-tob
Copy link
Contributor

This table lists a process' open named and anonymous pipes with endpoints resolved.
Example output:

osquery> select * from process_open_pipes limit 20;
+------+-----+------+-------+-----------+-------------+------------+--------------+
| pid  | fd  | mode | inode | type      | partner_pid | partner_fd | partner_mode |
+------+-----+------+-------+-----------+-------------+------------+--------------+
| 1    | 220 | rw   | 407   | named     |             |            |              |
| 1    | 221 | rw   | 408   | named     |             |            |              |
| 1    | 225 | rw   | 380   | named     |             |            |              |
| 1    | 250 | r    | 19564 | anonymous |             |            |              |
| 1057 | 21  | r    | 559   | named     | 1060        | 9          | w            |
| 1057 | 22  | r    | 1035  | named     | 1259        | 8          | w            |
| 1057 | 23  | r    | 1032  | named     | 1266        | 12         | w            |
| 1057 | 25  | r    | 1016  | named     | 1107        | 20         | w            |
| 1057 | 33  | r    | 1192  | named     | 3185        | 14         | w            |
| 1057 | 34  | r    | 1109  | named     | 1367        | 7          | w            |
| 1057 | 35  | r    | 1202  | named     | 1630        | 21         | w            |
| 1057 | 36  | r    | 1204  | named     | 3185        | 31         | w            |
| 1057 | 37  | r    | 1208  | named     | 1618        | 19         | w            |
| 1057 | 38  | r    | 1213  | named     | 3132        | 20         | w            |
| 1057 | 42  | r    | 1122  | named     | 2988        | 41         | w            |
| 1057 | 48  | r    | 1099  | named     | 1618        | 14         | w            |
| 1057 | 52  | r    | 965   | named     | 2036        | 13         | w            |
| 1058 | 6   | r    | 28513 | anonymous | 1058        | 7          | w            |
| 1058 | 7   | w    | 28513 | anonymous | 1058        | 6          | r            |
| 1058 | 8   | r    | 28515 | anonymous | 1058        | 9          | w            |
+------+-----+------+-------+-----------+-------------+------------+--------------+

@alessandrogario alessandrogario added feature Linux events Related to osquery's evented tables or eventing subsystem and removed events Related to osquery's evented tables or eventing subsystem labels Dec 21, 2019
Copy link
Member

@theopolis theopolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few notes throughout:

  • Please use explicit {} around new scopes to help prevent future mistakes.
  • In some places we should use C++11 patterns instead of C, no raw pointers or C arrays.
  • Logging does not need ending newlines and we should only use DLOG or LOG, not perror or printf.
  • Check if we need to update BUCK as well.
  • Should we add optimization to the implementation to be able to handle pid = N?

osquery/tables/system/linux/process_open_pipes.cpp Outdated Show resolved Hide resolved
osquery/tables/system/linux/process_open_pipes.cpp Outdated Show resolved Hide resolved
osquery/tables/system/linux/process_open_pipes.cpp Outdated Show resolved Hide resolved
osquery/tables/system/linux/process_open_pipes.cpp Outdated Show resolved Hide resolved
osquery/tables/system/linux/process_open_pipes.cpp Outdated Show resolved Hide resolved
osquery/tables/system/linux/process_open_pipes.cpp Outdated Show resolved Hide resolved
tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
@Smjert
Copy link
Member

Smjert commented Dec 24, 2019

Sorry if I'm intruding, but a couple of comments on the review:

  1. C++11 smart pointers intention is to substitute owning raw pointers, not any raw pointer. If the pointer points to a resource that is held by a smart pointer, which is the intention here, that's fine.
    Which is also why this in the works: https://en.cppreference.com/w/cpp/experimental/observer_ptr
    Nonetheless nullability might not be desired here.

  2. const references cannot be stored into a vector because vector elements must be assignable. Maybe std::reference_wrapper?

@scoders-tob
Copy link
Contributor Author

Thanks guys for your reviews. I've made the suggested changes.

@scoders-tob
Copy link
Contributor Author

Regarding optimization to handle pid = N, this won't be useful because all processes need to be traversed regardless to resolve pipe partners

Copy link
Member

@theopolis theopolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the iteration here! Do you mind taking another pass at the testing code? I left a few comments but there might be more that can be improved.

tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
@scoders-tob
Copy link
Contributor Author

Thanks @theopolis. Will do.

@scoders-tob
Copy link
Contributor Author

Done!

Copy link
Member

@theopolis theopolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks for the updates! I promise this is my last round of nitpicks 💃

Also curious if @Smjert can take another look?

tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
tests/integration/tables/process_open_pipes.cpp Outdated Show resolved Hide resolved
@scoders-tob
Copy link
Contributor Author

:)
Done!

Smjert
Smjert previously requested changes Jan 3, 2020
Copy link
Member

@Smjert Smjert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also keep the same naming convention we have for function in the test case.

osquery/tables/system/linux/process_open_pipes.cpp Outdated Show resolved Hide resolved
osquery/tables/system/linux/process_open_pipes.cpp Outdated Show resolved Hide resolved
osquery/tables/system/linux/process_open_pipes.cpp Outdated Show resolved Hide resolved
osquery/tables/system/linux/process_open_pipes.cpp Outdated Show resolved Hide resolved
osquery/tables/system/linux/process_open_pipes.cpp Outdated Show resolved Hide resolved
@scoders-tob
Copy link
Contributor Author

Thanks @Smjert! Done.

@scoders-tob
Copy link
Contributor Author

@theopolis All changes have been made to this as per reviews. If it looks good to you, could you please approve/merge this?

@theopolis theopolis merged commit b150367 into osquery:master Jan 17, 2020
@scoders-tob
Copy link
Contributor Author

Thanks!

@mike-myers-tob mike-myers-tob deleted the pipestable branch May 7, 2020 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants