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

[Plat-9721] Fix race condition in KSMachHeaders #1529

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

kstenerud
Copy link
Contributor

@kstenerud kstenerud commented Mar 3, 2023

Goal

This PR fixes some race conditions in KSMachHeaders

Changeset

  • The image header linked list now uses atomics.
  • The list head is now a dummy entry to simplify the algorithm. The real head is accessed via bsg_mach_headers_get_images().
  • The initial headers list is now created at startup rather than lazily (it was being called at startup every time anyway, so this complication is unnecessary).
  • Reduced the exposed API space, and clearly marked those intended for unit tests.

Testing

Updated unit tests to capture more edge cases.

@github-actions
Copy link

github-actions bot commented Mar 3, 2023

Bugsnag.framework binary size decreased by 368 bytes from 706,696 to 706,328 🎉

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  [ = ]       0  +2.6%    +368    [__LINKEDIT]
  +0.7%    +177  +0.7%    +177    [__TEXT]
  [ = ]       0  +0.5%     +48    __DATA,__bss
  +0.3%     +24  -0.2%     -24    [__DATA]
  +0.9%      +8  +0.9%      +8    __DATA,__data
  -0.3%      -8  -0.3%      -8    __TEXT,__unwind_info
  -0.1%     -25  -0.1%     -25    __TEXT,__cstring
  -1.0%     -32  -1.0%     -32    __DATA,__const
  -0.1%    -112  -0.1%    -112    Symbol Table
  -0.1%    -144  -0.1%    -144    __TEXT,__text
  -0.2%    -256  -0.2%    -256    String Table
  -0.1%    -368  [ = ]       0    TOTAL

Generated by 🚫 Danger

@kstenerud kstenerud force-pushed the PLAT-9721-race-condition-3b branch from a605239 to 7b45362 Compare March 3, 2023 15:37
Comment on lines 208 to 209
BSG_Mach_Header_Info *oldTail = atomic_load(&g_images_tail);
atomic_store(&g_images_tail, newImage);
atomic_store(&oldTail->next, newImage);
Copy link

@lemnik lemnik Mar 7, 2023

Choose a reason for hiding this comment

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

I think we're missing a CAS operation here to make sure the load/store don't race?

Something like:

Suggested change
BSG_Mach_Header_Info *oldTail = atomic_load(&g_images_tail);
atomic_store(&g_images_tail, newImage);
atomic_store(&oldTail->next, newImage);
BSG_Mach_Header_Info *oldTail = atomic_load(&g_images_tail);
for(;;) {
if(atomic_compare_exchange_strong(&g_images_tail, &oldTail, newImage)) {
atomic_store(&oldTail->next, newImage);
break;
}
}

@kstenerud kstenerud force-pushed the PLAT-9721-race-condition-3b branch from 7b45362 to 845f2e2 Compare March 7, 2023 10:22
Copy link

@lemnik lemnik left a comment

Choose a reason for hiding this comment

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

LGTM

@kstenerud kstenerud merged commit 8c1a484 into next Mar 7, 2023
@kstenerud kstenerud deleted the PLAT-9721-race-condition-3b branch March 7, 2023 13:15
@kstenerud kstenerud mentioned this pull request Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants