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

UIFrameFlash can lead to blocked action errors when opening the Encounter Journal #434

Closed
Meorawr opened this issue May 16, 2023 · 3 comments
Labels
Bug Something isn't working High Priority This issue is considered to be impactful. Mainline This issue affects the current live Retail client

Comments

@Meorawr
Copy link
Collaborator

Meorawr commented May 16, 2023

In patch 10.1.0 a new C_EncounterJournal.OnOpen function was added with protections applied to prevent it being called from an insecure call path. Unfortunately just before this function is called EncounterJournal_OnShow takes a scenic route through the MicroButton animation code which relies on UIFrameFlash.

UIFrameFlash has been historically reported as being a taint swamp (#183, #309) that needs to be purged with searing flame and as such it's trivial to run into cases where taint leeching into UIFrameFlash from other unrelated parts of the UI are breaking the calls to this new shiny and probably-rather-important function.

Test case

  1. Run the following command: /run UIFrameFlash(CreateFrame("Frame"), 1, 1, 2^32)
  2. Open the encounter journal.

As an alternative test case not involving UIFrameFlash but rather the help tips managed by the microbuttons, the following will also result in a blocked action error.

  1. Run the following command: /run MainMenuMicroButton_ShowAlert(SpellbookMicroButton, "Boo")
  2. Open the encounter journal.

Quick patch

Rather than altering the security requirements of the new functions we can probably hopefully just avoid any issues if their calls occurred as the very first thing in the script handler before taking a trip through functions dealing with potentially tainted data.

I've adjusted both the OnShow and OnHide calls with this patch just to preventatively deal with someone else coming along in three months time and adding protections to OnHide, as it would also be impacted by this same issue if it were to change.

diff --git a/Interface/AddOns/Blizzard_EncounterJournal/Blizzard_EncounterJournal.lua b/Interface/AddOns/Blizzard_EncounterJournal/Blizzard_EncounterJournal.lua
index f88179c5..90434379 100644
--- a/Interface/AddOns/Blizzard_EncounterJournal/Blizzard_EncounterJournal.lua
+++ b/Interface/AddOns/Blizzard_EncounterJournal/Blizzard_EncounterJournal.lua
@@ -574,6 +574,7 @@ function EncounterJournal_ResetDisplay(instanceID, instanceType, difficultyID)
 end
 
 function EncounterJournal_OnShow(self)
+	C_EncounterJournal.OnOpen();
 	self:RegisterEvent("SPELL_TEXT_UPDATE");
 	if ( tonumber(GetCVar("advJournalLastOpened")) == 0 ) then
 		SetCVar("advJournalLastOpened", GetServerTime() );
@@ -584,7 +585,6 @@ function EncounterJournal_OnShow(self)
 	UpdateMicroButtons();
 	PlaySound(SOUNDKIT.IG_CHARACTER_INFO_OPEN);
 	EncounterJournal_LootUpdate();
-	C_EncounterJournal.OnOpen();
 
 	if not self.lootJournalView then
 		EncounterJournal_SetLootJournalView(LOOT_JOURNAL_POWERS);
@@ -664,13 +664,13 @@ function EncounterJournal_CheckAndDisplayLootTab()
 end
 
 function EncounterJournal_OnHide(self)
+	C_EncounterJournal.OnClose();
 	self:UnregisterEvent("SPELL_TEXT_UPDATE");
 	UpdateMicroButtons();
 	PlaySound(SOUNDKIT.IG_CHARACTER_INFO_CLOSE);
 	self.searchBox:Clear();
 	EJ_EndSearch();
 	self.shouldDisplayDifficulty = nil;
-	C_EncounterJournal.OnClose();
 end
 
 function EncounterJournal_IsSuggestTabSelected(self)
@Meorawr Meorawr added Bug Something isn't working Mainline This issue affects the current live Retail client labels May 16, 2023
@Meorawr Meorawr added the High Priority This issue is considered to be impactful. label May 23, 2023
@MichaelPriebe
Copy link

I am getting this error after calling EncounterJournal_OpenJournal once it has been open and closed at least one time. It seems the first open is fine, but the second time it taints.

/run EncounterJournal_LoadUI(); EncounterJournal_OpenJournal(2, 63, 95);

  1. Run this.
  2. Close the window.
  3. Run it again.
  4. Taint.

@Mandra900

This comment was marked as off-topic.

@zeptognome
Copy link

Fixed on 10.2.7.54171 PTR. All 3 above test cases run without error.

@Meorawr Meorawr closed this as completed Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working High Priority This issue is considered to be impactful. Mainline This issue affects the current live Retail client
Projects
None yet
Development

No branches or pull requests

4 participants