Skip to content

[Runtime] Fix 32-bit outer-loop counters in omTensorSort and omTensor… - #3622

Open
jonathanhuang-bot wants to merge 3 commits into
onnx:mainfrom
jonathanhuang-bot:fix/f039-32bit-loop-counter-sort
Open

[Runtime] Fix 32-bit outer-loop counters in omTensorSort and omTensor…#3622
jonathanhuang-bot wants to merge 3 commits into
onnx:mainfrom
jonathanhuang-bot:fix/f039-32bit-loop-counter-sort

Conversation

@jonathanhuang-bot

Copy link
Copy Markdown
Collaborator

…TopK

Both omTensorSort (OMSort.c) and omTensorTopK (OMTopK.c) iterate over a tensor's outer 5 dimensions using a 6-element int64_t shape[] array, but declared their loop counters as plain 'int' (32-bit signed). When any outer dimension genuinely reaches INT_MAX+1 (2,147,483,648), the counter wraps to a large negative value on signed overflow (undefined behavior; wraps in practice under all mainstream compilers). Because the comparison promotes 'int' to int64_t, the wrapped negative value still compares less than the large positive shape[i], so the loop never exits via its counter. Each iteration computes a per-slice pointer offset from the now-negative dim values and passes it directly into the sort/heap routines -- a wild out-of-bounds read/write into whatever heap memory sits before the tensor's buffers.

Fix: change all five outer-loop counter declarations (dim0..dim4) from 'int' to 'int64_t' in both functions. This is the complete fix: once the counters match the width of the bound they are compared against, the overflow cannot occur. No caller changes needed -- both omTensorSort and omTensorTopK are internal runtime functions with a fixed call-site enumeration (NonMaxSuppression.cpp and TopK.cpp lowerings respectively, confirmed by grep).

Fixes: f039 (32-bit loop counters iterate int64 tensor dimensions)

Analysis file for this bug:
disposition-f039-analysis.md

…TopK

Both omTensorSort (OMSort.c) and omTensorTopK (OMTopK.c) iterate over a
tensor's outer 5 dimensions using a 6-element int64_t shape[] array, but
declared their loop counters as plain 'int' (32-bit signed). When any
outer dimension genuinely reaches INT_MAX+1 (2,147,483,648), the counter
wraps to a large negative value on signed overflow (undefined behavior;
wraps in practice under all mainstream compilers). Because the comparison
promotes 'int' to int64_t, the wrapped negative value still compares less
than the large positive shape[i], so the loop never exits via its counter.
Each iteration computes a per-slice pointer offset from the now-negative
dim values and passes it directly into the sort/heap routines -- a wild
out-of-bounds read/write into whatever heap memory sits before the tensor's
buffers.

Fix: change all five outer-loop counter declarations (dim0..dim4) from
'int' to 'int64_t' in both functions. This is the complete fix: once the
counters match the width of the bound they are compared against, the
overflow cannot occur. No caller changes needed -- both omTensorSort and
omTensorTopK are internal runtime functions with a fixed call-site
enumeration (NonMaxSuppression.cpp and TopK.cpp lowerings respectively,
confirmed by grep).

Fixes: f039 (32-bit loop counters iterate int64 tensor dimensions)
Signed-off-by: Jonathan Huang <jonathanhuang@ibm.com>
@jenkins-droid

Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

@AlexandreEichenberger AlexandreEichenberger 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

one comment (from AI)

One unrelated thing I noticed while reading: shape[i + (6 - rank)] at line 363 computes 6 - rank in uint64_t. The rank <= 6 guard on line 317 is an assert, so under NDEBUG a rank-7 tensor underflows that index into a huge value and writes past the two 6-element stack arrays. If this entry point can be reached with unvalidated rank, that deserves a real check rather than an assert

so you can the code pattern below.

  // Enforced in release builds too: rank > 6 would underflow the (6 - rank)
  // index below and write past shape[]/strides[]; axis != rank - 1 is
  // unsupported by the offset computation.
  if (rank == 0 || rank > 6 || axis != (rank - 1)) {
    fprintf(stderr,
        "omTensorSort: unsupported rank %llu / axis %llu "
        "(requires 1 <= rank <= 6 and axis == rank - 1)\n",
        (unsigned long long)rank, (unsigned long long)axis);
    return;
  }

Its an issue we have elsewhere too, but it might be good to start to address is as we edit functions for safety anyway.

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