Need help with SpellBook taint

[LanceDH]

I'm currently working on an addon that interracts with the SpellBook but I seem to taint it which causes errors when trying to cast a spell from the book (because CastSpell() is protected).

The idea is that I have a button showing a spell, and when clicked it opens the SpellBook to the page where the spell is located.
As far as I've seen there's no function for this so I basically go through all the tabs, pages and slots to find the correct spell.
The issue is that this, and even just toggling the SpellBook, will taint it.
So I'm trying to figure out if there's a way I can fix it.
Full code can be found on my GitHub

Once the button is pressed the only relevant code is the following:
(line 122- 141)
It get the name of the spell, then calls the function to scout through the spellbook for that name.
If it's found it plays a highlight animation, else it just opens on the default page.
And then it opens the SpellBook if it's not open yet.

elseif (self.unlockType == UNLOCKTYPE_SPELL) then
    local spellName = self.SpellName:GetText();
    local success, slot = OpenSpellBookAtSpell(spellName);
    if success then
        ILW_SpellBookHighlight:ClearAllPoints();
        ILW_SpellBookHighlight:SetPoint("TOPLEFT", slot, "TOPRIGHT");
        ILW_SpellBookHighlight:Show();
        ILW_SpellBookHighlight.increasing = true;
        ILW_SpellBookHighlight:SetAlpha(0); 
    else
        print("|cFFFFD100ILWhat:|r |cFFFF5555" .. spellName .." is not in the spellbook. It might be hidden.|r");
        SpellBookSkillLineTab_OnClick(_G["SpellBookSkillLineTab2"]);
        while (SpellBookPrevPageButton:IsEnabled()) do
            SpellBookPrevPageButton_OnClick();
        end
    end
        
    if (not SpellBookFrame:IsShown()) then
        ToggleSpellBook(BOOKTYPE_SPELL);
    end

The function to scout works like this:
(line 47 - 105)
It simply starts at the first tab, checks all the slots until it reaches an empty one or the last one.
Goes through the different pages on that tab until it's done and goes to the second tab.
if it finds the spell it stays on that page and returns success and the slot (to archor the highlight to)

local function OpenSpellBookAtSpell(searchName)
    local tabNr = 1;
    local maxTabs = 2;
    local buttonNr = 1;
    local buttonPerPage = 12;
    local spellName = "";
    local firsTimeInTab = false;
    
    SpellBookSkillLineTab_OnClick(_G["SpellBookSkillLineTab"..tabNr]);
    while (tabNr <= maxTabs) do
    
        -- go to the first page 
        if (not firsTimeInTab) then
            while (SpellBookPrevPageButton:IsEnabled()) do
                SpellBookPrevPageButton_OnClick();
            end
            firsTimeInTab = true;
        end
    
        -- if target slot has a spell in it
        if _G["SpellButton"..buttonNr.."SpellName"] ~= nil and _G["SpellButton"..buttonNr.."SpellName"]:IsShown() then
            -- if the current tab still has unchecked spells
            if (buttonNr <= buttonPerPage) then
                
                spellName = _G["SpellButton"..buttonNr.."SpellName"]:GetText();
                
                if (spellName == searchName) then
                    -- Found the spell, end the world
                    return true, _G["SpellButton"..buttonNr];
                end
                
                buttonNr = buttonNr + 2;
 
                -- reached limit on uneven, go even
                -- Needed because slotNr goes L->R U->D while spells go U->D L->R
                if (buttonNr > 12 and buttonNr %2 == 1) then
                    buttonNr = 2;
                end
            end
        else -- else check for next page
            
            -- has next page, flip page
            if (SpellBookNextPageButton:IsEnabled() ) then
                
                SpellBookNextPageButton_OnClick();
                buttonNr = 1;
            else -- else next tab
                
                buttonNr = 1;
                tabNr = tabNr + 1;
                SpellBookSkillLineTab_OnClick(_G["SpellBookSkillLineTab"..tabNr]);
                firsTimeInTab = false;
            end         
        end 
    end
    
    return false;
    
end

The template for the buttons are in the XML file.
Template lines 15 to 136
Actual buttons at 242 - 300

So I'm trying to figure out if there is a way to prevent the taint, or if there's just no way to do it correctly.

[lightspark]

Not at home yet, can't run live tests, but here's my wild guess after I checked your code on GH... You have quite many Show/Hide calls, but you dun always check if character is in combat or not, you can't toggle frame visibility whilst in combat... You have to use InCombatLockdown() function to check whether it's safe to make those calls or not.

And I strongly recommend using taint log, it helps quite much...

[semlar]

I think your only chance of getting this to work correctly without breaking the spellbook is to create a secure button which executes a macro to /click the spellbook forward/back buttons to the proper page, because calling any of the spellbook functions directly is going to taint everything it touches.

That being said, why not just make it so people can drag the spell off the button you're displaying, rather than open the spellbook? It seems like an extra step, unless I'm not understanding what your addon is meant to do.

[LanceDH]

Originally Posted by lightspark Not at home yet, can't run live tests, but here's my wild guess after I checked your code on GH... You have quite many Show/Hide calls, but you dun always check if character is in combat or not, you can't toggle frame visibility whilst in combat... You have to use InCombatLockdown() function to check whether it's safe to make those calls or not. And I strongly recommend using taint log, it helps quite much...

It's not the best way to do things, but I have the 2main frames not work during combat and hide once you enter combat.
The other ones are (in theory) impossible to trigger without either of those 2 frames being shown.
I used UnitAffectingCombat("player") for it though. Achieves the same thing but might look into using the function you gave instead.
As for taint log, totally forgot that was a thing.

