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

Issue/423 prevent index overflows #424

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

joshheald
Copy link
Contributor

Fixes #423

This is to resolve a crash which happened when -[WPPHAssetDataSource photoLibraryDidChange:] was called,with an invalid (seeming) changedIndexes value.

To test:

I wasn't ever able to actually reproduce the crash, but I believe it happened due to a combination of:

  1. Indexes which were greater than the count, leading to:
  2. Overflows to a very high UInt when doing the arithmetic in -[WPPHAssetDataSource adjustedIndexForIndex:forCount:]
  3. Those not being properly checked before they were passed to -[NSMutableIndexSet addIndex:], which requires that the UInts it's passed be between 0 and NSIntegerMax -1.

I've made a few tweaks to avoid that:

  1. Throw an exception if we try to adjust an index which is greater than the count. It will lead to a negative number, which makes no sense for an index
  2. Don't even attempt to adjust the index in -[WPPHAssetDataSource adjustedIndexesForIndexSet:forCount:] if it's greater than the count.
  3. Use Apple's record of the count of assets before the changeset, not our assetCount which may be out of date.

I've tested it by adding to the indexes of any changes in photoLibraryDidChange, before passing them to the adjust functions, and checking that it doesn't crash. I tested in the Woo app.

Testing steps:

  1. Launch the app using the new version of MediaPicker
  2. Open a product, and add an image to it
  3. Go to the camera and take some photos
  4. Go to Photos and edit some of the recent photos, and delete some too
  5. Go back to woo and check it doesn't crash

  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

From Apple’s docs:

`index
Index to add. Must be in the range 0 .. NSNotFound - 1.`

This has caused crashes in the Woo app when we’ve had overflows from the `adjustedIndexForIndex` function’s arithmetic. Those overflows shouldn’t happen from here now, but this gives extra protection, and is closer to the previously existing check (that used != NSNotFound, but that was ineffective against a UInt as NSNotFound is integer max, not uint max.)
@joshheald joshheald added the bug label Feb 28, 2025
Copy link
Contributor

@AliSoftware AliSoftware Feb 28, 2025

Choose a reason for hiding this comment

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

⚠️ You need to run bundle exec pod install in the Example project to also update its Podfile.lock so it accounts for that version bump

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants