fix(libsinsp): generalize big-endian handling beyond s390x#3003
Open
Scottcjn wants to merge 2 commits into
Open
fix(libsinsp): generalize big-endian handling beyond s390x#3003Scottcjn wants to merge 2 commits into
Scottcjn wants to merge 2 commits into
Conversation
flt_cast()'s byte-shift adjustment for `evt.rawarg.*` fields, applied on big-endian systems when len > sizeof(fromT), was compiled only under `#ifdef __s390x__`. s390x is not the only big-endian target -- big-endian PowerPC hits the same edge case -- so the guard now keys on endianness (`__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__`) instead of a specific arch. The `defined(__BYTE_ORDER__)` part keeps the MSVC build path (where the macro is absent) correct. The block body is unchanged. The scap-file test suite was likewise disabled by matching the s390x processor string; it now keys off CMAKE_CXX_BYTE_ORDER so every big-endian host is covered. Refs falcosecurity#3002 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Scott Boudreaux <scott@elyanlabs.com>
poiana
added
kind/bug
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Scottcjn The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
|
Welcome @Scottcjn! It looks like this is your first PR to falcosecurity/libs 🎉 |
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3003 +/- ##
=======================================
Coverage 75.76% 75.76%
=======================================
Files 299 299
Lines 33099 33099
Branches 5126 5126
=======================================
Hits 25079 25079
Misses 8020 8020
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
Hi @Scottcjn, thanks for the contribution. It seems that you need to address a minor formatting issue... |
Re-wrap a comment to the repo cmake-format config so the format-code pre-commit check passes. Signed-off-by: Scott Boudreaux <scott@elyanlabs.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area libsinsp
What this PR does / why we need it:
While looking at #3002 (big-endian PowerPC), the userspace side still treats
s390xas a synonym for "big-endian":flt_cast()infilter_compare.cppapplies a byte-shift adjustment forevt.rawarg.*fields whenlen > sizeof(fromT)on big-endian systems — but it was compiled only under#ifdef __s390x__. On any other big-endian target (e.g. big-endian PowerPC) that adjustment is silently dropped andevt.rawarg.*comparisons read the wrong bytes. The guard now keys on endianness (#if defined(__BYTE_ORDER__) && __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__) instead of a specific arch. Thedefined(__BYTE_ORDER__)part keeps the Windows/MSVC build path correct (the macro is absent there, and a bare#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__would wrongly evaluate true). The executable block body is unchanged — only the guard and comment.The scap-file test suite in
userspace/libsinsp/test/CMakeLists.txtwas disabled by matching thes390xprocessor string; it now keys offCMAKE_CXX_BYTE_ORDERso every big-endian host is covered.This mirrors how the repo already detects endianness elsewhere (
__BYTE_ORDER__inparse_connect.cpp,ifinfo.ut.cpp). The kernel-driver / modern-BPF side of #3002 is larger and out of scope here.Which issue(s) this PR fixes:
Refs #3002
Special notes for your reviewer:
Verified: the new guard compiles inactive on x86_64 (little-endian, no perf hit) and active on s390x — cross-compiled and run under
qemu-s390x. Theflt_castblock body is byte-identical to the previous__s390x__-gated code, so big-endian compile coverage is inherited from the existing s390x build.Does this PR introduce a user-facing change?:
🤖 Generated with Claude Code