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

Fix strict aliasing UB in MurMur hash implementation. #459

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Romain-Geissler-1A
Copy link

This was spotted when trying to upgrade the libseccomp fedora package to version 2.6.0 in fedora rawhide. It comes with gcc 15 and LTO enabled by default. When running the test 61-sim-transactions we get plenty of such errors in valgrind:

==265507== Use of uninitialised value of size 8
==265507==    at 0x4096AD: _hsh_add (gen_bpf.c:599)
==265507==    by 0x40A557: UnknownInlinedFun (gen_bpf.c:2016)
==265507==    by 0x40A557: gen_bpf_generate (gen_bpf.c:2341)
==265507==    by 0x400CDE: UnknownInlinedFun (db.c:2685)
==265507==    by 0x400CDE: UnknownInlinedFun (db.c:2682)
==265507==    by 0x400CDE: UnknownInlinedFun (api.c:756)
==265507==    by 0x400CDE: UnknownInlinedFun (util.c:162)
==265507==    by 0x400CDE: UnknownInlinedFun (util.c:153)
==265507==    by 0x400CDE: main (61-sim-transactions.c:128)
==265507==  Uninitialised value was created by a stack allocation
==265507==    at 0x409590: _hsh_add (gen_bpf.c:573)

Investigating this a bit, it seems that because of LTO the MurMur hash implementation is being inlined in _hsh_add. The way we call getblock32 with the explicit cast to const uint32_t* is a strict aliasing violation.

This is reproducible on a "fedora:rawhide" container (gcc 15) and using:
export CFLAGS='-O2 -flto=auto -ffat-lto-objects -g'

This was spotted when trying to upgrade the libseccomp fedora package to
version 2.6.0 in fedora rawhide. It comes with gcc 15 and LTO enabled by
default. When running the test 61-sim-transactions we get plenty of such
errors in valgrind:

==265507== Use of uninitialised value of size 8
==265507==    at 0x4096AD: _hsh_add (gen_bpf.c:599)
==265507==    by 0x40A557: UnknownInlinedFun (gen_bpf.c:2016)
==265507==    by 0x40A557: gen_bpf_generate (gen_bpf.c:2341)
==265507==    by 0x400CDE: UnknownInlinedFun (db.c:2685)
==265507==    by 0x400CDE: UnknownInlinedFun (db.c:2682)
==265507==    by 0x400CDE: UnknownInlinedFun (api.c:756)
==265507==    by 0x400CDE: UnknownInlinedFun (util.c:162)
==265507==    by 0x400CDE: UnknownInlinedFun (util.c:153)
==265507==    by 0x400CDE: main (61-sim-transactions.c:128)
==265507==  Uninitialised value was created by a stack allocation
==265507==    at 0x409590: _hsh_add (gen_bpf.c:573)

Investigating this a bit, it seems that because of LTO the MurMur hash
implementation is being inlined in _hsh_add. The way we call getblock32
with the explicit cast to const uint32_t* is a strict aliasing
violation.

This is reproducible on a "fedora:rawhide" container (gcc 15) and using:
export CFLAGS='-O2 -flto=auto -ffat-lto-objects -g'

Signed-off-by: Romain Geissler <[email protected]>
@coveralls
Copy link

Coverage Status

coverage: 90.246% (-0.006%) from 90.252%
when pulling e6904da on Romain-Geissler-1A:main
into 7db46d7 on seccomp:main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants