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.

Sign up for GitHub

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

Jump to bottom

Fix FIRCLSContextInitData crash #11699

Merged
merged 2 commits into from
Sep 6, 2023
Merged

Fix FIRCLSContextInitData crash #11699

merged 2 commits into from
Sep 6, 2023

Conversation

kgrigsby59
Copy link
Contributor

@kgrigsby59 kgrigsby59 commented Aug 18, 2023 •
edited
Loading

I'm getting a crash during startup using 10.13.0, 10.12.0, 10.3.0 and master.
This happens on an iPad (6th generation) running 16.6.

Screenshot 2023-08-18 at 11 52 31 AM

It happens all the time with our app when the Address Sanitizer is on and Detect use of stack after return is on.

Screenshot 2023-08-18 at 3 03 36 PM

The Problem

In [FIRCLSContextManager setupContextWithReport:settings:fileManager],

  1. A instance of the FIRCLSContextInitData struct is assigned to initDataObj but it's still a struct.
  2. The pointer of it is assigned to initData.
  3. The initData pointer is passed to asynchronous functions. The pointer is captured by not what it points at.
  4. FIRCLSContextInitData returns
  5. When the async functions run they try to access the stack and crash.

The Fix

In this PR I've made FIRCLSContextInitData a class and copy the strings into it instead of storing a pointer to the private buffer inside NSString that UTF8String returns. Being a class it will get captured in the async blocks and dealloc'ed after the last use.

Related tickets.

#8883
#10104
#9254
PR #10750

@google-cla
Copy link

google-cla bot commented Aug 18, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@ncooke3
Copy link
Member

ncooke3 commented Aug 18, 2023

Thanks @kgrigsby59! Let me know if you run into any issues with signing the CLA. I'll trigger the CI workflows.

@ncooke3 ncooke3 requested a review from samedson August 18, 2023 19:29
@kgrigsby59
Copy link
Contributor Author 

2023-08-18 10:24:50.860815-0400 DiamondKinetics[8456:688425] [Firebase/Crashlytics] Version 10.14.0
=================================================================
==8456==ERROR: AddressSanitizer: stack-use-after-return on address 0x00016545cd50 at pc 0x0001012ebea0 bp 0x00016fece570 sp 0x00016fece568
READ of size 1 at 0x00016545cd50 thread T7
    #0 0x1012ebe9c in __FIRCLSContextInitialize_block_invoke_2+0x6d8 (/private/var/containers/Bundle/Application/7B6201BC-4D43-4116-BF87-2EC8466ED5C9/DiamondKinetics.app/DiamondKinetics:arm64+0x100febe9c) (BuildId: 233e923b4bbd3a87b5f79d100334e1cc32000000200000000200000000001000)
    #1 0x106df2c90 in __wrap_dispatch_group_async_block_invoke+0xb8 (/private/var/containers/Bundle/Application/7B6201BC-4D43-4116-BF87-2EC8466ED5C9/DiamondKinetics.app/Frameworks/libclang_rt.asan_ios_dynamic.dylib:arm64+0x3ec90) (BuildId: eab26a7029d83ad09a494b0296e24d4525000000100000000000090000041000)
    #2 0x107923f04 in _dispatch_call_block_and_release+0x14 (/usr/lib/system/introspection/libdispatch.dylib:arm64+0x3f04) (BuildId: 8dab90719e243c99820ad15d63901de732000000200000000200000000061000)
    #3 0x10792579c in _dispatch_client_callout+0xc (/usr/lib/system/introspection/libdispatch.dylib:arm64+0x579c) (BuildId: 8dab90719e243c99820ad15d63901de732000000200000000200000000061000)
    #4 0x1079281b4 in _dispatch_queue_override_invoke+0x5c8 (/usr/lib/system/introspection/libdispatch.dylib:arm64+0x81b4) (BuildId: 8dab90719e243c99820ad15d63901de732000000200000000200000000061000)
    #5 0x107937a00 in _dispatch_root_queue_drain+0x160 (/usr/lib/system/introspection/libdispatch.dylib:arm64+0x17a00) (BuildId: 8dab90719e243c99820ad15d63901de732000000200000000200000000061000)
    #6 0x10793836c in _dispatch_worker_thread2+0xcc (/usr/lib/system/introspection/libdispatch.dylib:arm64+0x1836c) (BuildId: 8dab90719e243c99820ad15d63901de732000000200000000200000000061000)
    #7 0x1e25d9b90 in _pthread_wqthread+0xdc (/usr/lib/system/libsystem_pthread.dylib:arm64+0x1b90) (BuildId: de6f7fd7ca453635af3274bc843f8d2932000000200000000200000000061000)
    #8 0x1e25d971c in start_wqthread+0x4 (/usr/lib/system/libsystem_pthread.dylib:arm64+0x171c) (BuildId: de6f7fd7ca453635af3274bc843f8d2932000000200000000200000000061000)

