Skip to content

Fix out-of-range integer brng reading past brng_list - #177

Open
vchamarthi wants to merge 5 commits into
IntelPython:masterfrom
vchamarthi:fix/validate-brng-token
Open

vchamarthi wants to merge 5 commits into
IntelPython:masterfrom
vchamarthi:fix/validate-brng-token

Conversation

@vchamarthi

Copy link
Copy Markdown
Contributor

Issue

brng_list has BRNG_KINDS = 11 entries, irk_brng_t defines 0–10. The integer
path of _parse_brng_token_ coerced the user value straight to irk_brng_t, which
only checks it fits the enum's underlying integer type. The seeding routines then
index brng_list[brng].

brng=11 returned uninitialized memory, no warning, no error:

mkl_random.MKLRandomState(1, brng=11).randint(0, 100, 6)
array([1744283392, 31525, 299244432, 22256, 1744285296, 31525])

brng=15 raised an oneMKL error; brng=-1 was accepted as a negative index.
Separately, if (brng): treated brng=0 as unset, but 0 is MT19937:

Intel oneMKL ERROR: Parameter 1 was incorrect on entry to vslGetStreamStateBrng
mkl_random.MKLRandomState(1, brng=0).randint(0, 100, 6)
array([0, 0, 0, 0, 0, 0]) # brng="MT19937" gives [13 56 84 80 76 6]

How found

Coverity Scan CID 653949, OVERRUN: "Overrunning callee's array of size 11 by
passing argument __pyx_v_brng_token (which evaluates to 15) in call to
irk_randomseed_mkl". Confirmed against randomkit.h:50, randomkit.cpp:111 and
the .pyx call path, then reproduced per-process on a build of master. The
brng=0 bug surfaced while writing the boundary test.

Distinct from the _seed_impl OUT_OF_BOUNDS finding already dismissed in
coverity/README.md, which is the tuple unpack and is a false positive.

Fix

  • Expose BRNG_KINDS from randomkit.h so the bound tracks the C table.
  • Range-check the integer path; out of range warns and falls back to MT19937,
    as an unrecognized generator name already does.
  • Use brng is not None instead of truthiness, so brng=0 is honoured.

Tests

5 out-of-range values assert warn plus fallback; 0 and 10 assert accepted,
silent, and equal to the same generator selected by name.

@ndgrigorian

Copy link
Copy Markdown
Collaborator

@vchamarthi Seems it can still be broken for MKLRandomState(seed, brng=None) when seed is None for basically the same reason (irk_get_brng_and_stream_mkl returns -1 and passes to brng_list, so we get OOB read).

We should fix that here too

With no stream yet, irk_get_brng_and_stream_mkl returned -1, which then indexed brng_list out of bounds and left the state holding uninitialized memory.
@vchamarthi

Copy link
Copy Markdown
Contributor Author

@vchamarthi Seems it can still be broken for MKLRandomState(seed, brng=None) when seed is None for basically the same reason (irk_get_brng_and_stream_mkl returns -1 and passes to brng_list, so we get OOB read).

We should fix that here too

Fixed in 6d3fa4b. It's broader than seed=None: in __init__ there's no stream yet, so any seed with brng=None hit the -1 index. I made __init__ treat brng=None as "MT19937", the documented default, and added test_init_brng_none_uses_default, which fails before the fix and passes after.

Is defaulting to MT19937 OK, or would you rather brng=None raise a ValueError in the constructor? seed(brng=None) still keeps the current generator.

@antonwolfy antonwolfy added this to the 1.6.0 release milestone Sep 28, 2026
@ndgrigorian

Copy link
Copy Markdown
Collaborator

Is defaulting to MT19937 OK, or would you rather brng=None raise a ValueError in the constructor? seed(brng=None) still keeps the current generator.

Defaulting to MT19937 seems fine

@ndgrigorian ndgrigorian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread CHANGELOG.md
* Extended the `memcpy`-based fast path of `shuffle` to multi-dimensional `ndarray` inputs whose first-axis items are contiguous, which is also much faster than the previous buffered path [gh-159](https://github.com/IntelPython/mkl_random/pull/159)

### Fixed
* Fixed an out-of-range integer `brng` indexing `brng_list` past its end, which returned uninitialized memory as random values; it now warns and falls back to `MT19937`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing PR links

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants