Skip to content

Conversation

@Grocel
Copy link
Contributor

@Grocel Grocel commented Jun 5, 2025

There it is, like after 4 weeks in the making after a ton of testing. First major rewrite of the Wire Map Interface after I made the first version 10+ years ago.

About:
https://github.com/wiremod/wire/wiki/Wire-Map-Interface

Changes:

  • Improved security: Removed built-in Lua Run functions (port Lua). When used, an error message will be printed to the console and the code will not be run.
  • Improved security: Blocked AddOutput and RunPassedCode from wire input user data. It prints an error message instead.
  • Improved compatibility to GMods lua_run entity. You can copy-paste affected code key values to a lua_run entity with no further changes required.
  • Added additional support for hammer searchers (targetnames beginning with "!")
  • Added additional Lua globals for lua_run entity context.
  • Added support for Wirelink. Must be directly connected via wire tool. E2 E:Wirelink() does not work.
  • Added duplicator and GMod save support with basic anti-tampering and entity id validation.
  • Added Convars: sv_wire_mapinterface_min_trigger_time, sv_wire_mapinterface_max_sub_entities
  • Improved stability and reliability.
  • Improved performance and networking.
  • Fixed an unnamed number of bugs.
  • Backward compatible to old maps. No changes/recompile required, unless the now removed port Lua function had been used.

Demo Videos (slightly outdated):

2025-05-29_21-26-21.mp4
2025-05-29_21-23-59.mp4

Used E2 code:

@name wmi_bar_test
@inputs WMI_PROP:wirelink WMI_PROP_ENT:entity WMI_PROP_ENT_ID 
@outputs T WMI_PROP_ENT_OUT:entity
@persist
@trigger all

if (!WMI_PROP) {
    if (WMI_PROP_ENT) {
        WMI_PROP = WMI_PROP_ENT:wirelink()
    } elseif (WMI_PROP_ENT_ID) {
        WMI_PROP = entity(WMI_PROP_ENT_ID):wirelink()
    } else {
        WMI_PROP = entity():isWeldedTo():wirelink()    
    }
}
    
WMI_PROP_ENT_OUT = WMI_PROP:entity()

WMI_PROP["Moving Speed", number] = 512

event tick() {
    T = curtime() * 100
    
    for (X = 1, 16) {
        Y = (sin(X / 16 * 360 + T) + 1) / 2
        WMI_PROP["Bar " + X, number] = Y 
    }
}

Demo Map:

This pull request can be tested with this custom demo/test map, vmf included:
gm_wiremapinterface_v1-1.zip

It contains most use cases and test setup for the Wire Map Interface. You can use it for testing and learning how this system works and how it can be used.

Additional request and permission

I would also like to request to ship the map + vmf as an official part of Wiremod either in the main repository or as an additional addon from the Wire Team. I think that will help getting people more familiar with this feature.

I give full permission to use, modify and release/reupload the map, its source or any part of it when ever you like.

Notes:

  • The map might crash on creating GMod Save. It is an issue with point_worldtext, which has been fixed in the upcoming GMod update. It is not an issue of the Wire Map Interface itself.
  • The changed code and also the map do not have any dependencies beside the latest version of Wiremod. All changes work with the current main version of GMod as released on steam and on x64.

Grocel added 2 commits June 5, 2025 16:56
- Removed built-in Lua Run functions. When used an error message will be shown in game and the code will not be ran.
- Improved compatibility to GMods lua_run entity. You can copy-paste affected code key values to a lua_run entity with no further changes required.
- Improved security: Blocked AddOutput and RunPassedCode from user input. It prints an error message instead.
- Added additional support for hammer searchers (targetnames beginning with "!")
- Added additional Lua globals for lua_run entity context.
- Added duplicator and GMod save support with basic anti-tampering.
- Added Convars: sv_wire_mapinterface_min_trigger_time, sv_wire_mapinterface_max_sub_entities
- Improved stability and reliability.
- Improved performance.
- Fixed an unnamed number of bugs.
@Grocel
Copy link
Contributor Author

