libs icon indicating copy to clipboard operation
libs copied to clipboard

wip: chore: minor improvements; mainly: dropped g_syscall_code_routing_table and added an m4 script to automatically create syscall tables (normal + ia32)

Open FedeDP opened this issue 4 years ago • 19 comments

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area libscap

What this PR does / why we need it:

  • added a flexible array member to struct ppm_evt_hdr to manage events payload Std states:

As a special case, the last element of a structure with more than one named member may have an incomplete array type; this is called a flexible array member. In most situations, the flexible array member is ignored. In particular, the size of the structure is as if the flexible array member were omitted except that it may have more trailing padding than the omission would imply.

Note about the padding: we use an uint8_t for flexible array member, thus it shall not change padding in our case: you store sizeof(ppm_evt_hdr) + sizeof hdrs + sizeof(payload) and you load exactly the same; but the flexible array member helps in avoiding pointer arithmetic when dealing with hdrs and payload.

I tested the dump/read of scap files:

  1. new version from new version
  2. new version from old version
  3. old version from new version
  • merged g_syscall_code_routing_table into g_syscall_table

The g_syscall_code_routing_table was not really useful by itself, and the 2 tables had same indexing.

  • dropped syscall_table.c in favour of syscall_table.m4 script that will automatically build syscall_table.c at compile time. It allows adding a new syscall by just inserting a one-liner in the new X_SYSCALLS X macro.

It uses an m4 script; i think that m4 is the most widely available solution.
Why does this latter change matter? Because we were already hit by issues like forgetting to populate ia32 syscalls, or forgetting to populate PPM_SC_ code (the latter case is already somewhat mitigated by above point).

  • forced gnu99 as C standard

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

FedeDP avatar Sep 21 '21 14:09 FedeDP

Hi @FedeDP. Thanks for your PR.

I'm waiting for a falcosecurity member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

poiana avatar Sep 21 '21 14:09 poiana

/ok-to-test

leogr avatar Sep 21 '21 14:09 leogr

/retest

FedeDP avatar Sep 21 '21 14:09 FedeDP

Rebased on master.

FedeDP avatar Nov 15 '21 13:11 FedeDP

Closing and reopening to trigger the CI /close

leogr avatar Nov 25 '21 13:11 leogr

@leogr: Closed this PR.

In response to this:

Closing and reopening to trigger the CI /close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

poiana avatar Nov 25 '21 13:11 poiana

/reopen

leogr avatar Nov 25 '21 13:11 leogr

@leogr: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

poiana avatar Nov 25 '21 13:11 poiana

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

poiana avatar Feb 01 '22 09:02 poiana

/retest

FedeDP avatar Feb 02 '22 11:02 FedeDP

/test build-libs-bundled-deps

FedeDP avatar Mar 22 '22 11:03 FedeDP

/test build-libs-with-chisels

FedeDP avatar Mar 22 '22 11:03 FedeDP

I think i will split this PR in:

  • a PR that adds the flexible array member to struct ppm_evt_hdr to manage events payload + enforces c99 + merges g_syscall_code_routing_table into g_syscall_table
  • a PR that uses an m4 script to automatically create syscall tables

The latter is quite opinionated and might not be merged soon; the former instead should have the consensus to be merged.

FedeDP avatar Apr 21 '22 07:04 FedeDP

I think i will split this PR in:

  • a PR that adds the flexible array member to struct ppm_evt_hdr to manage events payload + enforces c99 + merges g_syscall_code_routing_table into g_syscall_table
  • a PR that uses an m4 script to automatically create syscall tables

The latter is quite opinionated and might not be merged soon; the former instead should have the consensus to be merged.

Hey @FedeDP

I totally agree with splitting this into several PRs. I'm also ok with C99. Moreover, I'd suggest documenting somewhere the C standard we are going to use.

leogr avatar Apr 21 '22 08:04 leogr

I split https://github.com/falcosecurity/libs/pull/298 from this one, with the first point above:

  • flexible array member to struct ppm_evt_hdr to manage events payload
  • enforces c99
  • merges g_syscall_code_routing_table into g_syscall_table

This PR (the #86) will instead become only the m4 script one and will be rebased on top of #298

FedeDP avatar Apr 22 '22 14:04 FedeDP

/hold

FedeDP avatar Apr 26 '22 12:04 FedeDP

/test build-libs-bundled-deps

FedeDP avatar Apr 28 '22 14:04 FedeDP

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

poiana avatar Aug 28 '22 09:08 poiana

Stale issues rot after 30d of inactivity.

Mark the issue as fresh with /remove-lifecycle rotten.

Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle rotten

poiana avatar Sep 27 '22 09:09 poiana

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community. /close

poiana avatar Oct 27 '22 09:10 poiana

@poiana: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community. /close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

poiana avatar Oct 27 '22 09:10 poiana

Most of this PR was split in multiple ones. We decided to avoid the m4 script, therefore there is not much else we can do. The only open points are:

  • c99
  • flexible array member that are also part of #298 (but i am not sure will ever be implemented).

FedeDP avatar Oct 31 '22 14:10 FedeDP