Improve and update networking for better performance and security

[CaptainPRICE]

Lots of code is still using old/deprecated library/functions such as usermessages and Entity.GetNetworked*/Entity.SetNetworked* (this alone downgrades performance to some degree in multiplayer). And then there are security issues, like receiving huge data on the server that normally can not even possibly happen and thus lagging the server... So... Here is the list of affected files (sorted by usage):

User Message (umsg)

Solution: Use net library (it has been introduced in GMod 13). See example usage, and more example.

lua/entities/gmod_wire_egp/lib/egplib/usefulfunctions.lua
lua/entities/gmod_wire_egp_hud/huddraw.lua
lua/entities/gmod_wire_egp_hud/init.lua
lua/entities/gmod_wire_expression2/core/custom/cl_remoteupload.lua
lua/entities/gmod_wire_expression2/core/custom/remoteupload.lua
lua/entities/gmod_wire_expression2/core/egpfunctions.lua
lua/entities/gmod_wire_eyepod.lua
lua/entities/gmod_wire_gpu/cl_init.lua
lua/entities/gmod_wire_gpu/init.lua
lua/entities/gmod_wire_hudindicator/cl_init.lua
lua/entities/gmod_wire_hudindicator/init.lua
lua/entities/gmod_wire_pod.lua
lua/entities/gmod_wire_spu/cl_init.lua
lua/entities/gmod_wire_spu/init.lua
lua/entities/info_wiremapinterface/entitycontrol.lua
lua/wire/beam_netvars.lua
lua/wire/client/cl_wire_map_interface.lua
lua/wire/cpulib.lua
lua/wire/flir.lua
lua/wire/gpulib.lua
lua/wire/stools/clutch.lua
lua/wire/stools/hdd.lua

Entity.GetNetworked*

Solution: Use appropriate Entity.GetNW* function. For example, instead of self:GetNetworkedFloat("Length", 50), do this: self:GetNWFloat("Length", 50).

lua/entities/gmod_wire_hologram.lua
lua/entities/gmod_wire_oscilloscope.lua
lua/entities/gmod_wire_speedometer.lua
lua/entities/gmod_wire_thruster.lua
lua/entities/gmod_wire_vectorthruster.lua
lua/entities/gmod_wire_waypoint.lua
lua/entities/gmod_wire_hudindicator/cl_init.lua
lua/wire/client/cl_wirelib.lua
lua/wire/stools/clutch.lua
lua/wire/stools/emarker.lua
lua/wire/stools/hudindicator.lua

Entity.SetNetworked*

Solution: Use appropriate Entity.SetNW* function. This is similar to Entity.GetNW* (see above), except that it is Set instead of Get.

lua/entities/gmod_wire_hologrid.lua
lua/entities/gmod_wire_oscilloscope.lua
lua/entities/gmod_wire_ranger.lua
lua/entities/gmod_wire_speedometer.lua
lua/entities/gmod_wire_thruster.lua
lua/entities/gmod_wire_vectorthruster.lua
lua/entities/gmod_wire_waypoint.lua
lua/entities/gmod_wire_wheel.lua
lua/entities/gmod_wire_hudindicator/init.lua
lua/entities/gmod_wire_expression2/core/hologram.lua
lua/wire/stools/clutch.lua
lua/wire/stools/emarker.lua
lua/wire/stools/hudindicator.lua

Possible Security Holes

// Scan later; Busy now.

Notes

• Make sure all network messages are first precached/registered on the server via util.AddNetworkString(string) function (I recommend using Initialize hook for this).
• Name of net message (for net receiver) is always in lower-case (internally).
• Each net message has a length limit of 64KB.
• Only one receiver can be used to receive the net message.
• Once net message arrives, you must read information in the exact same order as you wrote/sent it.
• Do NOT overuse it or waste bandwidth, for example if you know you're sending a number that is always going to be within range 0-255, do net.WriteUInt(n, 8). 8 is the number of bits (in this case it is one byte, unsigned). For more information, read wiki. I have included a table of non-negative powers of 2, see below.
• Remember: Server should never trust the client; make sure to always validate length (1st argument) of net message in server net receiver, also (second argument is the player who sent net message), check if player can actually send such net message in the given situation, etc.

