Skip to content

GH-44827: [C++] Skip null slots when transposing dictionary indices - #54686

Closed
Nayana-Naik73 wants to merge 1 commit into
apache:mainfrom
Nayana-Naik73:dict-transpose-null-indices
Closed

Nayana-Naik73 wants to merge 1 commit into
apache:mainfrom
Nayana-Naik73:dict-transpose-null-indices

Conversation

@Nayana-Naik73

@Nayana-Naik73 Nayana-Naik73 commented Oct 20, 2026 •

Copy link
Copy Markdown

Rationale for this change

TransposeDictIndices (behind DictionaryArray::Transpose, DictionaryArray::Compact, DictionaryUnifier::UnifyChunkedArray/UnifyTable, and the hash kernels when chunks have different dictionaries) passes every index to TransposeInts, null slots included, and TransposeInts reads transpose_map[index]. Index values under null slots are unspecified, and ValidateFull() deliberately does not bounds-check them. So a dictionary array with a large or negative index under a null slot validates fine and then reads far outside transpose_map. This is the crash described in GH-44827.

You can hit it from untrusted input: write an IPC stream with two batches whose dictionary<int32, utf8> column has different dictionaries and 1 << 30 stored under a null index. Read it back with RecordBatchStreamReader. Both batches pass ValidateFull(), and then DictionaryUnifier::UnifyChunkedArray dies under ASan with a BUS/READ fault in TransposeInts (int_util.cc:427, called from array_dict.cc:206). ConcatenateDictionaryIndices already skips null runs for this reason. The transpose path never got the same treatment.

What changes are included in this PR?

TransposeDictIndices zeroes the output indices when the input may have nulls and transposes only the valid runs with VisitSetBitRuns. Valid indices are transposed exactly as before.

Are these changes tested?

Yes. TestDictionary.TransposeNullsWithOutOfBoundsIndex builds a dictionary array that passes ValidateFull() and has out-of-range values (1 << 30 and -(1 << 30)) under its null slots. It checks Transpose, a sliced Transpose, and Compact. Without the fix it crashes in TransposeInts; with the fix it passes. arrow-array-test and arrow-compute-vector-test pass in an ASan debug build.

Are there any user-facing changes?

No, other than the crash being fixed. Null slots in a transposed index buffer now hold 0 instead of an arbitrary value.

This PR contains a "Critical Fix". It fixes an out-of-bounds read reachable from arrays that pass ValidateFull(), for example dictionary-encoded columns read from an untrusted IPC stream and then unified or compacted.

Was AI used for this PR?

In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.

PR code and description written by:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

This was prepared with AI assistance (the patch, the regression test and this description), and I reviewed it before submitting. I checked it against the ASan reproducer and the test above.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #44827 has been automatically assigned in GitHub to PR creator.

@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

This pull request has been automatically closed because you currently have 4 open pull requests, which is more than the limit of 3.

Due to the increase in pull requests opened by AI bots, and in order to keep the review queue manageable, Apache Arrow limits contributors without repository access to at most 3 concurrently open pull requests. This helps make sure each pull request gets the attention it needs and that work in progress does not go stale.

Once one of your other open pull requests has been merged or closed, you are welcome to reopen this one.

See also:

@github-actions github-actions Bot closed this Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant