ESOUI - More 'insecure code' errors, now in my addons!

ESOUI (https://www.esoui.com/forums/index.php)

- Lua/XML Help (https://www.esoui.com/forums/forumdisplay.php?f=175)

- - More 'insecure code' errors, now in my addons! (https://www.esoui.com/forums/showthread.php?t=6803)

Phinix

02/09/17 05:54 PM

More 'insecure code' errors, now in my addons!

So ZOS changed something. I can't find any reference to what it was, but it is causing many addons to generate random 'insecure code' LUA errors, with no mention of the addon or any hooks it uses in the traceback, making it something of a challenge to track down.

For example Master Recipe List. It is causing the following errors when changing categories on the Crown Store purchase tab of the Housing Editor:

Code:

EsoUI/Ingame/HousingEditor/FurnitureClasses_Shared.lua:450: attempt to access a private function 'GetMarketProductPricingByPresentation' from insecure code

stack traceback:

        EsoUI/Ingame/HousingEditor/FurnitureClasses_Shared.lua:450: in function 'ZO_HousingMarketProduct:GetMarketProductPricingByPresentation'

        EsoUI/Ingame/HousingEditor/Keyboard/HousingFurnitureProducts_Keyboard.lua:80: in function 'ZO_HousingFurnitureProducts_Keyboard:SetupMarketProductFurnitureRow'

        EsoUI/Ingame/HousingEditor/Keyboard/HousingFurnitureProducts_Keyboard.lua:59: in function 'setupCallback'

        EsoUI/Libraries/ZO_Templates/ScrollTemplates.lua:1462: in function 'ZO_ScrollList_UpdateScroll'

        EsoUI/Libraries/ZO_Templates/ScrollTemplates.lua:1222: in function 'ZO_ScrollList_Commit'

        EsoUI/Libraries/ZO_SortFilterList/ZO_SortFilterList.lua:155: in function 'ZO_SortFilterList:CommitScrollList'

        EsoUI/Ingame/HousingEditor/Keyboard/FurnitureClasses_Keyboard.lua:402: in function 'ZO_HousingFurnitureList:ContentsCommitScrollList'

        EsoUI/Ingame/HousingEditor/Keyboard/FurnitureClasses_Keyboard.lua:320: in function 'CommitScrollList'

        EsoUI/Libraries/ZO_SortFilterList/ZO_SortFilterList.lua:138: in function 'ZO_SortFilterList:RefreshData'

        EsoUI/Ingame/HousingEditor/Keyboard/FurnitureClasses_Keyboard.lua:372: in function 'ZO_HousingFurnitureList:OnCategorySelected'

If you look at EsoUI/Ingame/HousingEditor/FurnitureClasses_Shared.lua:450:

Code:

function ZO_HousingMarketProduct:GetMarketProductPricingByPresentation()

    return GetMarketProductPricingByPresentation(self.marketProductId, self.presentationIndex)

end

Master Recipe List doesn't touch this function, or either of it's values. Further it does not hook any of the functions listed in the traceback.

So, how are we supposed to troubleshoot a thing like this?

Rhyono

02/09/17 05:56 PM

The E issue can also apply to other things (like confirmation windows, not just inventory) and does not trigger an error message, which makes it extra special. They've definitely broken something and it seems to be affecting seemingly unrelated facets of the game.

Phinix

02/09/17 06:05 PM

Nothing to do but start commenting out blocks of code and seeing which is causing it. Starting with all hooks...

Randactyl

02/09/17 06:21 PM

here's the diffs from 2.6.4 to 2.7.5, beginning with the inventory files: https://github.com/esoui/esoui/commi...de21c06da19cf6

I'm looking through them, but more eyes would be helpful.

Phinix

02/09/17 06:26 PM

OK, so I managed to track down this specific case, and (HOPEFULLY) it will help others to eliminate this from their addons as well.

It appears to be a change in the way ZOS is handling hooks to certain global functions. For example in Master Recipe List, I narrowed the above errors down to two specific hooks. By changing these to use ZO_PreHook() I was able to keep the functionality of the hook and avoid these insecure code errors. So:

Code:

        local ESOMRL_ZO_TreeHeader_OnMouseUp = ZO_TreeHeader_OnMouseUp

        ZO_TreeHeader_OnMouseUp = function(self, upInside)

                if GetCraftingInteractionType() == CRAFTING_TYPE_PROVISIONING then

                        local treeNode = self.node.tree

                        if treeNode.exclusive or treeNode.open then

                                treeNode.exclusive = false

                                treeNode.open = false

                        end

                        stationNav = 1

                        stationItem = 1

                        ClearStationSelection()

                end

                ESOMRL_ZO_TreeHeader_OnMouseUp(self, upInside)

        end

Becomes:

Code:

        ZO_PreHook("ZO_TreeHeader_OnMouseUp",

        function(self, upInside)

                if GetCraftingInteractionType() == CRAFTING_TYPE_PROVISIONING then

                        local treeNode = self.node.tree

                        if treeNode.exclusive or treeNode.open then

                                treeNode.exclusive = false

                                treeNode.open = false

                        end

                        stationNav = 1

                        stationItem = 1

                        ClearStationSelection()

                end

        end)

...and:

Code:

        local ESOMRL_ZO_TreeSetNodeOpen = ZO_Tree.SetNodeOpen

        ZO_Tree.SetNodeOpen = function(self, treeNode, open, userRequested)

                if GetCraftingInteractionType() == CRAFTING_TYPE_PROVISIONING then

                        if stationNode == 0 then open = false stationNode = 1 end

                        if stationItem == 0 or stationNav == 0 then open = false end

                end

                ESOMRL_ZO_TreeSetNodeOpen(self, treeNode, open, userRequested)

        end

Becomes:

Code:

        ZO_PreHook(ZO_Tree, "SetNodeOpen",

        function(self, treeNode, open, userRequested)

                if GetCraftingInteractionType() == CRAFTING_TYPE_PROVISIONING then

                        if stationNode == 0 then open = false stationNode = 1 end

                        if stationItem == 0 or stationNav == 0 then open = false end

                end

        end)

YMMV but at least it a clue. ;)

EDIT:

So the obvious question for Master Chip (or anyone more knowledgeable) would be, WHY? :)

If I had to guess it would be the way internal references are cleared out from the active context, and it not happening with custom hooks because of some "waiting for return" type state, and some other, newer function that requires code states be "unburdened" by such waiting states in order to be considered "secure."

/stabdark

Randactyl

02/09/17 06:45 PM

Addon land is all considered insecure.

When you do:

Code:

local func = ZO_Func

ZO_Func becomes insecure. And then any functions called from within ZO_Func (and any called within those) will be considered insecure for that call.

ZO_PreHook has always been the correct way because it doesn't move ZO_Func into addon land.

Phinix

02/09/17 06:56 PM

So what you're saying is, we walk in insecure lands. :p

I wonder if this might be related to the inventory issues...

Randactyl

02/09/17 07:09 PM

That's why I identified the object pool corruption in the other thread.

edit: at least for me

Phinix

02/09/17 07:29 PM

So, what would be the accepted method for modifying function result data AFTER the original function has run?

Copying the entire original function into the prehook then modifying it and returning true seems bad. It would seemingly be mutually exclusive to other addons hooking the same function, no?

Randactyl

02/09/17 07:51 PM

There is no safe post hook.

See: http://www.esoui.com/forums/showthre...light=posthook

Randactyl

02/09/17 08:37 PM

@Diesosoon

That error looks completely unrelated to this discussion.

Please start a thread in addon help with that error and your addon list.

Phinix

02/09/17 09:28 PM

So In fixing this I am going back through my code and replacing hooks that don't need post processing with ZO_PreHook. I am wondering though, would it be "safe" to post-hook tooltips? It seems like everyone does, and it would need some fancy workaround to avoid the added lines going to the top of the tooltip instead of the bottom if using prehooks.

Here is what I am doing now:

Code:

        local ESOMRLSetBagItemTooltip = ItemTooltip.SetBagItem

        ItemTooltip.SetBagItem = function(control, bagId, slotIndex)

                local itemLink = GetItemLink(bagId, slotIndex)

                local itemId = GetItemIdFromLink(itemLink)

                ESOMRLSetBagItemTooltip(control, bagId, slotIndex)

                if ESOMRL.RecipeMaterials[itemId] ~= nil then

                        AddRecipeTooltipLine(control, itemId, itemLink, 1)

                elseif ESOMRL.IngredientMaterials[itemId] ~= nil then

                        AddIngredientTooltipLine(control, itemId)

                elseif ESOMRL.ResultMaterials[itemId] ~= nil then

                        AddRecipeTooltipLine(control, itemId, itemLink, 0)

                end

        end

Any reason this should cause problems?

Diesosoon

02/09/17 10:36 PM

Quote:

Originally Posted by Randactyl (Post 29747)

@Diesosoon

That error looks completely unrelated to this discussion.

Please start a thread in addon help with that error and your addon list.

Apologies.

Phinix

02/10/17 07:50 AM

EDIT: Forgive this last update was premature.

Phinix

02/10/17 11:48 AM

OK, so I have narrowed this inventory drag insecure LUA error down to a combination of TWO specific addons.

Map Coordinates by Garkin
Votan's Mini Map

With no other addons running, I can reliably generate the error with these two loaded together.

votan

02/10/17 11:54 AM

Quote:

Originally Posted by Phinix (Post 29777)

I use both, too. What do you do to reproduce?

Ayantir

02/10/17 12:12 PM

I don't see why Map Coordinates (and in a larger scope map addons) would trigger an insecure code. There's nothing in its code touching it. Used it daily for months and 0 issue.

Phinix

02/10/17 12:42 PM

Quote:

Originally Posted by votan (Post 29778)

I use both, too. What do you do to reproduce?

Just enable both together with no other addons loaded. Then open your inventory, click and drag any item (doesn't matter what type, if it is stolen, etc.), and you should get the error.

Here is a video showing the process:
How to duplicate the error

If for whatever reason you can't duplicate this on the first try (as I said the interaction seems random), just enable any other addon, reload, then disable that addon, and reload again, and repeat the above steps with the inventory.

Just in case something is not up to date on my end here are the versions of the two addons I have:
TestAddOns.rar

EDIT:
You may or may not have to open the map first, mouse over it to show some coordinate changes from Map Coordinates,
THEN open the inventory and drag an item. (Not confirmed.)

votan

02/10/17 12:48 PM

Quote:

Originally Posted by Phinix (Post 29780)

Just enable both together with no other addons loaded. Then open your inventory, click and drag any item (doesn't matter what type, if it is stolen, etc.), and you should get the error.

Here is a video showing the process:
How to duplicate the error

If for whatever reason you can't duplicate this on the first try (as I said the interaction seems random), just enable any other addon, reload, then disable that addon, and reload again, and repeat the above steps with the inventory.

Just in case something is not up to date on my end here are the versions of the two addons I have:
TestAddOns.rar

EDIT:
You may or may not have to open the map first, mouse over it to show some coordinate changes from Map Coordinates, THEN open the inventory and drag an item. (Not confirmed.)

Please try this:
Locate this line: WORLD_MAP_FRAGMENT.duration = 100
Set it to zero: WORLD_MAP_FRAGMENT.duration = 0

I think, it is the pin mouse-over animation of the hooked pins (over the scale pins feature request)

Phinix

02/10/17 12:59 PM

Quote:

Originally Posted by votan (Post 29782)

Please try this:
Locate this line: WORLD_MAP_FRAGMENT.duration = 100
Set it to zero: WORLD_MAP_FRAGMENT.duration = 0

Making this change I so far am not able to reproduce the error. If I change it back to 100 and /reloadui the error occurs immediately, same steps.

UPDATE:
I have confirmed that to generate this error you must follow a specific sequence:

1) Reload the game with Map Coordinates and Minimap loaded (and the above setting at 100).
2) Hit your keybind to open the map. The map MUST be the FIRST scene that you activate after reloading.
3) Click the bag icon at the top bar after the map is shown to move to the inventory.
4) Drag an item.

