Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

We need set delay in everything on tfs 1.2 #1845

Open
vankk opened this issue Jun 28, 2016 · 37 comments
Open

We need set delay in everything on tfs 1.2 #1845

vankk opened this issue Jun 28, 2016 · 37 comments
Labels
needs-triage Needs testing with screenshot/video confirmation priority: high

Comments

@vankk
Copy link
Contributor

vankk commented Jun 28, 2016

Hi my dear friends, I'm here to discuss a seriouslly problem that I'm facing along of two months, and it must be fixed before 1.2 releases. We need set delays in everything!

I say that because I'm facing this problem along of two months, the brazillian players are the worst in the world, they only complains and found bugs on everything.

What I'm facing for two months:

Every kinda of thing that send packets to the server can be abused, by saying that I mean, when you use a stealth ring and after removes it and use a program called WPE PRO it will crash the server!

Everything that have effects must needed seted a delay for it.

Why we need this?

To avoid unsafe scripts and also to not use storages to check exausted time on everything. I'm running a server with 50-70 daily players and all my server has been seted with storage exausted. That does not make any sense. We need set a delay.

I'll use the template issue to report the bug that I said above, and avoid it be closed, also this bug is the first of many!

Steps to reproduce

  1. Pick a stealth ring.
  2. Use the stealth ring and remove it with the program called WPE PRO, or any kinda of that program type.
  3. Crash.

Expected behaviour

Not crash or even seted a delay for this player?

Actual behaviour

The step three.

Environment

Returns a "floating poin exception" on console, that is all. I don't have any gdb log because I already fixed by setting a delay. The system is Ubuntu 14.04.

@marksamman marksamman added the needs-triage Needs testing with screenshot/video confirmation label Jun 28, 2016
@marksamman
Copy link
Member

If someone can reproduce this, please post a stacktrace from gdb.

@vankk
Copy link
Contributor Author

vankk commented Jun 28, 2016

Mark, on this video did by my friend, just shows up what kinda we are facing.

It shows up the program WPE PRO with a few DDoS, but the part of DDoS can be ignored, the most important part is how the WPE PRO is affecting the TFS 1.2

@marksamman
Copy link
Member

A sane value for maxPacketsPerSecond in config.lua should mitigate that.

@vankk
Copy link
Contributor Author

vankk commented Jun 28, 2016

With sane value how much did you mean?

@marksamman
Copy link
Member

The default value of 25 should be good enough. As for that video in particular, that's a weakness in some cast system and not TFS 1.2. Anyway, if it's possible to cause a crash, that should be fixed, the problem shouldn't be hidden behind some delays. So if someone can reproduce the crash, please share the gdb stacktrace so that it can be properly fixed.

@vankk
Copy link
Contributor Author

vankk commented Jun 28, 2016

The default value isn't too low? May players that use MageBot can be affect, isn't?

@marksamman
Copy link
Member

A bot that floods the server with packets is a DoS attack. If you're going to make exceptions for them, then you're making an exception for everyone who wants to DDoS the server. As for the other crash, would you mind opening a new issue about that? It's easier to track them that way (so that the label and open/closed state reflects one particular issue and not multiple issues).

@vankk
Copy link
Contributor Author

vankk commented Jun 28, 2016

There is no way to make a exceptions for thoses who use bot, but still not creating any exception for who wants DDoS the server?

I don't mind.

@marksamman
Copy link
Member

Not for bots that flood packets, that has to be fixed in the bot.

@vankk
Copy link
Contributor Author

vankk commented Jun 28, 2016

Do you mind in share what you did on ShadowCores? Because I remember that players still could use bots and didn't excedeed the limit packets. It's what you did with anti push and the rings?

@marksamman
Copy link
Member

ShadowCores used 40 for maxPacketsPerSecond, bots exceeding that would be disconnected and we couldn't have cared less. However, 40 will be too high for the average server that doesn't have a powerful CPU, efficient scripts and optimized sever configuration. Anti push was an unrelated change, and didn't affect how many packets the bots would send, only how many packets the server would choose to process.

@imranhhk
Copy link

imranhhk commented Jun 29, 2016

@vankk I'm having the same issue thanks for bringing it to attention. I'll try to reduce my maxPacketPerSecond as Mark has recommended and see if that solves this problem. (From 50 -> 35)

I also had increased it for the Magebot users but if it's causing this lag/kick problem it's better that they don't dash then the make the rest of the server suffer.

Edit:
I do not believe my issue is related to this, sorry. Even when I close the server and it's just me online I was still getting lag/kick during what I believe now to be is an external attack not in-game abuse.

@Mkalo
Copy link
Contributor

Mkalo commented Jun 30, 2016

I could not replicate this issue, maybe it has to be players around me to work?
Used autohotkey instead of wpe pro.

https://youtu.be/MjHwz92-D_A

Well it will be hard to replicate, I doubt this is going to happen in a local server anyway, and I wont go around servers crashing it just to see if it works or not. If anyone has a server with people that is willing to test it :)

@vankk
Copy link
Contributor Author

vankk commented Jul 1, 2016

It only works with WPE PRO, Mkalo.

@CZacK
Copy link

CZacK commented Jul 2, 2016

The problem was solved? If so, share with us friend Vankk.

@ranisalt
Copy link
Member

ranisalt commented Jul 2, 2016

Do as Mark said and open the server on gdb to print the stack trace when the server crashes. That way we will see exactly where something happened.

@Ezzz-dev
Copy link

