| Message ID | 20240502102241.1677978-1-amusil@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] hash, jhash: Fix unaligned access to the hash remainder. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 5/2/24 12:22, Ales Musil wrote: > The has was passed to memcpy as uin32_t *, however the hash bytes 'The has was passed' ? :) > might be unaligned at that point. Case it to uint8_t * instead > which has only single byte alignment requirement. > > 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 > ^ Please, wrap these lines. > #0 0x6191cb in hash_bytes /workspace/ovn/ovs/lib/hash.c:46:9 > #1 0x69d064 in hash_string /workspace/ovn/ovs/lib/hash.h:404:12 > #2 0x69d064 in hash_name /workspace/ovn/ovs/lib/shash.c:29:12 > #3 0x69d064 in shash_find /workspace/ovn/ovs/lib/shash.c:237:49 > #4 0x69dada in shash_find_data /workspace/ovn/ovs/lib/shash.c:251:31 > #5 0x507987 in add_remote /workspace/ovn/ovs/ovsdb/ovsdb-server.c:1382:15 > #6 0x507987 in parse_options /workspace/ovn/ovs/ovsdb/ovsdb-server.c:2659:13 > #7 0x507987 in main /workspace/ovn/ovs/ovsdb/ovsdb-server.c:751:5 > #8 0x7f47e3997087 in __libc_start_call_main (/lib64/libc.so.6+0x2a087) (BuildId: b098f1c75a76548bb230d8f551eae07a2aeccf06) > #9 0x7f47e399714a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a14a) (BuildId: b098f1c75a76548bb230d8f551eae07a2aeccf06) > #10 0x42de64 in _start (/workspace/ovn/ovs/ovsdb/ovsdb-server+0x42de64) (BuildId: 6c3f4e311556b29f84c9c4a5d6df5114dc08a12e) > Please, remove the '#' signs as github misinterprets them as PR/issue reference. And, please, remove the unnecessary info from the trace, e.g. BuildId, '/workspace/ovn/' part of the paths and maybe some other parts of the base libc frames. > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lib/hash.c:46:22 > > Signed-off-by: Ales Musil <amusil@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); We may accept the change, however, this looks more like a compiler bug to me. memcpy() accepts void pointers, so there is already an implicit cast. I didn't look into assembly, but I'd guess clang inlines the call and while doing that assumes the type. I'm not sure it is allowed to do that. Also, the 'n' here is always less than 4, so alignment should not be a problem because we can't copy the whole thing in a single aligned instruction (maybe there are instructions that can copy just 3 bytes without touching the 4th, but idk). Did you have a look at the asm by any chance? Best regards, Ilya Maximets.
On Thu, May 2, 2024 at 1:22 PM Ilya Maximets <i.maximets@ovn.org> wrote: > On 5/2/24 12:22, Ales Musil wrote: > > The has was passed to memcpy as uin32_t *, however the hash bytes > > 'The has was passed' ? :) > Oops :) > > > might be unaligned at that point. Case it to uint8_t * instead > > which has only single byte alignment requirement. > > > > 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 > > ^ > > Please, wrap these lines. > Ack > > > #0 0x6191cb in hash_bytes /workspace/ovn/ovs/lib/hash.c:46:9 > > #1 0x69d064 in hash_string /workspace/ovn/ovs/lib/hash.h:404:12 > > #2 0x69d064 in hash_name /workspace/ovn/ovs/lib/shash.c:29:12 > > #3 0x69d064 in shash_find /workspace/ovn/ovs/lib/shash.c:237:49 > > #4 0x69dada in shash_find_data /workspace/ovn/ovs/lib/shash.c:251:31 > > #5 0x507987 in add_remote > /workspace/ovn/ovs/ovsdb/ovsdb-server.c:1382:15 > > #6 0x507987 in parse_options > /workspace/ovn/ovs/ovsdb/ovsdb-server.c:2659:13 > > #7 0x507987 in main /workspace/ovn/ovs/ovsdb/ovsdb-server.c:751:5 > > #8 0x7f47e3997087 in __libc_start_call_main > (/lib64/libc.so.6+0x2a087) (BuildId: > b098f1c75a76548bb230d8f551eae07a2aeccf06) > > #9 0x7f47e399714a in __libc_start_main@GLIBC_2.2.5 > (/lib64/libc.so.6+0x2a14a) (BuildId: > b098f1c75a76548bb230d8f551eae07a2aeccf06) > > #10 0x42de64 in _start > (/workspace/ovn/ovs/ovsdb/ovsdb-server+0x42de64) (BuildId: > 6c3f4e311556b29f84c9c4a5d6df5114dc08a12e) > > > > Please, remove the '#' signs as github misinterprets them as PR/issue > reference. And, please, remove the unnecessary info from the trace, > e.g. BuildId, '/workspace/ovn/' part of the paths and maybe some other > parts of the base libc frames. > Ack > > > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lib/hash.c:46:22 > > > > Signed-off-by: Ales Musil <amusil@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); > > We may accept the change, however, this looks more like a compiler > bug to me. memcpy() accepts void pointers, so there is already an > implicit cast. I didn't look into assembly, but I'd guess clang > inlines the call and while doing that assumes the type. I'm not > sure it is allowed to do that. Also, the 'n' here is always less > than 4, so alignment should not be a problem because we can't copy > the whole thing in a single aligned instruction (maybe there are > instructions that can copy just 3 bytes without touching the 4th, > but idk). > > Did you have a look at the asm by any chance? > > As discussed offline, it doesn't inline and does the function call instead. > > Best regards, Ilya Maximets. > > Thanks, Ales
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];
The has was passed to memcpy as uin32_t *, however the hash bytes might be unaligned at that point. Case it to uint8_t * instead which has only single byte alignment requirement. 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 /workspace/ovn/ovs/lib/hash.c:46:9 #1 0x69d064 in hash_string /workspace/ovn/ovs/lib/hash.h:404:12 #2 0x69d064 in hash_name /workspace/ovn/ovs/lib/shash.c:29:12 #3 0x69d064 in shash_find /workspace/ovn/ovs/lib/shash.c:237:49 #4 0x69dada in shash_find_data /workspace/ovn/ovs/lib/shash.c:251:31 #5 0x507987 in add_remote /workspace/ovn/ovs/ovsdb/ovsdb-server.c:1382:15 #6 0x507987 in parse_options /workspace/ovn/ovs/ovsdb/ovsdb-server.c:2659:13 #7 0x507987 in main /workspace/ovn/ovs/ovsdb/ovsdb-server.c:751:5 #8 0x7f47e3997087 in __libc_start_call_main (/lib64/libc.so.6+0x2a087) (BuildId: b098f1c75a76548bb230d8f551eae07a2aeccf06) #9 0x7f47e399714a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a14a) (BuildId: b098f1c75a76548bb230d8f551eae07a2aeccf06) #10 0x42de64 in _start (/workspace/ovn/ovs/ovsdb/ovsdb-server+0x42de64) (BuildId: 6c3f4e311556b29f84c9c4a5d6df5114dc08a12e) SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lib/hash.c:46:22 Signed-off-by: Ales Musil <amusil@redhat.com> --- lib/hash.c | 2 +- lib/jhash.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)