ValveSoftware / halflife

Half-Life 1 engine based games
Other
3.68k stars 624 forks source link

[Engine] Rework resource list transmission to prevent overflows #2534

Open SamVanheer opened 5 years ago

SamVanheer commented 5 years ago

The engine is currently sending the list of resources in one go, using a single buffer that cannot overflow. When it does overflow, this fatal error occurs: afbeelding

This places an upper limit on the number of resources that is sometimes lower than the maximum number of resources of a particular type (model, sound, generic, decals) depending on filename length.

Reworking this removes the networking related limit, allowing the other limits to be increased afterwards.

Quake 1 sends this data here: https://github.com/id-Software/Quake/blob/bf4ac424ce754894ac8f1dae6a3981954bc9852d/WinQuake/sv_main.c#L181-L233

GoldSource has it reworked a bit and sends it in SV_SendRes_f.

Quake 2 generalizes the resource lists as a single list of config strings and sends as much as possible per packet, until everything is sent: https://github.com/id-Software/Quake-2/blob/372afde46e7defc9dd2d719a1732b8ace1fa096e/server/sv_user.c#L119-L172

Note that changing this will require a breaking change in the network protocol, making older engine builds incompatible.

Also note that Quake 2's code isn't secure against attacks involving spammed commands, and no validation is performed on the command arguments which makes it dangerous. I would advise developing a more secure method of performing list transfers.

RauliTop commented 5 years ago

Note that changing this will require a breaking change in the network protocol, making older engine builds incompatible.

Can you explain more about this?

SamVanheer commented 5 years ago

Note that changing this will require a breaking change in the network protocol, making older engine builds incompatible.

Can you explain more about this?

In order for this to work the client has to inform the server when it has to send more data. The client also has to understand the new way that lists are transferred. Old clients won't know what to do.

RauliTop commented 5 years ago

So, this isn't a good addition because of compatibility breaking

metita commented 5 years ago

@RauliTop I am sure this would affect No-Steam clients, as someone said in the past, this should not be a factor when considering improving the game.

SamVanheer commented 5 years ago

Yeah old clients are exclusively no-Steam at this point. Steam clients should always be updated to the latest version.

RauliTop commented 5 years ago

Yeah, of course.

So, if you really want to make this change, it can only be considered in a stable release. It can't be added in beta as you said: "Old clients won't know what to do"

I personally think that do a change like this without beta release can cause multiple problems. And we really saw so many on last betas, so imagine in a new stable release.

SamVanheer commented 5 years ago

It can be done in a beta, but both the client and server would need to be using the same beta (which is generally recommended anyway).

RauliTop commented 5 years ago

It can be done in a beta, but both the client and server would need to be using the same beta (which is generally recommended anyway).

And you make a beta server to only beta clients. For sure it's a good option (I'm joking)

Changes shouldn't break compability (like we saw in the past)

SamVanheer commented 5 years ago

This change doesn't break compatibility with the intended client. It just breaks compatibility with illegal no-Steam clients.

RauliTop commented 5 years ago

It just breaks compatibility with non-updated beta legal steam clients too.

Think what you want.

SamVanheer commented 5 years ago

...No?

There is only one beta at any given time, so once this change gets made it'll be in beta, it gets tested, and then gets released. All future betas will have the new code, and all public versions will have it too. Whether you're running the public or the beta version, you'll have an engine that's compatible with this.

And you should only be running the beta to test the changes. If you're running the beta constantly while playing on public version servers you're just asking for problems.

sisi399 commented 5 years ago

It will be the final nail in the coffin If any update breaks compatibility with non-steam builds like 4156, 4554.

Not really going to explain why but i am pretty sure anyone that actually plays the game or owns a public server will understand.

SamVanheer commented 5 years ago

Not to promote any illegal activities or anything, but no-Steam builds can just be re-cracked from the current Steam build if you want compatibility there. It's up to you to keep up with official development, not the other way around.