Project

General

Profile

Bug #3407

libosmocore utils_test isqrt_test() may trigger sanitizer

Added by neels 5 months ago. Updated 4 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Start date:
07/19/2018
Due date:
% Done:

90%

Spec Reference:

Description

Testing integer square-root
../../../src/libosmocore/tests/utils/utils_test.c:445:18: runtime error: signed integer overflow: 60369 * 60369 cannot be represented in type 'int'

I think it's a really bad idea to use random data in a regression test.
It thwarts the reproducibility and causes sporadic failure, like this one.
Rather pick constants from all corners of the desired value range.

Added by:

commit 15a5f8de00c9c11a985ab16279ffae02b1e4667f
Author: Harald Welte <laforge@gnumonks.org>
Date:   Wed Jun 6 16:58:17 2018 +0200

    Add osmo_isqrt32() to compute 32bit integer square root

    Change-Id: I2b96db6e037e72e92317fec874877e473a1cf909

History

#1 Updated by neels 5 months ago

It seems to me this test is guaranteed to fail, yet doesn't ever seem to print "ERROR"

                uint32_t r = rand();   <--- a value up to UINT32_MAX
                uint32_t sq = x*x;     <--- multiplied by itself will overflow for anything > UINT16_MAX
                uint32_t y = osmo_isqrt32(sq); <--- so the original x is then by definition unrecoverable
                if (y != x)
                        printf("ERROR: x=%u, sq=%u, osmo_isqrt(%u) = %u\n", x, sq, sq, y); <-- I never see this, expect to see it a lot.

Is it some algorithmic magic? I doubt that we want to have any magic like that?

#2 Updated by neels 5 months ago

oh geez, I completely missed the uint16_t x above that.

#3 Updated by neels 5 months ago

  • Status changed from New to In Progress
  • % Done changed from 0 to 90

Adding a cast fixes the sanitizer issue:

https://gerrit.osmocom.org/#/c/libosmocore/+/10066

and also checking stderr is a good idea: https://gerrit.osmocom.org/#/c/libosmocore/+/10067

#4 Updated by laforge 4 months ago

  • Status changed from In Progress to Resolved
  • Assignee set to neels

both patches have been merged.

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)