Fix a possible nil-dereference crash #478
Merged
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
krnowak
requested review from
iredelmeier,
jmacd,
lizthegrey,
paivagustavo,
rghetia and
tedsuo
as code owners
krnowak
force-pushed
the
fix-possible-crash
branch
from
9eabb39 to
641a9d3
Compare
lizthegrey
reviewed
Feb 16, 2020
There is a nil dereference crash if we perform some operations in certain order: - get a global meter - create an instrument - bind it - set the delegate - unbind the instrument - call some recording function on the not-really-bound-anymore instrument Unbind will run the no op run-once initialization routine, so the follow-up RecordOne call will not run it's initialization routine. Which RecordOne's initialization routine being skipped, the delegate to bounded instrument is not set, but the code is still trying to get a pointer to it and then unconditionally dereference it. Add an extra check for a nil pointer - if this is true, then Unbind was first and RecordOne should effectively be a no op.
krnowak
force-pushed
the
fix-possible-crash
branch
from
641a9d3 to
8890a30
Compare
|
Updated, added the test. |
MrAlias
approved these changes
Feb 19, 2020
rghetia
approved these changes
Feb 19, 2020
jmacd
approved these changes
Feb 19, 2020
| func TestUnbindThenRecordOne(t *testing.T) { | ||
| internal.ResetForTest() | ||
|
|
||
| // Note: this test uses oppsite Float64/Int64 number kinds |
There was a problem hiding this comment.
Oops, copy pasta. Will fix in a separate PR.
Merged
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.
If we get a bounded instrument from an instrument with an already set
delegate and then concurrently call Unbind and RecordOne on it, it is
possible that Unbind will trigger the sync.Once "initialize" member
with a no op function, so the sync.Once "initialize" in RecordOne will
not be triggered anymore. This in turn will not set the implPtr to the
real bound instrument, so an atomic load later in RecordOne will again
return a nil pointer which we unconditionally dereference.
Add an extra check for a nil pointer - if this is true, then Unbind
was first and RecordOne should effectively be a no op.