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

Change command validation on client #3590

Closed
DorpsGek opened this issue Feb 4, 2010 · 8 comments
Closed

Change command validation on client #3590

DorpsGek opened this issue Feb 4, 2010 · 8 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

DorpsGek commented Feb 4, 2010

dolly opened the ticket and wrote:

There is a new logic introduced in DoCommandP handling in command.cpp that was not in 0.7.x.

It's purpose is (according to the code comments) ....

	/* If the company isn't valid it may only do server command or start a new company!
 	 * The server will ditch any server commands a client sends to it, so effectively
 	 * this guards the server from executing functions for an invalid company. */

the actual code being...

 	if (_game_mode == GM_NORMAL && (cmd_flags & (CMD_SPECTATOR | CMD_SERVER)) == 0 && !Company::IsValidID(_current_company)) {
 		return_dcpi(CMD_ERROR, false);
 	}

But as a side effect this code also effectively disables the ability of patched servers to send "special" commands to clients (commands sent from server as OWNER_NONE). This functionality is required for some server modifications like citybuilder server goals for example (issuing CmdLandscapeClear commands from server code). I'm currently using this also for things like building/promoting industries from server side code and also other features.

Please consider changing this code so the check is executed only on server side, something like this...

 if (_network_server && _game_mode == GM_NORMAL && (cmd_flags & (CMD_SPECTATOR | CMD_SERVER)) == 0 && !Company::IsValidID(_current_company)) {
    		return_dcpi(CMD_ERROR, false);
   	}

It would allow us to continue running citybuilder and similar patched servers on 1.0.0 release without the need to modify clients.