Originally Posted by semlar I think your only chance of getting this to work correctly without breaking the spellbook is to create a secure button which executes a macro to /click the spellbook forward/back buttons to the proper page, because calling any of the spellbook functions directly is going to taint everything it touches. That being said, why not just make it so people can drag the spell off the button you're displaying, rather than open the spellbook? It seems like an extra step, unless I'm not understanding what your addon is meant to do.

Yea as I was waiting for a reply I pretty much implemented the dragging as well.
I was already going to add that but the reason I wanted to have the SpellBook is because I also do things like open the dungeon journal, talent frame, glyph frame, .. which... I'm probably also tainting beyon belief though I don't think they call protected functions like the SpellBook does.
Another annoyance is that IsPassiveSpell("name") doesn't actually work correctly as it returns false for Tiger Strikes whish is a passive, probably because it triggers another spell with the same name that isn't a passive.
Not the biggest issue but just annoying and might require more work.

I'll check the taint log and see what comes up. worst case I'll have to live without it.


Edit:
Seems WoW just instantly taints any global variable in addons.
But as far as I understand my functions need to be global to call them in my XML code?
Or is there some other way to do this.

[SDPhantom]

Originally Posted by LanceDH But as far as I understand my functions need to be global to call them in my XML code?

Correct. Though it's one of many reasons why it's suggested to not use XML. You can do almost everything directly from Lua.

[LanceDH]

Originally Posted by SDPhantom Correct. Though it's one of many reasons why it's suggested to not use XML. You can do almost everything directly from Lua.

Well fk.
Here I've been programming addons in nothing but LUA thinking I'm not doing it the way I should.
And now I, for once, code one with XML..

[lightspark]

Originally Posted by LanceDH It's not the best way to do things, but I have the 2main frames not work during combat and hide once you enter combat. The other ones are (in theory) impossible to trigger without either of those 2 frames being shown. I used UnitAffectingCombat("player") for it though. Achieves the same thing but might look into using the function you gave instead. As for taint log, totally forgot that was a thing.

However, I managed to get one when I clicked ILW_SpellBookTab (button in spellbook) TWICE, not once, but twice, on first click I saw a warning from your addon in my chat, however, on second one I've got a taint (line #491, inside ToggleUnlockedPage function).

'ILW_UnlockContainer:Hide()'.
!BugGrabber\BugGrabber.lua:573: in function <!BugGrabber\BugGrabber.lua:573>
[C]:: in function 'Hide'
ILearnedWhat\ILearnedWhat-6.2b1.lua:491: in function <ILearnedWhat\ILearnedWhat.lua:486>
ILearnedWhat\ILearnedWhat-6.2b1.lua:504: in function <ILearnedWhat\ILearnedWhat.lua:503>

Originally Posted by LanceDH Seems WoW just instantly taints any global variable in addons. But as far as I understand my functions need to be global to call them in my XML code? Or is there some other way to do this.

Well, write it in pure Lua, safest approach possible.

One more thing. Their API is almost always safe, there are some exceptions though, but NEVER (almost never tbh, there are few exceptions too) use functions blizz implemented in Lua themselves, it almost always causes taints. Your taint you were talking about in OP was caused by usage of blizz' SpellBookSkillLineTab_* functions.

P.S. BTW, sorry, completely missed the point in my first comment, was a bit tired. Went full retard, realized it just now after a short nap,

[LanceDH]

Originally Posted by lightspark However, I managed to get one when I clicked ILW_SpellBookTab (button in spellbook) TWICE, not once, but twice, on first click I saw a warning from your addon in my chat, however, on second one I've got a taint (line #491, inside ToggleUnlockedPage function).

Had no clue :Hide() also caused errors in combat so can't say I 'protected' any of those.
I guess PLAYER_REGEN_DISABLED, where I hide everything when combat starts, happens before errors start happening.
Will have to fix that now that I know.

Originally Posted by lightspark Well, write it in pure Lua, safest approach possible. One more thing. Their API is almost always safe, there are some exceptions though, but NEVER (almost never tbh, there are few exceptions too) use functions blizz implemented in Lua themselves, it almost always causes taints. Your taint you were talking about in OP was caused by usage of blizz' SpellBookSkillLineTab_* functions. P.S. BTW, sorry, completely missed the point in my first comment, was a bit tired. Went full retard, realized it just now after a short nap,

Wouldn't it be worth it to create frames in XML and use SetScript with local functions in LUA?
Or does the XML itself cause issues?
Having done it both ways now I must say I find XML to make the base for frames a lot easier to use and keep track of things, and doesn't clutter my LUA files as much.

[semlar]

Just wanted to clear something up, since people seem a little confused as to what taint actually is.

Anything an addon does is considered "tainted". Its purpose is to prevent an addon from tricking a blizzard function into executing code that it wouldn't normally be able to do directly, or otherwise influence the result of a protected function.

So, when we call a function like SpellBookPrevPageButton_OnClick, which calls SpellBookFrame_Update, which calls SpellBook_UpdatePlayerTab, which calls SpellBookFrame_UpdateSpells, which calls SpellButton_UpdateButton, which sets "self.offSpecID", which is then read by SpellButton_OnClick when you click the spell button, it directly influences whether or not "CastSpell" is called by the interface.

Since we've "tainted" the execution path by calling a function which ultimately affected a protected function, the interface prevents the protected function from being called.

When you create a global variable, it's tainted because it wasn't created by blizzard's "trusted" code, it was created by your addon, which is untrustworthy. If something in the interface were to read that variable it would taint its execution path.

This doesn't mean that global variables are bad, just that you need to take care not to name them something that would ever be referenced by anything else.