Non-negative powers of 2

n	2n
0	1
1	2
2	4
3	8
4	16
5	32
6	64
7	128
8	256
9	512
10	1,024
11	2,048
12	4,096
13	8,192
14	16,384
15	32,768
16	65,536
17	131,072
18	262,144
19	524,288
20	1,048,576
21	2,097,152
22	4,194,304
23	8,388,608
24	16,777,216
25	33,554,432
26	67,108,864
27	134,217,728
28	268,435,456
29	536,870,912
30	1,073,741,824
31	2,147,483,648
32	4,294,967,296

[Divran]

Lots of code is still using old/deprecated library/functions such as usermessages and Entity.GetNetworked/Entity.SetNetworked (this alone downgrades performance to some degree in multiplayer). [...] Entity.GetNetworked Solution: Use appropriate Entity.GetNW\ function. For example, instead of self:GetNetworkedFloat("Length", 50), do this: self:GetNWFloat("Length", 50).

Get/SetNW are aliases of Get/SetNetworked. They are the exact same function. Doing this is would be a coding style change or whatever, not a performance change. (Or if Get/SetNetworked* are finally removed, like the wiki has been saying for ages, then we would need to fix it obviously)

User Message (umsg) Solution: Use net library (it has been introduced in GMod 13). See example usage, and more example.

This is only necessary in cases where more than 256 bytes of data is sent. Unless someone has proof that net messages are faster/puts less strain on the server (significantly enough to be worth it) than usermessages, I don't see the point.

Notes [...]

Why are you stating the obvious

Non-negative powers of 2

Why are you stating the obvious

[CaptainPRICE]

I would like to shoutout @Mista-Tea, hello there, here we are talking about possible networking improvements and issues of Wire :) As far as I understand it, his NetWrapper library could help make Wire use less network bandwidth by using Net Vars instead of Entity.SetNW*/Entity.GetNW*, etc. Side note: Git submodule could be used to add netwrapper to wire. And it is licensed under MIT license, I am not an expert at code licensing, but I think MIT is fine. Say your thoughts... @bigdogmat @TomyLobo

[CaptainPRICE]

GetNW* are aliases of GetNetworked*

Yeah. But, once Entity.GetNetworked* goes away, are they still going to be aliases? What is wire going to do, redefine them? (Don't be ridiculous though, I do have such library but that's just for keeping old GLua addons/scripts compatible with GMod 13.)

Unless someone has proof that net messages are faster/puts less strain on the server (significantly enough to be worth it)

Ok, that at least shouldn't be hard to proof, since I have already experienced it.. later, I will show some Lua.

I don't see the point.

You don't see it? Well, at some point they will be removed from GLua and thus Wire networking will completely suck it (and error all over the place).

[bigdogmat]

As far as I understand it, his NetWrapper library could help make Wire use less network bandwidth by using Net Vars instead of Entity.SetNW/Entity.GetNW, etc.

How? The point of his library from what I can see is so you can network any value with a key, also if anything it'd use more bandwidth because of the type guessing with net.WriteType in his code.

Yeah. But, once Entity.GetNetworked* goes away, are they still going to be aliases? What is wire going to do, redefine them? (Don't be ridiculous though, I do have such library but that's only for keeping old Lua from breaking away.)

I know this isn't really a point to argue, but alias are never removed, that's the way it has almost always been. The only time we get rid of them was in the last major update, that being Gmod 13. We aren't getting another one of those, and so I doubt those aliases are going anywhere.

Of course I believe they should still be changed, although it should be considered clean up and treated as such.

Ok, that at least shouldn't be hard to proof, since I have already experienced it.. later, I will show some Lua.

Source still uses user messages, only downside to them is the limit. Unless using net messages can improve it, I don't think it really matters.

Of course like I said above, they should still be changed at some point, but lets not make a huge deal about it.

[Divran]

@CaptainPRICE Good job not even reading my post.

[CaptainPRICE]

Good job not even reading my post.

