(In reply to Ray Kraesig [:rkraesig] from comment #23) > Also re-*ni?*‍ing :yannis for the same reason as last time: do you think this calls for an accelerated uplift of the second patch? (Sorry: I misunderstood where we were in the cycle, and was too hasty in cancelling it yesterday.) Taking in the first patch without the second should bring us back to a similar situation as before bug 1842088, where the race condition would still exist but with mostly benign consequences. For people who would have crashed here, I think we would now instead be failing to honor the dynamic DLL blocklist (the static blocklist should still work), because `sWriteCopyView` would stay `nullptr` in the main process. I am a bit worried that there are scenarios where the race condition could theoretically, I think, lead to a different crash. However, if these variations could realistically occur timing-wise, I think we should already be seeing them, and I don't find crashes that would match. Here is what they would look like: ``` MainThread | Third-Party Thread ---------- | ------------------ main() | | LoadLibrary("thirdparty.dll") | patched_NtMapViewOfSection() | stub_NtMapViewOfSection() | AfterMapViewOfExecutableSection() | SharedSection::GetKernel32Exports() | return &writeCopyView->mK32Exports SharedSection::ConvertToReadOnly() | SharedSection::Reset() | nt::~AutoMappedView | nt::AutoMappedView::Unmap | | ... in AfterMapViewOfExecutableSection(), | k32Exports points to unmapped memory ... v MainThread | Third-Party Thread ---------- | ------------------ main() | | LoadLibrary("thirdparty.dll") | patched_NtMapViewOfSection() | stub_NtMapViewOfSection() | AfterMapViewOfExecutableSection() | SharedSection::SearchBlocklist() | writeCopyView->SearchBlocklist() SharedSection::ConvertToReadOnly() | SharedSection::Reset() | nt::~AutoMappedView | nt::AutoMappedView::Unmap | | ... in SharedSection::Layout::SearchBlocklist(), | the this pointer points to unmapped memory, | and/or we return a pointer to unmapped memory ... v ``` Overall, I hope both patches ultimately make it to 118 release, but waiting for a week before asking the second patch's uplift sounds fine to me.
Bug 1850969 Comment 24 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(In reply to Ray Kraesig [:rkraesig] from comment #23) > Also re-*ni?*‍ing :yannis for the same reason as last time: do you think this calls for an accelerated uplift of the second patch? (Sorry: I misunderstood where we were in the cycle, and was too hasty in cancelling it yesterday.) Taking in the first patch without the second should bring us back to a similar situation as before bug 1842088, where the race condition would still exist but with mostly benign consequences. For people who would have crashed here, I think we would now instead be failing to honor the dynamic DLL blocklist (the static blocklist should still work), because `sWriteCopyView` would stay `nullptr` in the main process. I am a bit worried that there are scenarios fixed where the race condition could theoretically, I think, lead to different crashes (the second patch already fixes these scenarios as well). However, if these variations could realistically occur timing-wise, I think we should already be seeing them, and I don't find crashes that would match. Here is what they would look like: ``` MainThread | Third-Party Thread ---------- | ------------------ main() | | LoadLibrary("thirdparty.dll") | patched_NtMapViewOfSection() | stub_NtMapViewOfSection() | AfterMapViewOfExecutableSection() | SharedSection::GetKernel32Exports() | return &writeCopyView->mK32Exports SharedSection::ConvertToReadOnly() | SharedSection::Reset() | nt::~AutoMappedView | nt::AutoMappedView::Unmap | | ... in AfterMapViewOfExecutableSection(), | k32Exports points to unmapped memory ... v MainThread | Third-Party Thread ---------- | ------------------ main() | | LoadLibrary("thirdparty.dll") | patched_NtMapViewOfSection() | stub_NtMapViewOfSection() | AfterMapViewOfExecutableSection() | SharedSection::SearchBlocklist() | writeCopyView->SearchBlocklist() SharedSection::ConvertToReadOnly() | SharedSection::Reset() | nt::~AutoMappedView | nt::AutoMappedView::Unmap | | ... in SharedSection::Layout::SearchBlocklist(), | the this pointer points to unmapped memory, | and/or we return a pointer to unmapped memory ... v ``` Overall, I hope both patches ultimately make it to 118 release, but waiting for a week before asking the second patch's uplift sounds fine to me.
(In reply to Ray Kraesig [:rkraesig] from comment #23) > Also re-*ni?*‍ing :yannis for the same reason as last time: do you think this calls for an accelerated uplift of the second patch? (Sorry: I misunderstood where we were in the cycle, and was too hasty in cancelling it yesterday.) Taking in the first patch without the second should bring us back to a similar situation as before bug 1842088, where the race condition would still exist but with mostly benign consequences. For people who would have crashed here, I think we would now instead be failing to honor the dynamic DLL blocklist (the static blocklist should still work), because `sWriteCopyView` would stay `nullptr` in the main process. I am a bit worried that there are scenarios where the race condition could theoretically, I think, lead to different crashes (the second patch already fixes these scenarios as well). However, if these variations could realistically occur timing-wise, I think we should already be seeing them, and I don't find crashes that would match. Here is what they would look like: ``` MainThread | Third-Party Thread ---------- | ------------------ main() | | LoadLibrary("thirdparty.dll") | patched_NtMapViewOfSection() | stub_NtMapViewOfSection() | AfterMapViewOfExecutableSection() | SharedSection::GetKernel32Exports() | return &writeCopyView->mK32Exports SharedSection::ConvertToReadOnly() | SharedSection::Reset() | nt::~AutoMappedView | nt::AutoMappedView::Unmap | | ... in AfterMapViewOfExecutableSection(), | k32Exports points to unmapped memory ... v MainThread | Third-Party Thread ---------- | ------------------ main() | | LoadLibrary("thirdparty.dll") | patched_NtMapViewOfSection() | stub_NtMapViewOfSection() | AfterMapViewOfExecutableSection() | SharedSection::SearchBlocklist() | writeCopyView->SearchBlocklist() SharedSection::ConvertToReadOnly() | SharedSection::Reset() | nt::~AutoMappedView | nt::AutoMappedView::Unmap | | ... in SharedSection::Layout::SearchBlocklist(), | the this pointer points to unmapped memory, | and/or we return a pointer to unmapped memory ... v ``` Overall, I hope both patches ultimately make it to 118 release, but waiting for a week before asking the second patch's uplift sounds fine to me.
(In reply to Ray Kraesig [:rkraesig] from comment #23) > Also re-*ni?*‍ing :yannis for the same reason as last time: do you think this calls for an accelerated uplift of the second patch? (Sorry: I misunderstood where we were in the cycle, and was too hasty in cancelling it yesterday.) Taking in the first patch without the second should bring us back to a similar situation as before bug 1842088, where the race condition would still exist but with mostly benign consequences. For people who would have crashed here, I think we would now instead be failing to honor the dynamic DLL blocklist (the static blocklist should still work), because `sWriteCopyView` would stay `nullptr` in the main process. I am a bit worried that there are scenarios where the race condition could theoretically, I think, lead to different crashes (the second patch already fixes these scenarios as well). However, if these variations could realistically occur timing-wise, I think we should already be seeing them, and I don't find crashes that would match. Here is what they would look like: ``` MainThread | Third-Party Thread ---------- | ------------------ main() | | LoadLibrary("thirdparty.dll") | patched_NtMapViewOfSection() | stub_NtMapViewOfSection() | AfterMapViewOfExecutableSection() | SharedSection::GetKernel32Exports() | return &writeCopyView->mK32Exports SharedSection::ConvertToReadOnly() | SharedSection::Reset() | nt::~AutoMappedView | nt::AutoMappedView::Unmap | | ... in AfterMapViewOfExecutableSection(), | k32Exports points to unmapped memory ... v ``` ``` MainThread | Third-Party Thread ---------- | ------------------ main() | | LoadLibrary("thirdparty.dll") | patched_NtMapViewOfSection() | stub_NtMapViewOfSection() | AfterMapViewOfExecutableSection() | SharedSection::SearchBlocklist() | writeCopyView->SearchBlocklist() SharedSection::ConvertToReadOnly() | SharedSection::Reset() | nt::~AutoMappedView | nt::AutoMappedView::Unmap | | ... in SharedSection::Layout::SearchBlocklist(), | the this pointer points to unmapped memory, | and/or we return a pointer to unmapped memory ... v ``` Overall, I hope both patches ultimately make it to 118 release, but waiting for a week before asking the second patch's uplift sounds fine to me.