Project

Profile

Help

HostedRedmine.com has moved to the Planio platform. All logins and passwords remained the same. All users will be able to login and use Redmine just as before. Read more...

Bug #881952

Server can't handle transform-mine and transform-irrigate as a unit order

Added by Alexandro Ignatiev almost 2 years ago. Updated about 1 month ago.

Status:
In Progress
Priority:
Normal
Category:
Server
Sprint/Milestone:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

I marked it "low-priority" because standard clients likely never send such an order, but v.2.5 and v.3.0 (where transforming activities are split from targeted ones) support it in theory. If you send an ORDER_ACTIVITY order with "Mine" or "Irrigate" activity with no target to transform the tile, the block of

if (pextra == NULL && activity_requires_target(order.activity)) {

in unittools.c sets the unit's activity idle (since it can't pillage, lol) and skips the order.
The order should be skipped only if the activity is actually pillage.
Below, we must put a fc_assert_ret_val(pextra, TRUE) in the branches using tile_has_extra(..., pextra)

History

#2 Updated by Alexandro Ignatiev almost 2 years ago

Ah sorry, it breaks pillage sometimes, this is the correct one.

#3 Updated by Marko Lindqvist almost 2 years ago

Master is not affected, as there Plant and Cultivate activities are separate from Mine and Irrigate. S3_0 version attached.

#4 Updated by Marko Lindqvist over 1 year ago

  • Status changed from Resolved to In Progress
  • Assignee changed from Marko Lindqvist to Alexandro Ignatiev

This has been on hold since it has seemed suspicious to me, and only now I found time to go through the code in detail. The result of that review is that this indeed does open up a new error situation. If client requests irrigate or mine activity for a terrain where it would not result in terrain transformation but an extra addition, server now let it pass all the way and server doesn't even try to fix the situation itself ( unit_assign_specific_activity_target() only handles pillage )

#5 Updated by Alexandro Ignatiev over 1 year ago

Sorry, don't understand. Explain please more thorough, how the error may occur? If a client asks an extra addition, it falls around the branch I have patched... If the terrain can't be transformed and there is no target, as well does... seems to me.

#6 Updated by Marko Lindqvist over 1 year ago

From server's point of view input from client is untrusted. So it's possible client sends illegal request to do transformative irrigate/mining when it's not possible. So my concern is about lack of target extra in a request when irrigate/mining on terrain in question should result in addition of an extra.
What you think code does in that situation? Where and how it rejects that request?

#7 Updated by Alexandro Ignatiev over 1 year ago

I think these lines prevent auto-setting mine/irrigate target where you can't tf-mine/irrigate

&& tile_terrain(dst_tile)
!= tile_terrain(dst_tile)->mining_result) // and irrigation->

after which the behaviour is mostly the same as it was, the check of can_unit_do_activity_targeted fails as well and the next branch I modified to have if (pextra) condition; as we have not set pextra, it's also not executed, and the orders are canceled. I can mod my client to check but it seems to me that it won't become less safe than it was.

#8 Updated by Marko Lindqvist over 1 year ago

It used to return immediately from handle_unit_orders() as unit_activity_needs_target_from_client() returned TRUE. The order was not even accepted. With your patch the thing is noticed only when executing orders, so it's quite a different thing.

That "! (... tile_terrain(dst_tile) != tile_terrain(dst_tile)->mining_result))" cause execution to go in the branch where server tries to assign target itself with unit_assign_specific_activity_target(). That doesn't work as unit_assign_specific_activity_target() can only handle pillage, for irrigate/mine it does nothing. Then there's some asserts in that branch that will fail, I think.

#9 Updated by Marko Lindqvist over 1 year ago

Marko Lindqvist wrote:

That "! (... tile_terrain(dst_tile) != tile_terrain(dst_tile)->mining_result))" cause execution to go in the branch where server tries to assign target itself with unit_assign_specific_activity_target(). That doesn't work as unit_assign_specific_activity_target() can only handle pillage, for irrigate/mine it does nothing. Then there's some asserts in that branch that will fail, I think.

I think there's two possible ways to fix this (esp the failing assert).

1) Adjust the if() so that server never enters to block to find target itself, when activity is irrigate or mine. Then later check for target being there will catch the case, and orders will be aborted

2) Add implementation to unit_assign_specific_activity_target() to handle irrigate and mine activities.

As the whole case we are discussing here is special case of illegal input from the client, I see no reason to cater too much for it -> solution 1 would be enough.

#10 Updated by Marko Lindqvist over 1 year ago

Marko Lindqvist wrote:

I think there's two possible ways to fix this (esp the failing assert).

Will you make a new revision of the patch with either solution?

#11 Updated by Alexandro Ignatiev over 1 year ago

I may think about it but in rather a long time...5

#12 Updated by Marko Lindqvist over 1 year ago

  • Sprint/Milestone changed from 2.6.3 to 2.6.4

#13 Updated by Marko Lindqvist about 1 year ago

  • Sprint/Milestone changed from 2.6.4 to 2.6.5

#14 Updated by Marko Lindqvist 11 months ago

  • Sprint/Milestone changed from 2.6.5 to 2.6.6

#15 Updated by Marko Lindqvist 5 months ago

  • Sprint/Milestone changed from 2.6.6 to 3.0.1

#16 Updated by Marko Lindqvist about 1 month ago

  • Sprint/Milestone changed from 3.0.1 to 3.0.2

Also available in: Atom PDF