| Message ID | 20211221110216.14345.12856.stgit@dceara.remote.csb |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix UndefinedBehaviorSanitizer reported issues and enable it in CI. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Dumitru Ceara <dceara@redhat.com> writes: > UB Sanitizer report: > lib/stopwatch.c:119:22: runtime error: index 18446744073709551615 out of bounds for type 'long long unsigned int [50]' > #0 0x698358 in calc_percentile lib/stopwatch.c:119 > #1 0x69ada1 in add_sample lib/stopwatch.c:231 > #2 0x69c086 in stopwatch_end_sample_protected lib/stopwatch.c:386 > #3 0x69c522 in stopwatch_thread lib/stopwatch.c:441 > #4 0x684bae in ovsthread_wrapper lib/ovs-thread.c:383 > #5 0x7f042838b298 in start_thread (/lib64/libpthread.so.0+0x9298) > #6 0x7f04277f2352 in clone (/lib64/libc.so.6+0x100352) > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- It's a good catch - even with the fixed qsort, the caculated percentiles are all incorrect. I wonder if it also makes sense to have an early exit case in the calc_percentile function. Either way, I think this makes a lot of sense to me. Acked-by: Aaron Conole <aconole@redhat.com>
On 12/21/21 15:57, Aaron Conole wrote: > Dumitru Ceara <dceara@redhat.com> writes: > >> UB Sanitizer report: >> lib/stopwatch.c:119:22: runtime error: index 18446744073709551615 out of bounds for type 'long long unsigned int [50]' >> #0 0x698358 in calc_percentile lib/stopwatch.c:119 >> #1 0x69ada1 in add_sample lib/stopwatch.c:231 >> #2 0x69c086 in stopwatch_end_sample_protected lib/stopwatch.c:386 >> #3 0x69c522 in stopwatch_thread lib/stopwatch.c:441 >> #4 0x684bae in ovsthread_wrapper lib/ovs-thread.c:383 >> #5 0x7f042838b298 in start_thread (/lib64/libpthread.so.0+0x9298) >> #6 0x7f04277f2352 in clone (/lib64/libc.so.6+0x100352) >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- > > It's a good catch - even with the fixed qsort, the caculated percentiles > are all incorrect. > > I wonder if it also makes sense to have an early exit case in the > calc_percentile function. Either way, I think this makes a lot of sense > to me. We could add an early exit but given that because calc_percentile() is static and called from a single place within stopwatch.c, just after we validated that sw->n_samples > 0, I thought that's not really necessary. > > Acked-by: Aaron Conole <aconole@redhat.com> > Thanks for the review! Dumitru
diff --git a/lib/stopwatch.c b/lib/stopwatch.c index f5602163bc16..1c71df1a127e 100644 --- a/lib/stopwatch.c +++ b/lib/stopwatch.c @@ -114,7 +114,6 @@ static void calc_percentile(unsigned long long n_samples, struct percentile *pctl, unsigned long long new_sample) { - if (n_samples < P_SQUARE_MIN) { pctl->samples[n_samples - 1] = new_sample; } @@ -228,13 +227,12 @@ add_sample(struct stopwatch *sw, unsigned long long new_sample) sw->min = new_sample; } - calc_percentile(sw->n_samples, &sw->pctl, new_sample); - if (sw->n_samples++ == 0) { sw->short_term.average = sw->long_term.average = new_sample; return; } + calc_percentile(sw->n_samples, &sw->pctl, new_sample); calc_average(&sw->short_term, new_sample); calc_average(&sw->long_term, new_sample); }
UB Sanitizer report: lib/stopwatch.c:119:22: runtime error: index 18446744073709551615 out of bounds for type 'long long unsigned int [50]' #0 0x698358 in calc_percentile lib/stopwatch.c:119 #1 0x69ada1 in add_sample lib/stopwatch.c:231 #2 0x69c086 in stopwatch_end_sample_protected lib/stopwatch.c:386 #3 0x69c522 in stopwatch_thread lib/stopwatch.c:441 #4 0x684bae in ovsthread_wrapper lib/ovs-thread.c:383 #5 0x7f042838b298 in start_thread (/lib64/libpthread.so.0+0x9298) #6 0x7f04277f2352 in clone (/lib64/libc.so.6+0x100352) Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- lib/stopwatch.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)