Grocel commented Jun 24, 2025

Current state of this PR:

grafik

@thegrb93
Copy link
Contributor

The duplicator modifications are my main concern, and then wire_map_interface.lua is very messy. Do we not have a table type that cleans itself when entities are removed?

@thegrb93
Copy link
Contributor

Are the duplicator modifications so that a duplication can wire itself to the map entities?

@Grocel
Copy link
Contributor Author

Grocel commented Jun 27, 2025

Are the duplicator modifications so that a duplication can wire itself to the map entities?

Yes. There can be two cases.

  1. The entity being connected to is spawned with the dupe, the default case (eg. props).
  2. Or the entity is not duplicate and is spawned by the map (eg. prop_dynamic, brush entities, etc.). Note: The info_wiremapinterface can give a lot of entities Wiremod ports.

The spawnid used is a hash of the map creation id, map name and the id of the info_wiremapinterface it is being assigned too. It is needed so the entities knows to which info_wiremapinterface it is supposed to reassign to when duped.

Entities being duped lose their map creation id, so I need to remember it when duped. It is hashed with the other parameters to prevent issues when duped across maps. The hash is used for data validation too. If something doesn't fit the entity will not be assigned to the info_wiremapinterface and it will not get any wire ports.

Remember: The info_wiremapinterface entity it self can never be duped. It is a map entity with no model or physics. It gives other entities wire ports in accordance to its settings.

The info_wiremapinterface_savestate is used for gmod saves only. It can not be duped with the duplicator, but it can be saved into a gmod save game, which also runs though the duplicator library. Only one instance of this entity can exist globally. It stores and restores the dupe data. The logic is separated from the info_wiremapinterface to prevent abuse and other issue.

As a map controlled entity, the info_wiremapinterface can have tons of hammer IO. Hammer IO data would be lost on duplication, storing it would allow abuse. So I separated the connection data and assigned entities to a separate entity the info_wiremapinterface_savestate.

The duplicator logic in the entities being assigned to the info_wiremapinterface is triggered by:

  • The info_wiremapinterface_savestate on save load.
  • When a preexisting entity is being attempted to wire connect to by duplicator. That's what the new Wire_ApplyDupeInfo hook is for.
  • When the entity is spawned by the duplicator.

All three cases are important to make dupe support fully reliable, because when duped the entity might not have wire ports yet. In fact the pasted entity might not even be fully initialized as wire able entity yet (just a prop) when Wiremod/dupe attempts to connect to it.

The initialization will only run once per entity is only done when it has the WireMapInterfaceEntDupeInfo entity modifier. The modifier is applied when the entity is assigned to the the info_wiremapinterface.

@Grocel
Copy link
Contributor Author

Grocel commented Jun 27, 2025

The duplicator modifications are my main concern, and then wire_map_interface.lua is very messy. Do we not have a table type that cleans itself when entities are removed?

The table is a look up table between the custom spawn id (wireEntSpawnId) and the entity with the wire ports. It is needed for the duplicator logic, so it can find entities being resigned to info_wiremapinterface. ents.GetMapCreatedEntity() would is not suitable alternative, as the entity might be not be a map generated entity, eg. form a despencer/spawner from a duplicator spawn. The spawn id embeds validation data (hash) and is unique for each relevant entity. The wireEntMapId is a failback.

A table that cleans itself? You mean setting a metatable with a __mode flag to it, right?

I am not sure if this was available in all GMod branches. So far I know the the weak table does not detect entities going invalid (NULL), as invalid entities is not a lost reference, is it? The cleanup logic only runs every 10 minutes at most. See: g_nextCleanup. In practice a lot less. A request to clean up the list will be triggered when attempts to handle an invalid entity, when an entity is removed or an entity is added.

In short the logic is debounced and does not get spammed.