votan

02/10/17 01:50 PM

Quote:

Originally Posted by Phinix (Post 29783)

I have confirmed that to generate this error you must follow a specific sequence:

1) Reload the game with Map Coordinates and Minimap loaded (and the above setting at 100).
2) Hit your keybind to open the map. The map MUST be the FIRST scene that you activate after reloading.
3) Click the bag icon at the top bar after the map is shown to move to the inventory.
4) Drag an item.

Ok, next test:
Locate WORLD_MAP_SCENE:AddFragment(ZO_FadeSceneFragment:New(MAP_COORDINATES.control))
in Garkin's addon.
Change to:
WORLD_MAP_SCENE:AddFragment(ZO_HUDFadeSceneFragment:New(MAP_COORDINATES.control))

Phinix

02/10/17 02:08 PM

Quote:

Originally Posted by votan (Post 29784)

This also seems to solve the errors. I set your previous test back to 100, made the above change, and after reloading there was no error. Set the above back to the original, reloaded, followed the same sequence, errors.

I also tried with both edits set, and going straight to the inventory instead of going to the map first, and either way it seems either or both of these changes together avoid the error in half a dozen tests so far.

votan

02/10/17 02:13 PM

Quote:

Originally Posted by Phinix (Post 29785)

Yes, the problem is that Garkin's original fragment has a 200ms fade, which is borderline (if it is the last fragment, it will be the cause of the error)
One could change the fade duration of an other build-in fragment to be greather than 200ms to solve the problem, too.

