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.

Sign up for GitHub

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

Jump to bottom

[PATCH v5] fix issues reported by GCC undefined sanitizer #2044

Merged
merged 19 commits into from
Apr 16, 2024
Merged

[PATCH v5] fix issues reported by GCC undefined sanitizer #2044

merged 19 commits into from
Apr 16, 2024

Conversation

JereLeppanen
Copy link
Collaborator

@JereLeppanen JereLeppanen commented Mar 28, 2024 •
edited
Loading

No description provided.

@odpbuild odpbuild changed the title fix issues reported by GCC undefined sanitizer [PATCH v1] fix issues reported by GCC undefined sanitizer Mar 28, 2024
@odpbuild odpbuild changed the title [PATCH v1] fix issues reported by GCC undefined sanitizer [PATCH v2] fix issues reported by GCC undefined sanitizer Mar 29, 2024
@JereLeppanen
Copy link
Collaborator Author

v2:

  • Rebase.

JannePeltonen
JannePeltonen requested changes Apr 8, 2024
View reviewed changes
platform/linux-generic/odp_pool.c Show resolved Hide resolved
platform/linux-generic/odp_packet.c Outdated Show resolved Hide resolved
platform/linux-generic/odp_hash_crc_gen.c Outdated Show resolved Hide resolved
platform/linux-generic/odp_parse.c Outdated Show resolved Hide resolved
platform/linux-generic/odp_pkt_queue.c Outdated Show resolved Hide resolved
platform/linux-generic/odp_random_std.c Outdated Show resolved Hide resolved
test/performance/odp_pktio_ordered.c Show resolved Hide resolved
test/performance/odp_packet_gen.c Outdated Show resolved Hide resolved
example/ipsec_crypto/odp_ipsec_misc.h Show resolved Hide resolved
helper/hashtable.c Outdated Show resolved Hide resolved
@odpbuild odpbuild changed the title [PATCH v2] fix issues reported by GCC undefined sanitizer [PATCH v3] fix issues reported by GCC undefined sanitizer Apr 15, 2024
@JereLeppanen
Copy link
Collaborator Author

All comments processed in v3.

JannePeltonen
JannePeltonen reviewed Apr 16, 2024
View reviewed changes
platform/linux-generic/odp_random_std.c Outdated
@@ -58,21 +58,21 @@ static int32_t _random_data(uint8_t *buf, uint32_t len, uint64_t *seed)
}

