On 4/26/22 16:29, Adrian Moreno wrote: > > > On 4/12/22 15:29, Dumitru Ceara wrote: >> When compiled with '-fsanitize=undefined' running 'ovsdb-client >> --timestamp monitor Open_vSwitch' in a sandbox triggers the following >> undefined behavior (flagged by UBSan): >> >> lib/dynamic-string.c:207:38: runtime error: applying zero offset to >> null pointer >> #0 0x196c05b in ds_put_strftime_msec lib/dynamic-string.c:207:38 >> #1 0x196c2aa in xastrftime_msec lib/dynamic-string.c:225:5 >> #2 0x1935f33 in pmd_info_show_perf lib/dpif-netdev.c:799:17 >> #3 0x1935f33 in dpif_netdev_pmd_info lib/dpif-netdev.c:1487:13 >> #4 0x1938155 in pmd_perf_show_cmd lib/dpif-netdev.c:1537:5 >> #5 0x1d7634b in process_command lib/unixctl.c:310:13 >> #6 0x1d7634b in run_connection lib/unixctl.c:344:17 >> #7 0x1d7634b in unixctl_server_run lib/unixctl.c:395:21 >> #8 0x168b5bd in main vswitchd/ovs-vswitchd.c:130:9 >> #9 0x7f6fa2fd8492 in __libc_start_main (/lib64/libc.so.6+0x23492) >> #10 0xe6911d in _start (vswitchd/ovs-vswitchd+0xe6911d) >> >> Reported-by: Ilya Maximets <[email protected]> >> Signed-off-by: Dumitru Ceara <[email protected]> >> --- >> lib/dynamic-string.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c >> index fd0127ed1740..3d415af4f4e0 100644 >> --- a/lib/dynamic-string.c >> +++ b/lib/dynamic-string.c >> @@ -200,8 +200,8 @@ ds_put_strftime_msec(struct ds *ds, const char >> *template, long long int when, >> for (;;) { >> size_t avail = ds->string ? ds->allocated - ds->length + 1 : 0; >> - size_t used = strftime_msec(&ds->string[ds->length], avail, >> template, >> - &tm); >> + char *dest = ds->string ? &ds->string[ds->length] : NULL; >> + size_t used = strftime_msec(dest, avail, template, &tm); >> if (used) { >> ds->length += used; >> return; > > The code looks OK to me. However, calling strftime_msec(NULL, 0,...) > once if the dynamic string is empty seems to me a bit obfuscated. > > I would personally just add: > > if (!ds->string) { > ds_reserve(ds, 64); > } > > before the loop to mentally get rid of that corner case.
Sure, that's fine too, I have no preference either way. Ilya, what do you think, does this need a v2? > > In any case: > > Acked-by: Adrian Moreno <[email protected]> > Thanks for the review! _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