@thegrb93
Copy link
Contributor

No, not __mode, but CallOnRemove or EntityRemoved hooks. It might be cleaner to not use lookup tables at all but I'll have to look closer.

Comment on lines +642 to +647
self:CallOnRemove("WireMapInterface_OnRemove", function(this)
if this._WMI_RemoveOverrides then
this:_WMI_RemoveOverrides()
end
end)
end
Copy link
Contributor Author

@Grocel Grocel Jun 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not __mode, but CallOnRemove or EntityRemoved hooks. It might be cleaner to not use lookup tables at all but I'll have to look closer.

Yeah, CallOnRemove is already a thing. That clean up thing is more of a backup thing to make sure there would absolutely no way it could leak memory. I don't think it is not possible to avoid the lookup table in this case.

local spawnIdDuped = WIREENT._WMI_GetSpawnIdDuped(self, interfaceEnt)
local mapId = WIREENT._WMI_MapCreationIdDuped(self)

WireLib.UnregisterWireMapInterfaceSpawnId(spawnId, spawnIdDuped, mapId, self)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's where it is called to unregister itself from the lookup table. WIREENT:_WMI_RemoveOverrides is called if the entity is removed or is being unassigned from it's info_wiremapinterface.

@thegrb93
Copy link
Contributor

thegrb93 commented Jul 2, 2025

I think the dupe changes look ok, but I'll look closer soon.

@Grocel Grocel requested a review from thegrb93 July 3, 2025 09:34
@thegrb93
Copy link
Contributor

thegrb93 commented Jul 3, 2025

Will look at cl_wire_map_interface soon. There's some performance concerns there.

@Anticept
Copy link
Member

Anticept commented Jul 8, 2025

Hello all!

There's a lot back and forth with this PR, and I'm stepping in and putting the discussion on hold here on github.

This is not a PR block, nor is it a PR push, rather its mediation time.

I'll be working on trying to get a good roundtable going in discord so that people dont need to keep repeating themselves, and brainstorm if there are simple solutions to the concerns raised.

This is in an effort to keep peace, and also give the PR a fair shake for discussion. Even if one still has access, please do not post further comments until unlocked.

@wiremod wiremod locked and limited conversation to collaborators Jul 8, 2025
@wiremod wiremod unlocked this conversation Jul 12, 2025
@Anticept
Copy link
Member

Anticept commented Jul 12, 2025

This PR has been discussed at a roundtable, and is preemptively approved for merger.

The only reason this PR can now be blocked is for any serious unsolvable technical/security issue.

local inputNameLower = string.lower(inputName)

local params = output.param or ""
if params ~= "" then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with params, doesn't seem to get used elsewhere? Could do if (output.param or "") ~= "" then if Lua supports that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, it seems a little ugly to me.

Copy link
Contributor

@DarthTealc DarthTealc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Can't spot any obvious issues with the code that haven't already been fixed, and my previous test of the test map had no lua errors or functionality issues. Can't really input on wire-oriented exploits or map implementation as I'm not a mapper, but seems to have checks to abort in cases where maps have implemented things in a dangerous way so it shouldn't be possible to run arbitrary lua.

@Grocel
Copy link
Contributor Author

Grocel commented Jul 14, 2025

Can't really input on wire-oriented exploits or map implementation as I'm not a mapper, but seems to have checks to abort in cases where maps have implemented things in a dangerous way so it shouldn't be possible to run arbitrary lua.

You are right. The test map has a penetration test setup.
pentest

If you attempt to input anything to it, it will abort and print a warning (serverside).
pentest_result

Note: The protection does not affect the input name / description. It's just that verbose to make the tester know what to expect.

@Grocel Grocel requested a review from DarthTealc July 14, 2025 13:04
BlackIceKB
BlackIceKB approved these changes Jul 16, 2025
@Anticept Anticept merged commit 26abfc8 into wiremod:master Jul 22, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants