On 6/12/24 20:12, Mike Pattrick wrote: > On Wed, Jun 12, 2024 at 9:50 AM Ales Musil <[email protected]> wrote: > >> >> >> On Wed, Jun 12, 2024 at 3:32 PM Mike Pattrick <[email protected]> wrote: >> >>> When compiling with '-fsanitize=address,undefined', the "ovs-ofctl >>> ct-flush" test will yield the following undefined behavior flagged >>> by UBSan. This patch uses memcpy to move the 128bit value into the >>> stack before reading it. >>> >>> lib/ofp-prop.c:277:14: runtime error: load of misaligned address >>> for type 'union ovs_be128', which requires 8 byte alignment >>> ^ >>> #0 0x7735d4 in ofpprop_parse_u128 lib/ofp-prop.c:277 >>> #1 0x6c6c83 in ofp_ct_match_decode lib/ofp-ct.c:529 >>> #2 0x76f3b5 in ofp_print_nxt_ct_flush lib/ofp-print.c:959 >>> #3 0x76f3b5 in ofp_to_string__ lib/ofp-print.c:1206 >>> #4 0x76f3b5 in ofp_to_string lib/ofp-print.c:1264 >>> #5 0x770c0d in ofp_print lib/ofp-print.c:1308 >>> #6 0x484a9d in ofctl_ofp_print utilities/ovs-ofctl.c:4899 >>> #7 0x4ddb77 in ovs_cmdl_run_command__ lib/command-line.c:247 >>> #8 0x47f6b3 in main utilities/ovs-ofctl.c:186
Thanks for cleaning up the trace. Please, also remove the '#' tags. GitHub trats them as references to issues/PRs and that is annoying. Also, while most of the addresses in the trace are not important and it's good to strip or shorten them, the actual address where the memory access happened is important here, so we can see what the actual alignment was. Was it part of the original error message? Clang usually provides them, not sure about gcc. >>> >>> Signed-off-by: Mike Pattrick <[email protected]> >>> --- >>> >> >> Hi Mike, >> >> this is interesting, do you have an idea why it didn't fail in CI by now? >> Also AFAIR the ofprops is supposed to be aligned to 8 bytes so unless the >> buffer itself isn't allocated at an address that is not itself 8 bytes >> aligned it shouldn't happen. In that case we might actually have a problem >> with other sizes. >> > > Report is seen with gcc + ubsan, but not clang + ubsan. It is possible that > this is only seen due the test, this warning wasn't seen live. I agree with Ales on this one. Properties supposed to be aligned. We need to find why they are not. i.e. is it a property itself or something before it. We may need to take similar approach as in commit: a5cc859a4228 ("ofp-actions: Use aligned structures when decoding ofp actions.") > > Cheers, > M > > >> >> >>> lib/ofp-prop.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/ofp-prop.c b/lib/ofp-prop.c >>> index 0a685750c..ed6365414 100644 >>> --- a/lib/ofp-prop.c >>> +++ b/lib/ofp-prop.c >>> @@ -271,10 +271,12 @@ enum ofperr >>> ofpprop_parse_u128(const struct ofpbuf *property, ovs_u128 *value) >>> { >>> ovs_be128 *p = property->msg; >>> + ovs_be128 aligned; >>> if (ofpbuf_msgsize(property) != sizeof *p) { >>> return OFPERR_OFPBPC_BAD_LEN; >>> } >>> - *value = ntoh128(*p); >>> + memcpy(&aligned, p, sizeof aligned); FWIW, this doesn't actually fix the issue. At least not in all the cases. Compiler is free to make alignment assumptions based on the pointer type, so we can still have unaligned access inside the memcpy. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