I don't know what you mean. But in your edit, you say:

Doing this is would be a coding style change or whatever, not a performance change.

So, using net library instead of umsg is a coding style change to you? Interesting.. And right below that, you say:

Unless someone has proof that net messages are faster/puts less strain on the server (significantly enough to be worth it) than usermessages

Oh Divran, you're the one that didn't read the post.. I'm just gonna proof it, that umsg are queued and netmsg are not (wait for it; read my posts)..

Source still uses user messages, only downside to them is the limit. Unless using net messages can improve it, I don't think it really matters.

Oh, but it does really matter because it makes servers lag. That not enough?

[bigdogmat]

Oh, but it does really matter because it makes servers lag. That not enough?

Never have I heard that they lagged a server, nor have they ever in my experience, please show proof.

The queue is known information, although all of the queue is sent the next frame. I don't see this mattering in most situations.

[thegrb93]

There's nothing wrong with using umsg for small messages. If you want to findreplaceall-in-files all the aliases then go ahead, but nobody really cares about aliases.

[TomyLobo]

Thanks, @CaptainPRICE for posting an exhaustive list without clearing it with the wire team first - yours truly, Evil Hacker.

[Divran]

EDIT: You are all right, this was too rude, I agree. Erased.

EDIT: okay now that the initial frustration has passed, let me just say that what you did that was retarded was: you cut out small sections from my post and only payed attention to the parts that said what you wanted to hear. Every comment you have about my post was already answered in that post.

[oldmud0]

Wow, rudest thing I've heard from Divran in years :astonished:

[Divran]

all outta fucks to give

[Nebual]

@Divran yikes, keep your venting to Discord!

@CaptainPRICE I believe net messages do have slightly more overhead than umsgs, though they are of course superior for large data transfers like uploading E2's. I'd be interested in any benchmarks you make, though I think due to the differences in queueing/overhead, there may not be a clear winner for all circumstances.

[AbigailBuccaneer]

@Divran abusive language isn't okay, especially not from a well-respected developer. We can do better than this.

@CaptainPRICE we'd welcome a pull request to replace {Get,Set}Networked with {Get,Set}NW, though as mentioned this is purely an alias and so the change would be for deprecation reasons rather than performance reasons. This was already partially done in #1249. As for replacing usermessages with net messages, #946 already exists.

Probably the biggest source of legacy code around networking is in the beam vars library, which is used for networking visual wire paths. #969 is the issue for replacing that with a nicer system.

[Divran]

You are right, I am being rude. I will stop doing this to CaptainPRICE from now on. No more rudeness from me again.

[AbigailBuccaneer]

As there's already multiple PRs which cover bits of this, I'm going to go ahead and close this with a pull request about removing usage of the mentioned deprecated functions.

[CaptainPRICE]

For some reason... This was closed as "job was done" from my understanding. Unfortunately. @AbigailBuccaneer had closed this.

Well. Upgrading from deprecated functions [Get/Set]Networked* to [Get/Set]NW* was just a single (first) part of this, see title too. And the security (second) part of this proposal was never addressed, where we shall have focus on security holes, and possible bad codes (that could be abused by clientside Lua which could potentially cause server lag and such). But, "and security" point was ignored for whatever reasons (perhaps it was false thought as "non-existing"). And now since this was closed, nobody ever paid back any attention into it, leaving "security" part ever unfinished...

Resurrection

So, let me point out a possible security hole (just one) as an introduction, to hopefully revive awareness of the 'security' as an issue.. In a Constant Value entity, a client could use net library to send 15k+ long string to the server, no anti-spam system inbetween: (clientside sends) https://github.com/wiremod/wire/blob/master/lua/wire/stools/value.lua#L152-L154

(serverside receives) https://github.com/wiremod/wire/blob/master/lua/wire/stools/value.lua#L29-L39

[Divran]

Regarding the constant value example above: As everyone can see, the serverside receive hook actually does not do anything with the data inside the receive hook (it's only used later). This means if a user is able to lag a server using this net message, all the performance hit will be coming from the net message itself, not from the lua code we have written in this file.