Skip to content

Attempt at fixing crash when blurring image on iOS - #37614

Closed
sammy-SC wants to merge 1 commit into
react:mainfrom
sammy-SC:export-D46263204
Closed

Attempt at fixing crash when blurring image on iOS#37614
sammy-SC wants to merge 1 commit into
react:mainfrom
sammy-SC:export-D46263204

Conversation

@sammy-SC

Copy link
Copy Markdown
Contributor

Summary:
changelog: [internal]

We do not control what vImageBoxConvolve_ARGB8888 returns, it may return 0. If it does return 0, we will allocate memory chunk of size 0. Yes, malloc will let you do that. Well, it depends on the implementation, but according to the spec it is legal. The only requirement is to by able to call free on that without crash.

If vImageBoxConvolve_ARGB8888 does return 0 and we allocate memory of size 0. Call to vImageBoxConvolve_ARGB8888 with tempBuffer of size 0 will lead to a crash.

The documentation for vImageBoxConvolve_ARGB8888 and tempBuffer states:

To determine the minimum size for the temporary buffer, the first time you call this function pass the kvImageGetTempBufferSize flag. Pass the same values for all other parameters that you intend to use in for the second call. The function returns the required minimum size, which should be a positive value. (A negative returned value indicates an error.) The kvImageGetTempBufferSize flag prevents the function from performing any processing other than to determine the minimum buffer size.

I think the keyword word there is "should be a positive value". 0 is not a positive value.

Differential Revision: D46263204

@facebook-github-botfacebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner fb-exported labels May 29, 2023
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D46263204

@analysis-bot

analysis-bot commented May 29, 2023

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,744,784-1
androidhermesarmeabi-v7a8,056,413+0
androidhermesx869,235,928+0
androidhermesx86_649,086,825-2
androidjscarm64-v8a9,307,489+0
androidjscarmeabi-v7a8,496,713+1
androidjscx869,369,698+1
androidjscx86_649,624,713-1

Base commit: 551bbbc
Branch: main

Summary:
Pull Request resolved: #37614
changelog: [internal]
We do not control what `vImageBoxConvolve_ARGB8888` returns, it may return 0. If it does return 0, we will allocate memory chunk of size 0. Yes, malloc will let you do that. Well, it depends on the implementation, but according to the spec it is legal. The only requirement is to by able to call free on that without crash.
If `vImageBoxConvolve_ARGB8888` does return 0 and we allocate memory of size 0. Call to `vImageBoxConvolve_ARGB8888` with tempBuffer of size 0 will lead to a crash.
[The documentation](https://developer.apple.com/documentation/accelerate/1515945-vimageboxconvolve_argb8888#discussion) for `vImageBoxConvolve_ARGB8888` and tempBuffer states:
> To determine the minimum size for the temporary buffer, the first time you call this function pass the kvImageGetTempBufferSize flag. Pass the same values for all other parameters that you intend to use in for the second call. The function returns the required minimum size, which **should be a positive value**. (A negative returned value indicates an error.) The kvImageGetTempBufferSize flag prevents the function from performing any processing other than to determine the minimum buffer size.
I think the keyword word there is "should be a positive value". 0 is not a positive value.
Reviewed By: javache, yungsters
Differential Revision: D46263204
fbshipit-source-id: 0475b91411b0f54de56bf089f02b5ceaa4f4be3d
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D46263204

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label May 30, 2023
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request has been merged in 5eadd93.

Szymon20000 pushed a commit to Szymon20000/react-native that referenced this pull request Jun 14, 2023
Summary:
Pull Request resolved: react#37614
changelog: [internal]
We do not control what `vImageBoxConvolve_ARGB8888` returns, it may return 0. If it does return 0, we will allocate memory chunk of size 0. Yes, malloc will let you do that. Well, it depends on the implementation, but according to the spec it is legal. The only requirement is to by able to call free on that without crash.
If `vImageBoxConvolve_ARGB8888` does return 0 and we allocate memory of size 0. Call to `vImageBoxConvolve_ARGB8888` with tempBuffer of size 0 will lead to a crash.
[The documentation](https://developer.apple.com/documentation/accelerate/1515945-vimageboxconvolve_argb8888#discussion) for `vImageBoxConvolve_ARGB8888` and tempBuffer states:
> To determine the minimum size for the temporary buffer, the first time you call this function pass the kvImageGetTempBufferSize flag. Pass the same values for all other parameters that you intend to use in for the second call. The function returns the required minimum size, which **should be a positive value**. (A negative returned value indicates an error.) The kvImageGetTempBufferSize flag prevents the function from performing any processing other than to determine the minimum buffer size.
I think the keyword word there is "should be a positive value". 0 is not a positive value.
Reviewed By: javache, yungsters
Differential Revision: D46263204
fbshipit-source-id: baa8fac5b3be6fb5bed02800cd725cc4cf43485a
@raid5

raid5 commented Oct 8, 2024

Copy link
Copy Markdown

👋 was this attempt successful?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.fb-exportedMergedThis PR has been merged.p: FacebookPartner: FacebookPartner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@sammy-SC@facebook-github-bot@analysis-bot@raid5