EnsurePackage*Async() handling options.RegisterNewerIfAvailable(true). RegisterPackageByPackageFull/FamilyNameAsync crash. Test + Warning fixes #4845
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
DrusTheAxe
added
area-Deployment
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
DrusTheAxe
changed the title
DrusTheAxe
requested review from
sotteson1,
cwruss,
ryalanms,
wcheng-msft,
jlnordin,
letao-msft,
rhuang-msft and
sachintaMSFT
…yPackageFullNameAsync() take their target parameter as const hstring& resulting in the reference captured by their implementations' co_await and not the actual value. The caller deallocates the memory before Register... is done with it potentially leading to BadMojo(TM). Tests don't notice it as they're too simple to notice this use-after-free -- the memory may be deallocated but the memory's not overwritten (yet) in simple loads (like the tests). Larger scale (real world) use with more diverse memory patterns notice the problem (usually as a crash).
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…b.com/microsoft/WindowsAppSDK into user/howardk/RegisterNewerIfAvailable
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
DrusTheAxe
changed the title
…elpful and negatively helpful (400+ warnings that are, for us, features).
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
DrusTheAxe
added a commit
that referenced
this pull request
Nov 8, 2024
…. RegisterPackageByPackageFull/FamilyNameAsync crash. Test + Warning fixes (#4845) EnsurePackage*Async() not handling options.RegisterNewerIfAvailable(true) RegisterPackageByPackageFullNameAsync() and RegisterPackageByPackageFullNameAsync() take their target parameter as const hstring& resulting in the reference captured by their implementations' co_await and not the actual value. The caller deallocates the memory before Register... is done with it potentially leading to BadMojo(TM). Tests don't notice it as they're too simple to notice this use-after-free -- the memory may be deallocated but the memory's not overwritten (yet) in simple loads (like the tests). Larger scale (real world) use with more diverse memory patterns notice the problem (usually as a crash). ApplicationDataTests_Elevated test cases were blocked state due to user context vs environmental state vs access control (ACLs whee!). Changed RunAs and associated test process setup to play nice together. Fixed some PackageManagerTests that had incomplete state setup potentially causing false errors if a past previous test failed. Fixed 400+ build warnings (mostly CS8305 due to [Experimental] attribute, almost all from OAuth generated C#/WinRT) Removed redundant .props import (fixing a VS warning) Fixed misleading log message Removed wrong test cases Added diagnostics to all Setup fixtures to hunt down Setup failures Change IsReady tests to RunAs RestrictedUser Fixed compiler warnings Split ApplicationDataTests_Elevated to separate source file. Added diagnostics to ApplicationDataTests[_Elevated]. Moved TAEF display+compare support for C++/WinRT hstring into shared file (test/inc/WindowsAppRuntime.Test.TAEF.cppwinrt.h). Added Parameters to ApplicationData tests to (hopefully) troubleshoot failure. Added whomi /all to build pipeline test output. Changed ApplicationData tests' default to IsolationLevel=Class (not TAEF's default =Module) Changed PackageManager tests' default to IsolationLevel=Class (not TAEF's default =Module) Factored tests into separate runs to aid troubleshooting failures Added suppression of CS8305 (Feature is experimental... duh) as not helpful and negatively helpful (400+ warnings that are, for us, features). https://task.ms/54835036 https://task.ms/54858998 https://task.ms/54884960
DrusTheAxe
added a commit
that referenced
this pull request
Nov 9, 2024
…fAvailable(true). RegisterPackageByPackageFull/FamilyNameAsync crash. Test + Warning fixes (#4845) (#4864) * EnsurePackage*Async() handling options.RegisterNewerIfAvailable(true). RegisterPackageByPackageFull/FamilyNameAsync crash. Test + Warning fixes (#4845) EnsurePackage*Async() not handling options.RegisterNewerIfAvailable(true) RegisterPackageByPackageFullNameAsync() and RegisterPackageByPackageFullNameAsync() take their target parameter as const hstring& resulting in the reference captured by their implementations' co_await and not the actual value. The caller deallocates the memory before Register... is done with it potentially leading to BadMojo(TM). Tests don't notice it as they're too simple to notice this use-after-free -- the memory may be deallocated but the memory's not overwritten (yet) in simple loads (like the tests). Larger scale (real world) use with more diverse memory patterns notice the problem (usually as a crash). ApplicationDataTests_Elevated test cases were blocked state due to user context vs environmental state vs access control (ACLs whee!). Changed RunAs and associated test process setup to play nice together. Fixed some PackageManagerTests that had incomplete state setup potentially causing false errors if a past previous test failed. Fixed 400+ build warnings (mostly CS8305 due to [Experimental] attribute, almost all from OAuth generated C#/WinRT) Removed redundant .props import (fixing a VS warning) Fixed misleading log message Removed wrong test cases Added diagnostics to all Setup fixtures to hunt down Setup failures Change IsReady tests to RunAs RestrictedUser Fixed compiler warnings Split ApplicationDataTests_Elevated to separate source file. Added diagnostics to ApplicationDataTests[_Elevated]. Moved TAEF display+compare support for C++/WinRT hstring into shared file (test/inc/WindowsAppRuntime.Test.TAEF.cppwinrt.h). Added Parameters to ApplicationData tests to (hopefully) troubleshoot failure. Added whomi /all to build pipeline test output. Changed ApplicationData tests' default to IsolationLevel=Class (not TAEF's default =Module) Changed PackageManager tests' default to IsolationLevel=Class (not TAEF's default =Module) Factored tests into separate runs to aid troubleshooting failures Added suppression of CS8305 (Feature is experimental... duh) as not helpful and negatively helpful (400+ warnings that are, for us, features). https://task.ms/54835036 https://task.ms/54858998 https://task.ms/54884960 * Fixed (sync'd) dependencies * Added missing dependency to Version.Details.xml and ran DevCheck -SyncDepednencies * Removed unused files accidentally brought in via the cherrypick from main * Removed some whitespace to see if a file change will fix a git problem
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.
EnsurePackage*Async() not handling options.RegisterNewerIfAvailable(true)
RegisterPackageByPackageFullNameAsync()andRegisterPackageByPackageFullNameAsync()take their target parameter asconst hstring&resulting in the reference captured by their implementations'co_awaitand not the actual value. The caller deallocates the memory beforeRegister...is done with it potentially leading to BadMojo(TM). Tests don't notice it as they're too simple to notice this use-after-free -- the memory may be deallocated but the memory's not overwritten (yet) in simple loads (like the tests). Larger scale (real world) use with more diverse memory patterns notice the problem (usually as a crash).ApplicationDataTests_Elevated test cases are blocked state due to user context vs environmental state vs access control (ACLs whee!). Changed RunAs and associated test process setup to play nice together.
Fixed some PackageManagerTests that had incomplete state setup potentially causing false errors if a past previous test failed.
Fixed 400+ build warnings (mostly CS8305 due to [Experimental] attribute, almost all from OAuth generated C#/WinRT)
https://task.ms/54835036
https://task.ms/54858998
https://task.ms/54884960