Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for out of bound reads #523

Merged
merged 7 commits into from
Jul 15, 2022
Merged

Check for out of bound reads #523

merged 7 commits into from
Jul 15, 2022

Conversation

Arjan-Zuidema
Copy link
Contributor

@Arjan-Zuidema Arjan-Zuidema commented Jul 13, 2022

Description

When using a MobX store for the data a warning will be triggered from MobX about out of bounds read (index). This is because the length of the array is not checked before accessing.

This patch checks the length of the array before reading the value by index.

Checklist

@ghost ghost added the cla-needed label Jul 13, 2022
@ghost ghost removed the cla-needed label Jul 13, 2022
@Arjan-Zuidema Arjan-Zuidema marked this pull request as ready for review July 13, 2022 10:10
src/FlashList.tsx Outdated Show resolved Hide resolved
src/FlashList.tsx Outdated Show resolved Hide resolved
This also gets rid of the dataLength variable
Comment on lines 554 to 556
if (trailingItem === undefined) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry fora yet-another change, I always find some small improvement 😄

This check should no longer be necessary since we ensure in the previous if that trailingItem exists

Suggested change
if (trailingItem === undefined) {
return null;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. Also fixed!

@naqvitalha naqvitalha merged commit 2efb3f2 into Shopify:main Jul 15, 2022
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