Does this bug still happen ? Can't it be a thread-safety issue within dispatcher & scheduler making changes to live game code ?

@kito2
Copy link

kito2 commented Oct 2, 2016

Guys I recommend you to just lower the size of packets.

The magebot has it's own configuration for "dash" and it is called "boost timer"... Players will have to manage their configuration in order to use the allowed packet limit without being kicked.

For example in CipSoft, players using boost between 1-50 has higher chances to get deleted. I personally own char 500, using dash for about 2 years with boost timer on 70 and haven't been deleted yet.

If packet size is limited to 25, then your boost timer will be limited to 70. Lets say that the difference between 70 and 1-50 config is around a 5% lower on the moving speed.

Still not a good solution, but players might acomodate to our protection configurations.

Just saying.

@cmarfil
Copy link

cmarfil commented Feb 3, 2017

I have been able to experience the same problem, in my case they use WPE pro with the boat npcs, I do not know exactly how they do it, but I have seen live how they put the cpu to 100% and server crash without logs.

@kito2
Copy link

kito2 commented Feb 3, 2017

They repeat the packets, that's why they make crash the server maybe.

@cmarfil
Copy link

cmarfil commented Feb 3, 2017

Yes, I think so, in my case I have:
maxPacketsPerSecond = 25

@kito2
Copy link

kito2 commented Feb 3, 2017

You can put to 1 and it will still crash, since those sent packets are "ilegal" somehow.

@cmarfil
Copy link

cmarfil commented Feb 3, 2017

I just tried with a stealth ring, also traveling from venore to svargrond without stopping, really wpe pro works but I can't crash the server, however I'm testing in a local environment, not in production.
Can it affect more when there are other players nearby or something like that?

@kito2
Copy link

kito2 commented Feb 3, 2017

They open cast, connect lots of MC, and start sending the packets.

@cmarfil
Copy link

cmarfil commented Feb 4, 2017

There must be a solution, the big ots sure that they know it, their ots do not go the brs asking for money for not crashing the server...

@OPfeajoief13
Copy link

bounty added

@darkjav
Copy link

darkjav commented Jun 26, 2017

@OPfeajoief13
Thanks, work fine, but i think not is the best solution for the another actions out of the NPC system.

@dudantas
Copy link
Contributor

later I will present you some solutions outside the npc system, including source

@Zbizu
Copy link
Contributor

Zbizu commented May 1, 2020

So the issue is related to cast system we don't have? If that's the case, I don't think that there is much to do.

@EPuncker
Copy link
Contributor

EPuncker commented May 1, 2020

So the issue is related to cast system we don't have? If that's the case, I don't think that there is much to do.

it seems so... we need @vankk to confirm if this issue is present in a clear tfs install or if this issue can be closed

@yamaken93
Copy link
Member

yamaken93 commented May 1, 2020

TFS has a major fail: almost no rate limits. Imagine a client spamming 30 set outfit packets, 20 add vip packets, 17 market packets, etc. This slow downs the TFS engine and enable macros without restrictions. This is a design/lack of feature issue as a server(whatever its serving) should have rate limits and currently tfs only has one when it comes to packet(client requests): maxPacketsPerSecond. If you want to set maxPacketsPerSecond high so you allow the bot dash feature to work, you allow all the other packets to have such high limit when they don't need or worst its harmful.

@ramon-bernardo
Copy link
Contributor

@yamaken93 this is a big missing feature here, I considered using CONDITION on my server but it's not viable.

@Zbizu
Copy link
Contributor

Zbizu commented Oct 23, 2021

Some features like walking should be kept allowed, but for non-vital computation heavy or database reliant requests (like pulling data out of market), I believe an unified protocol delay could be made. Or a few of them with enums.

Another place worth looking into is character list/gameworld login request. Limiting correct requests for character lists (if not already implemented) and amounts of character relogs spams per minute could help the server a bit.

As for moving items, I believe "fast hand" game functionality should be kept, but the players should have a chance against bots so something like 30 or 50 ms delay would do the trick without making the feature noticeable.

Could be implemented when solving data race some time after we get protocol 12 merged.

@ramon-bernardo
Copy link
Contributor

ramon-bernardo commented Oct 23, 2021

Some features like walking should be kept allowed, but for non-vital computation heavy or database reliant requests (like pulling data out of market), I believe an unified protocol delay could be made.

Another place worth looking into is character list/gameworld login request. Limiting correct requests for character lists (if not already implemented) and amounts of character relogs spams per minute could help the server a bit.

We can create an enum with the bytes, and from that a count for each type.
I now started enum bytes based on protocolgameparse

@Zbizu
Copy link
Contributor

Zbizu commented Oct 23, 2021

We can reuse the code for muting spam on local chat btw.

@Zbizu
Copy link
Contributor

Zbizu commented Oct 25, 2021

by the way, what is max pathfinding distance? What should it be for tasks outside viewport range?

this needs checking (it's important):

Hypothetically there are things in protocolgame that go directly from parsing position to pathfinding.
Pathfinding calls on long distances tend to be computing heavy.

Connecting the dots we can draw a conclusion that the server might be vulnerable for spam of crafted packets that could theoretically lag the server. This needs to be tested.

Potential solutions:

  • Limiting spammability of pathfinding-generating packets
  • Setting viewport as max position difference before searching for things that are supposed to be in game window

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage Needs testing with screenshot/video confirmation priority: high
Projects
None yet
Development

No branches or pull requests