[Freeciv-Dev] Re: (PR#4594) topology fix for goto route packet

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

The third option has no disadvantage except in certain religious mindsets.

Jason Short wrote:
> rwetmore@xxxxxxxxxxxx wrote:
>>Jason Short wrote:
[...]
>>>The second attached patch, fix-server.diff, will avoid the crash by 
>>>testing for the normal-ness of the position.  Note that this means the 
>>>client must send NORMAL positions even though it seems the server could 
>>>easily call normalize_map_pos on them.  This patch is targeted toward 
>>>S1_14 and fixes a potential security hole.
>>
>>This is a *bad* fix. Requiring clients to do things in order to prevent
>>server crashes just violates fundamental programming norms.
> 
> Of course, the patch does exactly the opposite.

It *requires* clients to send only normal positions. You stress this twice
in the above lines. That really doesn't seem like just "the opposite" ...

>>And not doing
>>things to trivially cleanup the problem, i.e. failing the client, is just
>>rampant user-unfriendliness.
>>
>>This case has been discussed many times and enforcing religious constraints
>>on client behaviour has always raised the same negative reaction.
> 
> It has been discussed many times before, and every time I say the same 
> thing:
> 
> If you have a special case, e.g. (255,255) as an "end-of-route marker", 
> then you cannot simply call normalize_map_pos and be done with it.

Agreed, you need to first check the special case, then deal with remaining
normalization issues - that is case 3 below which you always reject in
favour of client restrictions and a case 1 solution.

> The options are:
> 
>    if (!is_normal_map_pos(x, y)) break;
> 
>    if (!normalize_map_pos(x, y)) break;
> 
>    if ((x == 255 && y == 255) || !normalize_map_pos(x, y)) break;
> 
> The first has the disadvantage that client must send only normal positions.
> 
> The second has the disadvantage that it's just plain wrong under torus 
> topologies - (255,255) will pass the test since it is a real position.
> 
> The third has the disadvantage that if the client actually sends any 
> non-normal positions you'll end up with buggy behavior: eventually 
> they'll send (255,255) and have their goto route prematurely terminated.

No disadvantage ... (-1,-1) has always been a special case, and can
certainly continue to be one in the future.

It has *no* impact on any  other normalization issues as long as you deal
with it first, i.e.before applying some generic normalization. There is
nothing buggy in normalizing any other coordinates as they are not and
have never been special.

The only *bug* is a religious issue about treating special cases as
special cases - but that is just the way life works sometimes :-).

> IMO the first choice is undisputably the best. 

I know, you always insist on restricting client behaviour as the solution
to the server problem. It is neither necessary nor advisable.

> Of course, there is a 
> fourth choice: adding an additional is_valid flag (to network code and 
> internal storage) to mark the special case.  This has been discussed and 
> rejected in the past, but personally I'm not opposed to it - it makes 
> the logic cleaner but the code lengthier.

Redefining the special case for bad coordinates is an option. There is no
real good solution here within the existing coordinate space and thus one
needs an extra flag. This is also a lot of code change for something that
is not required as making an existing coordinate special has always been
the valid solution and still is.

> jason

Learn to live with coordinates that are real or unreal, and don't be hung
up on the artificial condition of normalness that is neither well defined
nor particularly useful in most technical situations.

Cheers,
RossW
=====