| Message ID | 20220412132910.2858-1-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] dynamic-string: Fix undefined behavior due to offsetting null pointer. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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 <i.maximets@ovn.org> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > 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. In any case: Acked-by: Adrian Moreno <amorenoz@redhat.com>
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 <i.maximets@ovn.org> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> 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 <amorenoz@redhat.com> > Thanks for the review!
On 4/26/22 17:16, Dumitru Ceara wrote: > 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 <i.maximets@ovn.org> >>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> --- >>> 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? Yeah, the ds_reserve() seems cleaner. We don't really need to check for !ds->string before calling it though, IIUC. With that we may also remove the check while calculating 'avail'. Best regards, Ilya Maximets. > >> >> In any case: >> >> Acked-by: Adrian Moreno <amorenoz@redhat.com> >> > > Thanks for the review! >
On 5/17/22 22:52, Ilya Maximets wrote: > On 4/26/22 17:16, Dumitru Ceara wrote: >> 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 <i.maximets@ovn.org> >>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>>> --- >>>> 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? > > Yeah, the ds_reserve() seems cleaner. We don't really need to > check for !ds->string before calling it though, IIUC. > > With that we may also remove the check while calculating 'avail'. Done, thanks for the suggestions. I posted v2: dynamic-string: Fix undefined behavior due to offsetting null pointer. Thanks, Dumitru > > Best regards, Ilya Maximets. > >> >>> >>> In any case: >>> >>> Acked-by: Adrian Moreno <amorenoz@redhat.com> >>> >> >> Thanks for the review! >> >
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;
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 <i.maximets@ovn.org> Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- lib/dynamic-string.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)