dolly
(Ex's servers)

Reported version: 1.0.0-beta4
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/3590
@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 5, 2010

Rubidium wrote:

The problem is that allowing invalid companies can cause dereferencing NULL pointers with the new pool structure (it couldn't with the one in 0.7). So effectively removing the check for clients, like you do, means that a someone can trigger a segmentation fault (read: crash) on all clients of a server.

If I have to chose between crashing clients and you needing a custom build the choice is quite easy.


This comment was imported from FlySpray: https://bugs.openttd.org/task/3590#comment7521

@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 5, 2010

SmatZ wrote:

You can always create empty company with zillions of money, which the server won't allow connecting to, and use it for your "hacks".
Also, I wonder how it could work in the past, when for example the (invalid) company didn't have enough money for executing your command. Didn't it desync clients?


This comment was imported from FlySpray: https://bugs.openttd.org/task/3590#comment7524

@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 6, 2010

muxy wrote:

The invalid company has always enough money. Its in the 0.7.5 release :

if (!IsValidCompanyID(company)) return INT64_MAX;

And this feature has been translated into the trunk :

if (!Company::IsValidID(company)) return INT64_MAX;

It means that this feature (server sending commands like invalid company) is already inside. What Dolly (and also all goal server groups around - Ex, Mega, Luukland, ... ) needs, is to have the server the ability to send commands in order to have some control around the NETWORK GAME.

Goal servers has introduced a new way of playing and is creating big communities around OpenTTD and according the number of players we think they (players) like this way of playing (goal server games).

Can you tell us at what place it could trigger these segmentation fault, then we can help to avoid them. We do not choose the crashing client way, we choose to provide to the OpenTTD community a new way of playing OpenTTD in network mode.

I also know and think that merging goal server code into trunk is not ready yet (like Infra Sharing or other great patches) because there are as many releases as communities and, for now, we dont share code around communities and initial conception is not the same and dont use same techno. Luukland servers are organized around MySql Database, and i dont think other do like this. But at the end we have same needs. Could you please take this in account ? We will be happy to cooperate in such a way. Thanks for having read.


This comment was imported from FlySpray: https://bugs.openttd.org/task/3590#comment7533

@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 6, 2010

dolly wrote:

Well you can join our servers and see for yourself (or look at Akoz's citybuilder patch that was posted on forums some time ago). Not a single crash or repeated client desyncs for a very long time (using original unpatched clients). The possibilities with creating real company on server are quite limited (money, town authorities...) when compared what was possible on 0.7.x using OWNER_NONE.

So i would like to ask for your opinion. Is there a chance a patch would ever be accepted for trunk doing the following? ...

- allow DEDICATED server to issue and execute SELECTED commands in a special (maybe server company or special command flag) context
- such commands will not be subjected to town allowance, money and maybe also other checks on clients
- the most needed commands i can think of now are: CmdLandscapeClear, CmdBuildIndustry, and maybe also CmdPlaceSign, CmdRenameSign


This comment was imported from FlySpray: https://bugs.openttd.org/task/3590#comment7534

@DorpsGek
Copy link
Member Author

sparr wrote:

Can someone explain how the previous usage could cause a null pointer dereference with the new code? It seems like it would be better to address that actual problem, rather than forbid the behavior at this level.


This comment was imported from FlySpray: https://bugs.openttd.org/task/3590#comment7571

@DorpsGek
Copy link
Member Author

Rubidium wrote:

Old pool:
- an array with locations of 'array' chunks
- then continuous arrays with size "largest object in array" for each chunk
For example pool[id >> 8][id & 0xFF] was roughly how the lookup went. For companies it was even a simple array, not a pool.
Smoke, ships and such need to store way less information than e.g. trains, yet they all were in the same pool, i.e. lots of wasting of memory.

New pool:
- an array with pointers to the instances
- then an instance
I.e. *pool[id] is roughly how lookups go now, however for unused/invalid IDs (that are still within the realms of the pool) that would yield NULL. Effect in total is that less memory is used.
Also the company array has become a pool, so now *company[0] can yield a crash (dereferencing NULL), whereas it would be a valid read for the old ways. Note that 'valid' here only implies that it's correctly allocated memory, not that the memory has the right meaning.

Now the 'actual' problem: 0.7 and earlier did just accept the invalid company stuff because it didn't cause crashes, but it wasn't conceptually the right thing to do (i.e. reading from/writing to an invalid company). In 1.0 it does conceptually the right thing. Furthermore testing it only once instead of spreading it around the different functions isn't going to help; it only increases the maintaince cost and the chance of bugs. The Cmd* functions are already more than complex enough.

Even then DoCommandP was never meant to be called by invalid companies, that it worked is something completely different. It's like AIs assuming that cargo 0 is always 'passengers'; it'll work, but there might come a time where that simply isn't true (e.g. with NewGRFs).

Finally I'm not a fan of adding stuff that doesn't get tested on a regular basis, i.e. in the nightlies. Adding those functions that are specifically used for only your server means that we might break it at any time during development and, because you only seem to be occasionally testing new releases, mean that it gets shipped in a broken state and then you're mad at us again for breaking it.


This comment was imported from FlySpray: https://bugs.openttd.org/task/3590#comment7572

@DorpsGek
Copy link
Member Author

muxy wrote:

Okay, then you mean that a DoCommand and the Cmd called after, must refer to a company instance.

Then is it possible to introduce an extra hidden company ? This company has owner bits set to zero and will not be able to create "owned items" such as tracks, stations, all the stuff from a regular company. But it will be possible to this company to perform some action on "un-owned items", like creating new industry, building town items, and such things. Of course this company will have enough money ... and all the stuff around.

Just searching a way to maintain some functionnal things in order to keep city builder/mania alive.

Can we have your point of view about goal servers and city mania/builder in network mode ?


This comment was imported from FlySpray: https://bugs.openttd.org/task/3590#comment7586

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) labels Apr 7, 2018
@TrueBrain
Copy link
Member

Didn't read through the whole topic, but I don't think this should be part of OpenTTD. Feel free to rebuttal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Development

No branches or pull requests

2 participants