On 5/2/24 15:28, Ales Musil wrote:
> The pointer was passed to memcpy as uin32_t *, however the hash bytes
> might be unaligned at that point. Case it to uint8_t * instead
'Case' ?
> which has only single byte alignment requirement. This seems to be
> a false positive reported by clang [0].
After thinking some more, it's not actually a false positive per se.
According to the C spec we're not actually allowed to have misaligned
pointers even if we're not reading/writing through them.
So, technically, the initial cast to uint32_t pointer is no correct.
I don't think we can fully avoid such casts without loosing type checking,
but I think we need to revert changes to hash functions made in
commit db5a101931c5 ("clang: Fix the alignment warning.").
i.e. we should go back to using uint8_t pointer and cast it on the
get_unaligned_u32() call with ALIGNED_CAST. We will still have a
misaligned pointer, but it will be immediately cast back, so should
cause less issues.
Note: all arithmetic should be done on the uint8_t pointer, not a
misaligned uin32_t one to avoid potential other UB conditions.
Best regards, Ilya Maximets.
>
> lib/hash.c:46:22: runtime error: load of misaligned address
> 0x507000000065 for type 'const uint32_t *' (aka 'const unsigned int *'),
> which requires 4 byte alignment
> 0x507000000065: note: pointer points here
> 73 62 2e 73 6f 63 6b 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ^
> 00 00 00 00 00 00 00 00 00
> 0 0x6191cb in hash_bytes ovs/lib/hash.c:46:9
> 1 0x69d064 in hash_string ovs/lib/hash.h:404:12
> 2 0x69d064 in hash_name ovs/lib/shash.c:29:12
> 3 0x69d064 in shash_find ovs/lib/shash.c:237:49
> 4 0x69dada in shash_find_data ovs/lib/shash.c:251:31
> 5 0x507987 in add_remote ovs/ovsdb/ovsdb-server.c:1382:15
> 6 0x507987 in parse_options ovs/ovsdb/ovsdb-server.c:2659:13
> 7 0x507987 in main ovs/ovsdb/ovsdb-server.c:751:5
>
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lib/hash.c:46:22
>
> [0] https://github.com/llvm/llvm-project/issues/90848
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
> lib/hash.c | 2 +-
> lib/jhash.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/hash.c b/lib/hash.c
> index c722f3c3c..986fa6643 100644
> --- a/lib/hash.c
> +++ b/lib/hash.c
> @@ -43,7 +43,7 @@ hash_bytes(const void *p_, size_t n, uint32_t basis)
> if (n) {
> uint32_t tmp = 0;
>
> - memcpy(&tmp, p, n);
> + memcpy(&tmp, (const uint8_t *) p, n);
> hash = hash_add(hash, tmp);
> }
>
> diff --git a/lib/jhash.c b/lib/jhash.c
> index c59b51b61..0a0628589 100644
> --- a/lib/jhash.c
> +++ b/lib/jhash.c
> @@ -114,7 +114,7 @@ jhash_bytes(const void *p_, size_t n, uint32_t basis)
> uint32_t tmp[3];
>
> tmp[0] = tmp[1] = tmp[2] = 0;
> - memcpy(tmp, p, n);
> + memcpy(tmp, (const uint8_t *) p, n);
> a += tmp[0];
> b += tmp[1];
> c += tmp[2];
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
