[Freeciv-Dev] Re: [PATCH] bug in vnotify_conn_ex() (PR#1192)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

Ross W. Wetmore wrote:

> At 03:44 AM 02/01/02 -0500, Jason Short wrote:
>
> > This buglet is very similar to PR#1184.
> >
> > The function vnotify_conn_ex() notifies a player of an event.  A 
> > coordinate set can optionally be passed in to give the location of the 
> > event, or (-1,-1) can be passed to identify a "no-location" event.  This 
> > is a small hack, but nothing much to worry about.
[snip]


> Remember, is_normal_map_pos() was only deemed useful in asserts and 
> allowed for the case of CHECK_POS(), i.e. to find passed coordinates 
> now considered bogus during the process of debugging the new policy. 
That was your position, but not mine.  I agreed that that was mostly true.


> The general rule is ... 
> is_normal_map_pos() use is almost always a mistake or hiding one :-).
In this case, the mistake is that (-1,-1) is being used to indicate that 
there is no tile position, when it is possible (in theory, and hopefully 
in practice soon) that (-1,-1) is also a real, valid position.  Our 
choices then become:
1.  Use is_real_tile to check the tile's validity, and special-case 
(-1,-1) as a nonexistent position.  This is totally wrong IMO; if 
non-normal positions are ever passed in eventually (-1,-1) will be 
legitimately passed in and will be treated as a nonexistent position.  I 
am baffled as to why you have chosen this way...
2.  Force all map positions passed in to be "normal", and check for 
is_normal_map_pos.  This is the same as #1 except that we force the 
caller to deal with calling normalize_map_pos, and thus avoid the 
potential introduced by #1.  It _does_ present a problem because on the 
client side the set of normal positions may be different, but it's safe 
enough to special-case (-1,-1) there because we trust that the server 
has taken care of any problems.
3.  Do a real cleanup and pass in extra data to let us know whether the 
position is relevant.  For instance, we could pass in pointer to a 
struct map_position (which may be NULL) or simply an extra flag 
containing the data (which is better since it can be sent across the 
network as such).  This would be the "correct" solution, but it would 
take more work.  Raimar has expressed the opinion that the fix isn't 
worth the effort.
Now, you may point out that all we're doing is sending (-1,-1) as the 
position to the client in place of (-1,-1).  But suppose such an event 
happens and (-1,-1) is sent to the client.  Should the client think that 
this event has no coordinates associated with it (for instance, if it 
happens out-of-sight), or that the coordinates (-1,-1) are associated 
with the event?  If we use your system, there is no way to tell.  But if 
we force these positions to be "normal" and specify that (-1,-1) can 
never be "normal" then there is no ambiguity.
<change of topic>
When we talk about the similar goto_dest problem, the use of #1 is even 
worse, because the solution you propose doesn't even special-case out 
the "nonexistent position" values!  Under an arbitrary topology, when 
position (0,0) (or (-1,-1) which would be better) is initialized as the 
goto_dest, your code will call normalize_map_pos on it to check for 
existence.  But this is totally wrong!  A position is either (1) 
non-existent, in which case the coordinates will be a fixed (0,0) which 
may either be real or not or (2) existent, in which case the coordinates 
had better be real and should probably be normal as well.  Again, making 
the code "correct" would be the best choice, but choice #1 must be 
avoided at all costs.  Essentially, you are "initializing" the goto_dest 
to just point at some random position on the map.
</change of topic>

Perhaps we can just get off this argument and do the full fix instead?


> > There are still some minor pitfalls: (1) it 
> > obviously won't work if is_normal_map_pos(-1,-1), and (2) we need to be 
> > careful about non-normal positions other than (-1,-1) being passed in; 
> > these would probably indicate a bug.  Both of these can be detected with 
> > a well-placed assertion.
> The use of assertions is really unnecessary. This is a reasonable point
> to do full checks and fixups as an entry/exit data portal. Killing a server
> everytime a client does something bogus is not good form.
This data is being sent to the client, not taken from it.  It is the 
server that is generating these positions, and it should not be possible 
for the client to get the server to generate a bogus position.  But if 
it is possible, then this is the place to catch it.

> The original code did full checks. There might actually be a reason for 
> this, other than happenstance. Certainly it has not caused problems, so
> there is no reason to change it here.
Currently it is safe to pass in (-1,-1) to mark a "non-existent" 
position, because (-1,-1) will always be unreal.  Under different 
topologies, (-1,-1) will not always be unreal.  Thus, a change is necessary.
The reason the original code did the full check is that coordinates were 
not assumed to be normal when they were passed in, so the code knew it 
had better check for full realness (and normalize them too).  This is 
the situation that CHECK_MAP_POS has changed.  You may argue that the 
change is unnecessary, but it has three definite advantages: (1) less 
code overall, (2) the code is faster, and (3) hacks like this will work 
without introducing bugs.
jason