Address 0x00016545cd50 is located in stack of thread T0 at offset 80 in frame
    #0 0x1012f7680 in -[FIRCLSContextManager setupContextWithReport:settings:fileManager:]+0xc (/private/var/containers/Bundle/Application/7B6201BC-4D43-4116-BF87-2EC8466ED5C9/DiamondKinetics.app/DiamondKinetics:arm64+0x100ff7680) (BuildId: 233e923b4bbd3a87b5f79d100334e1cc32000000200000000200000000001000)

  This frame has 1 object(s):
    [32, 104) 'initDataObj' (line 52) <== Memory access at offset 80 is inside this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-use-after-return (/private/var/containers/Bundle/Application/7B6201BC-4D43-4116-BF87-2EC8466ED5C9/DiamondKinetics.app/DiamondKinetics:arm64+0x100febe9c) (BuildId: 233e923b4bbd3a87b5f79d100334e1cc32000000200000000200000000001000) in __FIRCLSContextInitialize_block_invoke_2+0x6d8
Shadow bytes around the buggy address:
  0x0001364cb950: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x0001364cb960: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x0001364cb970: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x0001364cb980: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x0001364cb990: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
=>0x0001364cb9a0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5[f5]f5 f5 f5 f5 f5
  0x0001364cb9b0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x0001364cb9c0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x0001364cb9d0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x0001364cb9e0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x0001364cb9f0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scopAddressSanitizer report breakpoint hit. Use 'thread info -s' to get extended information about the report.
e:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
Thread T7 created by T0 here:
    <empty stack>

==8456==ABORTING

@kgrigsby59
Copy link
Contributor Author

@ncooke3 I'm having trouble with the CLA. My employer signed the CLA and added me to the authorized contributers with my work email. That email is not my primary GitHub one but is listed in my emails.

@kgrigsby59
Copy link
Contributor Author

kgrigsby59 commented Aug 18, 2023 •
edited
Loading

@ncooke3 Is the failing check about the name being too long? FIRCLSContextInitData.h If so what would you suggest. It's named the same as the original struct which wasn't in its own file.

@paulb777
Copy link
Member

@kgrigsby59 Sorry about the trouble with the CLA. Have you worked through the suggestions at https://github.com/firebase/firebase-ios-sdk/pull/11699/checks?check_run_id=16024717651?

We can review and respond to other questions once the PR's CLA is sorted out.

@kgrigsby59 kgrigsby59 force-pushed the kgrigsby/fix_FIRCLSContextInitData_crash branch from f6c2508 to 5b7f026 Compare August 19, 2023 13:51
@kgrigsby59 kgrigsby59 force-pushed the kgrigsby/fix_FIRCLSContextInitData_crash branch from 5b7f026 to aef55cf Compare August 19, 2023 14:26
@paulb777
Copy link
Member

It looks like we need a CLA for the email address - [email protected]

@kgrigsby59
Copy link
Contributor Author

Yes, we're trying. My employer signed the agreement yesterday. And i'm added via a google group. But when I go to the link you provided it says the CLA is still missing.

@kgrigsby59
Copy link
Contributor Author

Can you check if the corporate agreement is still being processed?

@paulb777
Copy link
Member

We'll check next week. cc: @morganchen12

@kgrigsby59
Copy link
Contributor Author

It looks like the CLA is sorted out now.

@paulb777 paulb777 closed this Aug 21, 2023
@paulb777 paulb777 reopened this Aug 21, 2023
@paulb777 paulb777 closed this Aug 22, 2023
@paulb777 paulb777 reopened this Aug 22, 2023
@paulb777
Copy link
Member

Just to verify, your commit email [email protected] is included in the google group associated with the corporate CLA? https://cla.developers.google.com/about#add-contributors

@kgrigsby59
Copy link
Contributor Author

Yes

@kgrigsby59
Copy link
Contributor Author

kgrigsby59 commented Sep 4, 2023 •
edited
Loading

@paulb777 Is there something I’m supposed to be doing to move this along?

@samedson
Copy link
Contributor

samedson commented Sep 6, 2023

@kgrigsby59 sorry for the delay. Going to run a couple smoke tests and review this today

samedson
samedson approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@samedson samedson left a comment

Choose a reason for hiding this comment

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

Thank you for contributing this fix!

@samedson samedson merged commit 6830729 into firebase:master Sep 6, 2023
@kgrigsby59 kgrigsby59 deleted the kgrigsby/fix_FIRCLSContextInitData_crash branch September 8, 2023 06:32
andrewheard pushed a commit that referenced this pull request Sep 20, 2023
@andrewheard
Fix FIRCLSContextInitData crash (#11699)
d886ffa
@firebase firebase locked and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@samedson samedson samedson approved these changes

Assignees
No one assigned
Labels
api: crashlytics cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kgrigsby59 @ncooke3 @paulb777 @samedson @morganchen12