for (uint32_t i = 0; i < len / 4; i++) {
*(uint32_t *)(uintptr_t)buf = xorshift64s32(seed);
*(odp_una_u32_t *)buf = xorshift64s32(seed);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not assuming alignment here can throw away the work done to align the pointer if _ODP_UNALIGNED is zero. If the CPU does not support unaligned accesses, the compiler may have to resort to using multiple store instructions here, even though it is not really necessary.

I would suggest doing something like this here:

for (uint32_t i = 0; i < len / 4; i++) {
        if (_ODP_UNALIGNED)
                *(uint32_t *)(uintptr_t)buf = xorshift64s32(seed);
        else
                *(odp_una_u32_t *)buf = xorshift64s32(seed);
        buf += 4;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it a bit differently, in v4. But maybe there's no real need to optimize the no-unaligned case, and we could just remove the whole thing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but since it is already there and you fixed the alignment issues, I think it can as well stay.

@JannePeltonen
Copy link
Collaborator

Minor improvement proposal to _random_data(). With that, Reviewed-by: Janne Peltonen janne.peltonen@nokia.com

@odpbuild odpbuild changed the title [PATCH v3] fix issues reported by GCC undefined sanitizer [PATCH v4] fix issues reported by GCC undefined sanitizer Apr 16, 2024
@JereLeppanen
linux-gen: pool: free ring shm if pool creation fails
f8f909a 
If _odp_pool_create() fails, free ring_shm if it has been reserved. If
adjust_size() fails, instead of returning immediately, first free the
reserved resources, including ring_shm.

Also, for consistency, set ring pointer to NULL whenever ring_shm is
freed.

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
linux-gen: pool: flush pool cache only if the pool has a ring
ad7174c 
In cache_flush(), if the pool's ring is NULL, i.e. the pool is not
reserved, return immediately and don't try to flush the cache. This
avoids taking the address of a member of the ring struct via a NULL
pointer, which is undefined behavior.

Fixes GCC undefined sanitizer error:

odp_pool.c:138:7: runtime error: member access within null pointer of type 'struct pool_ring_t'

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
linux-gen: crypto_openssl: copy capabilities only if destination is n…
27e7c65 
…ot NULL

Don't copy capabilities if the destination pointer is NULL. In these
cases size is also zero, so nothing is copied anyway. However,
memcpy() destination is declared to never be NULL, so calling it with
NULL destination is undefined behavior.

Fixes GCC undefined sanitizer errors:

odp_crypto_openssl.c:1911:2: runtime error: null pointer passed as argument 1, which is declared to never be null
odp_crypto_openssl.c:2013:2: runtime error: null pointer passed as argument 1, which is declared to never be null

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
linux-gen: sched: use static array size in schedule_group_create()
76b5488 
With an empty thread mask, thr_tbl[] in schedule_group_create() ends
up being zero size, which is undefined behavior. Fix by using a static
array size.

Fixes GCC undefined sanitizer error:

odp_schedule_basic.c:1922:6: runtime error: variable length array bound evaluates to non-positive value 0

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
linux-gen: packet: in packet_free_multi_ev(), return immediately if n…
cb53b7d 
…umber of events is zero

If packet_free_multi_ev() is called with zero events, the pkt_hdrs[]
array ends up being zero size, which is undefined behavior. Fix by
returning early if number of events is zero.

Fixes GCC undefined sanitizer error:

odp_packet.c:702:20: runtime error: variable length array bound evaluates to non-positive value 0

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
linux-gen: crc: avoid undefined behavior in bit shifts
1f5d00e 
Avoid undefined behavior in bit shifts in reflect_u*() functions by
using types of sufficient width for the values being shifted.

Fixes GCC undefined sanitizer error:

odp_hash_crc_gen.c:90:16: runtime error: left shift of 237 by 24 places cannot be represented in type 'int'

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
linux-gen: hash: use unaligned types in _odp_hash_crc32c()
8cd6d9d 
Since the data is not necessarily aligned, use unaligned types to read
it, in order to avoid undefined behavior.

Fixes GCC undefined sanitizer errors:

hash_crc32.h:35:24: runtime error: load of misaligned address 0x0000005e12d4 for type 'const uint64_t', which requires 8 byte alignment
hash_crc32.h:40:14: runtime error: load of misaligned address 0x0000005ff879 for type 'const uint32_t', which requires 4 byte alignment
hash_crc32.h:51:47: runtime error: load of misaligned address 0x0000005e67af for type 'const uint16_t', which requires 2 byte alignment

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
linux-gen: parse: use unaligned types when parsing ethernet header
4a401bd 
An ethernet header being parsed may not be aligned in memory, so use
unaligned types to read it, in order to avoid undefined behavior.

Fixes GCC undefined sanitizer errors:

odp_parse.c:43:30: runtime error: load of misaligned address 0x7fc1d4675093 for type 'const uint16_t', which requires 2 byte alignment
odp_parse.c:49:21: runtime error: load of misaligned address 0x7fc1d4674655 for type 'const uint16_t', which requires 2 byte alignment
odp_parse.c:52:21: runtime error: load of misaligned address 0x7fc1d4674657 for type 'const uint16_t', which requires 2 byte alignment
odp_parse.c:72:30: runtime error: load of misaligned address 0x7fc1d4674667 for type 'const uint16_t', which requires 2 byte alignment

Also, remove unnecessary intermediate casts to const void * and uintptr_t.

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
linux-gen: tm: align allocation of queue_blk_t for internal packet queue
d259871 
Align allocation of queue_blk_t, in order to avoid unaligned access to
queue_blk_t members, which is undefined behavior.

Fixes many GCC undefined sanitizer errors, for example:

odp_pkt_queue.c:66:22: runtime error: member access within misaligned address 0x7fe5c3d1f010 for type 'struct queue_blk_t', which requires 64 byte alignment

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
linux-gen: random: use unaligned types
c334381 
Random data destination may not be aligned. Use unaligned types to
write the random data, in order to avoid undefined behavior.

Fixes GCC undefined sanitizer errors:

odp_random_std.c:61:31: runtime error: store to misaligned address 0x7fffc1307109 for type 'uint32_t', which requires 4 byte alignment
odp_random_std.c:69:32: runtime error: store to misaligned address 0x7fffc1307109 for type 'uint16_t', which requires 2 byte alignment

Also, remove some unnecessary intermediate casts to uintptr_t.

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
validation: crypto: in write_header_and_trailer(), return immediately…
654fca3 
… if header and trailer lengths are zero

If write_header_and_trailer() is called with both header_len and
trailer_len zero, the buffer[] array ends up being zero size, which is
undefined behavior. Fix by returning early in that case.

Fixes GCC undefined sanitizer error:

crypto_op_test.c:165:10: runtime error: variable length array bound evaluates to non-positive value 0

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
validation: crypto: use static array sizes for cipher and auth keys i…
6a49c6d 
…n session_create()

When calling session_create() with cipher or auth key length zero, the
cipher_key_data[] and/or auth_key_data[] arrays end up being zero
size, which is undefined behavior. Fix by using static array sizes.

Fixes GCC undefined sanitizer errors:

odp_crypto_test_inp.c:191:10: runtime error: variable length array bound evaluates to non-positive value 0
odp_crypto_test_inp.c:192:10: runtime error: variable length array bound evaluates to non-positive value 0

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
test: performance: pktio_ordered: avoid undefined behavior in bit shi…
75ff8ee 
…ft in calc_ipv4_5tuple_hash()

Avoid undefined behavior in bit shift in calc_ipv4_5tuple_hash() by
casting to an unsigned integer of sufficient width before the shift
operation.

Fixes GCC undefined sanitizer error:

odp_pktio_ordered.c:336:24: runtime error: left shift of negative value -25535

Also, use unsigned types in ipv4_tuple5_t. This avoids potential
implementation defined results in calc_flow_idx() when assigning
unsigned values to a tuple.

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
test: performance: cpu_bench: use unaligned type when accessing packe…
5d0214e 
…t data

Packet data may not be aligned. Use unaligned type when accessing
packet data in order to avoid undefined behavior.

Fixes multiple GCC undefined sanitizer errors, for example:

odp_cpu_bench.c:206:11: runtime error: store to misaligned address 0x7fde78bda1aa for type 'uint32_t', which requires 4 byte alignment

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
test: packet_gen: use static array size
114a5f7 
In open_pktios(), if the number of rx or tx threads is zero, the
pktin[] or pktout[] arrays end up being zero size, which is undefined
behavior. Fix by using a static array size. Also, move the arrays into
a smaller scope.

Fixes GCC undefined sanitizer error:

odp_packet_gen.c:790:20: runtime error: variable length array bound evaluates to non-positive value 0

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
example: ipfragreass: avoid undefined behavior in bit shift in hash()
45aa19b 
Avoid undefined behavior in bit shift in hash() by casting to an
unsigned integer of sufficient width before the shift operation.

Fixes GCC undefined sanitizer error:

odp_ipfragreass_reassemble.c:92:23: runtime error: left shift of 40448 by 16 places cannot be represented in type 'int'

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
example: ipsec: use unsigned integers to form ip4 address
21d2d25 
In parse_ipv4_string(), use unsigned variables and scanf formatters to
avoid negative values, and cast to uint32_t in order to avoid
undefined behavior in bit shift operations.

Fixes GCC undefined sanitizer error:

odp_ipsec_misc.h:192:47: runtime error: left shift of 192 by 24 places cannot be represented in type 'int'

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
helper: hashtable: round up hash node size
386ea35 
Round node size up to the hash node alignment requirement.

Fixes GCC undefined sanitizer errors:

hashtable.c:203:22: runtime error: member access within misaligned address 0x7f92c156007b for type 'struct odph_hash_node', which requires 8 byte alignment
hashtable.c:204:22: runtime error: member access within misaligned address 0x7f92c156007b for type 'struct odph_hash_node', which requires 8 byte alignment
hashtable.c:205:4: runtime error: member access within misaligned address 0x7f92c156007b for type 'struct odph_hash_node', which requires 8 byte alignment
hashtable.c:266:13: runtime error: member access within misaligned address 0x7f92c156007b for type 'struct odph_hash_node', which requires 8 byte alignment
hashtable.c:267:25: runtime error: member access within misaligned address 0x7f92c156007b for type 'struct odph_hash_node', which requires 8 byte alignment
hashtable.c:271:2: runtime error: member access within misaligned address 0x7f92c156007b for type 'struct odph_hash_node', which requires 8 byte alignment
odph_list_internal.h:31:13: runtime error: member access within misaligned address 0x7f92c156007b for type 'struct odph_list_object', which requires 8 byte alignment
odph_list_internal.h:32:13: runtime error: member access within misaligned address 0x7f92c156007b for type 'struct odph_list_object', which requires 8 byte alignment
odph_list_internal.h:40:12: runtime error: member access within misaligned address 0x7f92c156007b for type 'struct odph_list_object', which requires 8 byte alignment
odph_list_internal.h:41:12: runtime error: member access within misaligned address 0x7f92c156007b for type 'struct odph_list_object', which requires 8 byte alignment

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@JereLeppanen
github_ci: add undefined sanitizier to the sanitizer job
e513e83 
Add undefined sanitizier to the sanitizer job.

Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
@odpbuild odpbuild changed the title [PATCH v4] fix issues reported by GCC undefined sanitizer [PATCH v5] fix issues reported by GCC undefined sanitizer Apr 16, 2024
@JereLeppanen
Copy link
Collaborator Author

v5: Rebase.

@MatiasElo MatiasElo merged commit 9492b6d into OpenDataPlane:master Apr 16, 2024
171 of 172 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JannePeltonen JannePeltonen JannePeltonen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JereLeppanen @JannePeltonen @MatiasElo