Divide by zero in munit_rand_state_at_most()

[chriso]

.. when calling munit_rand_int_range(any_negative_int, 0);

Address sanitizer:

munit/munit.c:676:42: runtime error: division by zero
SUMMARY: AddressSanitizer: undefined-behavior munit/munit.c:676:42 in 
Error: child killed by signal 8

Valgrind:

==60== Process terminating with default action of signal 8 (SIGFPE)
==60==  Integer divide by zero at address 0x802DA55A0
==60==    at 0x406CDD: munit_rand_state_at_most (munit.c:676)
==60==    by 0x406D74: munit_rand_at_most (munit.c:698)
==60==    by 0x406E21: munit_rand_int_range (munit.c:714)

The % max when max=0 is the issue on line 676:

const munit_uint32_t min = (~max + 1U) % max;

[jibsen]

I haven't tried it, but munit_rand_int_range() passes the range (max - min) as the max parameter (line 714). So max in line 676 should only be 0 if min and max are equal?

[chriso]

Ah yep, that could be the case. I've been randomising min/max and they meet at 0 so there's a chance I called it with (0, 0)

…

On Fri, 24 Mar 2017 at 5:52 pm, Jørgen Ibsen ***@***.***> wrote: I haven't tried it, but munit_rand_int_range() passes the range (max - min) as the max parameter (line 714). So max in line 676 should only be 0 if min and max are equal? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWRZzvj25JZn4kKISqb9r_BY00nkdblks5ro3YlgaJpZM4MnxC2> .

[nemequ]

Interesting. It's somewhat nonsensical to call munit_rand_int_range with min == max, though certainly understandable if the min/max are themselves random… we should definitely handle it more gracefully than a division by zero.

My first thought is to add a munit_assert_int(min, !=, max); to munit_rand_int_range, or we could just do if (min == max) return max;. I'm not sure which would be best… I don't want to force people to add an unnecessary check in their tests, but silently returning a non-random value and covering up a potential bug in their logic could be worse :(

I'm leaning toward the assertion, but first: does anyone have an opinion?

[jibsen]

Since the range is inclusive, I would be inclined to say that asking for a random number in [1, 1] should return 1.

[nemequ]

Fixed by returning min (not asserting that min != max).

[chammaaomar]

This issue seems to persist in the master branch. Today, I used munit_rand_int_range with min == max and got floating point error. I fixed it in my repo by going into the munit.c and adding if (min ==max) return max; since the range is inclusive, that's the behaviour that makes most sense to me as remarked by @jibsen

[codylico]

This seems to be a regression caused by e32a8ca.

The commit referencing this issue (a603d26) is on the 'master' branch, and actually seems to fix this issue. However, e32a8ca occurs after that, and reverts the changes from a603d26.