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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

rwetmore@xxxxxxxxxxxx wrote:
> Jason Short wrote:
> 
>>Because the server doesn't rigorously check the input coordinates for a 
>>client goto route, the client can send invalid (non-normal) coordinates 
>>to either trigger an assert or get Bad memory access on the server (this 
>>could be a security risk).
> 
> 
> This is a server bug.

Indeed.

> [...]
> 
> 
>>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.

> 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.

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.

IMO the first choice is undisputably the best.  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.

jason