iOS: Fix an image loader crash #11145
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes. 1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash. 2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value, the program will crash because the captured value has already been freed. This explains how the program can crash on `OSAtomicOr32Barrier(1, &cancelled);`: `cancelled` is a captured value that may already have been freed. **Test plan (required)** I built a test app which consistently repros this bug: https://github.com/rigdern/RNImageRepro (see the repro steps in the README). After this change, the test app no longer repros the crash. ** Notes ** While working on this code, some of the design intentions were not clear to me. For example, are the cancelation handlers always supposed to be run on the UI thread? If not and they can be run on multiple threads, I believe it's possible for a cancelation handler to run multiple times (it's not clear to me if that's a bad thing). Is the scenario allowed where both a completion handler and a cancelation handler run for a singe request? I believe this is possible in the current implementation but I wasn't sure if this was intended.
|
By analyzing the blame information on this pull request, we identified @javache and @skatpgusskat to be potential reviewers. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
cc @javache |
|
You're completely right that there's a lack of threading guarantees here. RCTNetworking is mostly meant to run its own queue, but everything image-related blows a big hole in that assumption. I do think the underlying layers should be prepared to accept cancel to be invoked multiple times. RCTNetworkTask already protects against this internally. Thank you so much for fixing this! |
|
@facebook-github-bot shipit |
facebook-github-bot
added
the
Import Started
|
@javache has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
robclouth
pushed a commit
to robclouth/react-native
that referenced
this pull request
Dec 7, 2016
Summary: Fixes facebook#10433 The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes. 1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash. 2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value Closes facebook#11145 Differential Revision: D4261499 Pulled By: javache fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
mkonicek
pushed a commit
that referenced
this pull request
Dec 12, 2016
Summary: Fixes #10433 The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes. 1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash. 2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value Closes #11145 Differential Revision: D4261499 Pulled By: javache fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
gabceb
pushed a commit
to tes/react-native
that referenced
this pull request
Dec 19, 2016
Summary: Fixes facebook#10433 The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes. 1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash. 2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value Closes facebook#11145 Differential Revision: D4261499 Pulled By: javache fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
expbot
pushed a commit
to expo/expo
that referenced
this pull request
Dec 21, 2016
This time facebook/react-native#11145 fbshipit-source-id: 7647834
ide
pushed a commit
to expo/react-native
that referenced
this pull request
Dec 29, 2016
Summary: Fixes facebook#10433 The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes. 1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash. 2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value Closes facebook#11145 Differential Revision: D4261499 Pulled By: javache fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
DanielMSchmidt
pushed a commit
to DanielMSchmidt/react-native
that referenced
this pull request
Jan 4, 2017
Summary: Fixes facebook#10433 The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes. 1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash. 2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value Closes facebook#11145 Differential Revision: D4261499 Pulled By: javache fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
MattFoley
pushed a commit
to skillz/react-native
that referenced
this pull request
Jan 5, 2017
liskiew
pushed a commit
to goodylabs/react-native
that referenced
this pull request
Mar 16, 2017
Summary: Fixes facebook#10433 The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes. 1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash. 2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value Closes facebook#11145 Differential Revision: D4261499 Pulled By: javache fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
liskiew
pushed a commit
to goodylabs/react-native
that referenced
this pull request
Mar 17, 2017
Summary: Fixes facebook#10433 The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes. 1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash. 2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value Closes facebook#11145 Differential Revision: D4261499 Pulled By: javache fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #10433
The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes.
Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash.
Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value, the program will crash because the captured value has already been freed. This explains how the program can crash on
OSAtomicOr32Barrier(1, &cancelled);:cancelledis a captured value that may already have been freed.Test plan (required)
I built a test app which consistently repros this bug: https://github.com/rigdern/RNImageRepro (see the repro steps in the README). After this change, the test app no longer repros the crash.
Notes
While working on this code, some of the design intentions were not clear to me.
For example, are the cancelation handlers always supposed to be run on the UI thread? If not and they can be run on multiple threads, I believe it's possible for a cancelation handler to run multiple times (it's not clear to me if that's a bad thing).
Is the scenario allowed where both a completion handler and a cancelation handler run for a singe request? I believe this is possible in the current implementation but I wasn't sure if this was intended.