votan

02/10/17 02:21 PM

@Chip: Any chance, that build-in code is always the "last" one in the scene change transition? (Even if a custom fragment has a fade duration of 5000?)

Phinix

02/10/17 11:59 PM

Nice work tracking this down, Votan. I do have just a couple questions.

First I suppose would be, what changed? Obviously this fade time you mention wasn't a problem before the update. Also, and this could just be my own lack of understanding how scene fragments work clearly, why should the time it takes a fragment to fade in/out (presumably between scene transitions?) move it from secure to insecure code?

As I understood it, scenes were just like UI container tables with fragments being the pool of sub-objects that get built on the fly when the scene fires. Unless a fragment is firing off some code in a local context that then makes reference to a protected function, I don't really understand how the whole scene could be poisoned by the time it takes a fragment to appear.

Rhyono

02/11/17 12:33 AM

It sounds like the game needs to end on a built-in scene or it panics. So perhaps it's not so much that the time it takes changes its security, but perhaps the last one to fade (typically a built-in, which is secure) is the one it checks for security reasons? That looks like what Votan was saying but why it'd be like that is puzzling.

votan

02/11/17 01:56 AM

Quote:

Originally Posted by Phinix (Post 29792)

Good question. Maybe the hijack of the world map fragment :o

Look at these posts. Especially the last one. And look at the date.
http://www.esoui.com/forums/showpost...5&postcount=50
http://www.esoui.com/forums/showpost...31&postcount=3
http://www.esoui.com/forums/showpost...74&postcount=9

All times are GMT -6. The time now is 04:18 AM.