andrei-drexler / ironwail

High-performance QuakeSpasm fork
GNU General Public License v2.0
546 stars 50 forks source link

SZ_GetSpace: overflow in AD1.7 ad_sepulcher and sv_protocoll 999 #93

Closed j4reporting closed 2 years ago

j4reporting commented 2 years ago

map ad_sepulcher won't start in AD1.7/1.7p1 with default sv_protocoll 999

]game
"game" is "id1;ad_1.7"
]sv_protocol
"sv_protocol" is "999"
]map ad_sepulcher
Host_Error: SZ_GetSpace: overflow without allowoverflow set

map loads with sv_protocoll 666

]sv_protocol 666
]map ad_sepulcher

FITZQUAKE 0.85 SERVER (4539 CRC)

The Forgotten Sepulcher
Using protocol 666
Slipgate_Touris entered the game

same issue does exist in vkquake
https://github.com/Novum/vkQuake/issues/484

mhQuake commented 2 years ago

999 can bloat the protocol data, particularly from sending positions and angles using full float representation. The AD particle system is going to be particularly bad for this as it sends all of it's individual particles as sprite entities, which need their positions updated each frame. A few thousand of those and it's going to overflow it's buffers.

999 wasn't designed to be particularly clever when it comes to data compression, so that's an unfortunate consequence.

neogeographica commented 2 years ago

FWIW I also got "SZ_GetSpace: overflow without allow overflow set" on this map: https://www.celephais.net/board/view_thread.php?id=62188

Just a vanilla id1 map there, altho a large-ish one.

(And yeah it does load with sv_protocol 666.)

h4724 commented 2 years ago

I contacted andrei a few days ago about the same issue (on Telefragged from RRP) and he gave the following response:

Hey! Yeah, it's a limitation of the networking code, it should be the same in QS, except I changed the default sv_protocol value to 999, which sends more data to the client and is more likely to trigger this on maps with tons of entities. I'll either change it back to 666 or try to isolate the QSS code that deals with this, but that's tricky since there are a lot of networking changes in QSS and they kind of depend on one another.

andrei-drexler commented 2 years ago

Yeah, same as QS with sv_protocol 999, or QSS with sv_protocol base999 (without the FTE extensions). As mh said, space efficiency wasn't the primary factor when 999 was designed. The issue here is that there's a fixed-size (64K) buffer used to store the initial map state and send it to clients when they connect (the so-called "signon buffer"). With thousands of entities and an inefficient encoding 64K isn't enough. I can't make that buffer any larger... but I can make it so multiple buffers are used when 1 isn't enough. This doesn't actually break the protocol - an unpatched QS (that normally wouldn't be able to load the map using 999) can actually connect to an Ironwail server that sends multiple buffers. Demos recorded in Ironwail still play in QS. The only thing that breaks is demo recording when an unpatched client is connected to a patched server, since the code assumes there's a single signon buffer, so it only stores the last one in the demo. Anyway, the fix should be ready pretty soon.

andrei-drexler commented 2 years ago

Fixed in f72501719222a021c1df28d203fc992b736c97f9.