search

seccomp / libseccomp

The main libseccomp repository
GNU Lesser General Public License v2.1
791 stars 170 forks source link

BUG: fix aliasing UB in scmp_bpf_sim #433

Closed thesamesam closed 22 hours ago

thesamesam commented 2 months ago

See https://github.com/seccomp/libseccomp/pull/425.

Punning sys_data_b between uint32_t and struct seccomp_data isn't legal, use memcpy to fix the testsuite with Clang 17.

Modern compilers recognise this idiom and optimise it out anyway.

coveralls commented 2 months ago

Coverage Status

coverage: 89.454%. remained the same when pulling 4f354f47a6f8e81f2bf7add623df7424e84ebba9 on thesamesam:alias-ub into 9da5d174e3ef219baab020a79c789f2075ace45c on seccomp:main.

RossComputerGuy commented 1 month ago

Still experiencing failures with LLVM 17.0.6 https://termbin.com/j2ey

thesamesam commented 1 month ago

I can't reproduce that, I'm afraid. I get failures if I revert my patch, but it passes otherwise.

Are you sure it was applied? I also can't see anything obvious that would cause yours if the patch is applied.

RossComputerGuy commented 1 month ago

Are you sure it was applied?

I am 100% the patch is applied, I'm using Nix and it would fail if the patch failed to apply. It said the patch was applied.

thesamesam commented 1 month ago

No idea then, unfortunately. You'll need to try debug it. The full build log could also be useful but I don't see myself able to spend more time on a failure I can't reproduce if it's not caused by my PR, sorry.

I suggest starting with object files from a good and bad tree and bisecting via those.

pcmoore commented 22 hours ago

Merged via 2847f10dddca72167309c04cd09f326fd3b78e2f, thanks for following through with this fix @thesamesam. If anyone is still seeing failures with this patch applies, please open a new issue so we can debug it further.

thesamesam commented 21 hours ago

Thanks all!