Skip to content

fix(array): retain dictionary before replacement#926

Merged
zeroshade merged 2 commits into
apache:mainfrom
fallintoplace:fix/data-set-dictionary-lifetime
Jul 13, 2026
Merged

fix(array): retain dictionary before replacement#926
zeroshade merged 2 commits into
apache:mainfrom
fallintoplace:fix/data-set-dictionary-lifetime

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Data.SetDictionary released the existing dictionary before retaining the replacement. Replacing a dictionary with the same pointer while the parent held the final reference could therefore clear the dictionary before resurrecting its reference.

This retains the validated replacement first, swaps it into the Data object, then releases the previous dictionary. It also handles nil input and reports an explicit panic for an invalid dictionary data implementation.

Tests:

  • go test ./arrow/array -run 'TestData(SetDictionaryWithSamePointerRetainsDictionary|ResetClearsDictionary)'
  • go test ./arrow/array
  • go test ./arrow/... -run '^$'

@fallintoplace fallintoplace requested a review from zeroshade as a code owner July 11, 2026 07:47

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correct fix for a real use-after-free — releasing the old dictionary before retaining the replacement could free-then-resurrect a cleared *Data when the parent held the last reference. The retain-before-release ordering now matches how Reset handles buffers/children, and the regression test captures the exact case. One test nit inline.

Comment thread arrow/array/data.go
d.dictionary = dict.(*Data)

if newDict != nil {
newDict.Retain()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Retain-before-release ordering is correct. Please add a SetDictionary(nil) test (clearing is now supported per the PR description but untested), and optionally one asserting the explicit panic for a non-*array.Data implementation.

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Retaining the replacement dictionary before releasing the old one is the correct fix, and the same-pointer replacement / nil-clear tests cover it well. Thanks!

@zeroshade zeroshade merged commit 168951e into apache:main Jul 13, 2026
23 checks passed
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.

2 participants