clientside demo recorder does not filter N_SERVINFO messages

[andrius4669]

One of my friends, MTH, observed bug which happened only in spaghettimod servers (his particular demo was captured in DEMOPHOBIA). Demos recorded in these servers switched local client cn to cn of one recording demo, therefore breaking spectating of one who recorded demo. I was able to confirm broken demo by playing it back in unmodified client. I tracked this down to N_SERVINFO being present in demo file. Therefore, I looked at code and it was pretty obvious that it does not filter out N_SERVINFO message. While N_SERVINFO isn't sent more than once in normal servers, pisto apparently used it to allow multiple authconnect offers from server. Please filter N_SERVINFO message from being included to demos, add DEMORECORDER_SKIP_PACKET_NC to N_SERVINFO handler or something.

[tpoechtrager]

It would be great if you could share a "broken" demo. Thanks.

[tpoechtrager]

Does this (... totally untested ...) patch: https://gist.github.com/tpoechtrager/1645acc6bdf7d423f59a fix the issue?

[andrius4669]

http://s.go.ro/5e41mif0 that's demo MTH sent me. https://imgur.com/RZinfmU he is cn 13. I didn't used wc-ng myself, but from how he described issue, it should be easily reproducable by recording demo with wc-ng in DEMOPHOBIA. I'm not totally aware of mechanics behind DEMORECORDER_SKIP_PACKET and don't know wc-ng code very well, so I can't really tell whether that particular patch would fix issue.

[andrius4669]

normal demos do not contain any N_SERVINFO messages, by the way

[andrius4669]

your patch does some stuff which is totally unnecessary: it checks if it's first packet and drops only N_SERVINFO packets which come after first one. like I mentioned, normal demos NEVER contain N_SERVINFO packets. your demo recording engine apparently starts recording demo AFTER N_SERVINFO packet, therefore it does not cause issues in server which does not generate additional N_SERVINFO packets. non-conditionally excluding N_SERVINFO seems to be right choice for me, therefore I suggested DEMORECORDER_SKIP_PACKET_NC variant in my first comment.

[tpoechtrager]

I forgot about putdemoinfoobj() and thought N_SERVINFO was still required for the server description in the scoreboard. I haven't really touched the demorecorder code for several years.

Anyway, sending N_SERVINFO multiple times is a protocol violation, and thus the real bug. Nevertheless, I will fix this issue and do new builds next week.

[pisto]

Of course it is unusual, but I'm tailoring my protocol hacks to what is accepted by the vanilla client. you did that too when you were trying to measure the real ping of the clients.

[andrius4669]

@pisto while I agree that there is no violation (there is no official document on protocol, official client is reference implementation, and it does not break), you could have done this in different, less hacky, way, by using N_REQAUTH. @tpoechtrager should fix this regardless, N_SERVINFO with non-negative cn is kind of message which definitely should not be present in demo files, no matter how server behaves.

[pisto]

@andrius4669 the N_SERVINFO are used in spaghettimod for two modules. One to change the server description for already connected clients (I don't actually use that module right now), another to have network fences: since it's the only packet that triggers an immediate response without having any side effect, I use that to determine exactly when a client has received and elaborated all the previous network messages. It is handy to write a reliable maploaded module (hopmod checked for N_MAPCRC or for the first N_PING, but that works much worse because those messages aren't sent exactly when the client parses the new map message and require manual adjustment of timing thresholds). I do not use network fences anymore when doing N_REQAUTH on client join, because unfortunately the client responds with auth requests that are queued in the game::messages buffer, so the would be sent actually slightly after the fence response (in contrast to the challenge answer which is instead sent directly; I have already asked eihrul to fix it but he ignored me).

[tpoechtrager]

Fixed. New builds will most likely follow tomorrow.

[tpoechtrager]

New builds available @ http://wc-ng.sauerworld.org/builds/nightly/b/list. Sorry for the slight delay.