Skip to content

Fix an index error in ArenaVectorBase::insertAt - #3486

Merged
aheejin merged 1 commit into
WebAssembly:masterfrom
aheejin:insert_at_fix
Jan 13, 2021
Merged

Fix an index error in ArenaVectorBase::insertAt#3486
aheejin merged 1 commit into
WebAssembly:masterfrom
aheejin:insert_at_fix

Conversation

@aheejin

@aheejin aheejin commented Jan 13, 2021

Copy link
Copy Markdown
Member

Because resize() sets usedElements to its argument, we were
accessing data[usedElements], which can be outside of allocated memory
depending the internal state, i.e., allocatedElements's value.

It is hard to come up with a test case for this because apparently the
failure condition depends on the vector's internal state.

Because `resize()` sets `usedElements` to its argument, we were
accessing `data[usedElements]`, which can be outside of allocated memory
depending the internal state, i.e., `allocatedElements`'s value.

It is hard to come up with a test case for this because apparently the
failure condition depends on the vector's internal state.
@aheejin
aheejin requested a review from dcodeIO January 13, 2021 14:50
@aheejin aheejin changed the title Fix an index error in ArenaVectorBase Fix an index error in ArenaVectorBase::insertAt Jan 13, 2021

@dcodeIO dcodeIO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ouch. Good find!

@aheejin
aheejin merged commit f882781 into WebAssembly:master Jan 13, 2021
@aheejin
aheejin deleted the insert_at_fix branch January 13, 2021 16:52
kripken added a commit that referenced this pull request Jan 14, 2021
Followup to #3486, I wonder if it isn't a little more clear this way,
which avoids the confusion of usedElements being changed
while we are using it.

In general I think it's best to only use usedElements in the most
internal methods, and to call size() otherwise.
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