[Freeciv-Dev] Re: resubmission: bug in vnotify_conn_ex (PR#1185)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

--- "Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx> wrote:
> At 11:46 AM 02/07/20 -0700,
> jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> >This is a resubmission of the patch to fix a future
> bug in vnotify_conn_ex.
> >
> >The bug is two-fold:
> >
> >- is_real_tile and friends should not be called
> unless the game has 
> >started.  Thus, the check for server_state >=
> RUN_GAME_STATE has been 
> >moved up.
> 
> Correct.
> 
> >- (-1,-1) is a special case meaning that the event
> has no associated 
> >coordinates. 
> 
> This should be trapped next and passed through.

Correct.

> > In the future it may be possible for (-1,-1) to be
> a real 
> >position; however, it is assumed that it will not
> be a normal position 
> >(otherwise this code will completely break). 
> 
> Incorrect, or only poorly thought through. If
> (-1,-1) is anything but a 
> special case, it will need to be dealt with
> consistently throughout the
> code. Presumably, a new flag value will be
> implemented, but it doesn't
> matter - that should be left to the person fixing
> things at that time.

(-1,-1) must remain a non-normal position to stay a
special case without changing the rest of the code. 
This is what the assertion is intended to guarantee.

> If it is a normal coordinate, it will be passed
> through unchanged by 
> the suggested alternative you keep refusing to
> accept. This alternative
> does not break any existing or past code.

Incorrect.  If (-1,-1) can mean either the coordinates
(-1,-1) _or_ the special-case of no coordinates, then
for it to be passed through directly to the client
will lead to a direct bug.  The client will probably
treat real events at the position (-1,-1) as having no
associated coordinates.

> > Thus I have added two 
> >assertions: one to assert that (-1,-1) is a
> non-normal position, 
> 
> An unnecessary restriction which in fact breaks
> current code. Go back
> and reread the earlier email thread. It is either
> special and set to
> (-1,-1) or normal and set to (-1,-1). 

See above.  I still feel you are misunderstanding the
situation.

> No need to crash the server in either case, or add
> new faulty conditions 
> to what you think it should be until you fix the
> rest of the codebase 
> to conform and provide the backwards compibility for
> earlier clients 
> that treat things as they currently are.
> 
> >and 
> >another to assert that any position other than
> (-1,-1) that is passed in 
> >is normal. 
> 
> The is_normal test here is again one of the "foolish
> mistakes", is_real
> is the only relevant condition of concern here as
> this is a portal case.
> Use of normalize_map_pos() will detect this and do
> the correct thing.
> It will also fixup any real but unnormalized
> coordinates without causing
> an assertion in debug code, or segfault in released
> versions and allow
> the game to proceed normally.
> 
> Normalize_map_pos() does what is needed. It doesn't
> require a new set
> of useless constraints be applied and then
> religiously checked where 
> their only real effect is to add performance penalty
> and crash the 
> server unnecessarily.

Again, incorrect.  In the (hopefully near) future,
(-1,-1) will be a real, non-normal position in some
topologies.  Thus if the position (-1,-1) is passed to
us we have two situations: where it is intended as the
special-case (no associated coordinates), or where it
is intended as the real but unnormalized position
(-1,-1).  The second is a bug, but impossible to
detect with checks in the code since the two
conditions are (practically) indistinguishable.

The only way to check for this bug is to check
positions other than (-1,-1) to make sure they are
always normal.  That way as long as we never get any
non-normal positions passed in (aside from the special
case) we can be reasonably sure that the code is
bug-free.

Or, to put it another way: if the assertion fails, it
does not crash the server "unnecessarily".  Rather, it
has found a bug that would otherwise go unnoticed for
a long time.

> The alternative is in the corecleanups and has been
> running happily for
> many moons without segfaults or spurious assertion
> errors.

Your alternative does nothing to detect the bugs I
describe above.  It runs happily either because there
are no bugs of this type in the code and/or because
the bugs are nearly undetectible without the
assertions.

jason


__________________________________________________
Do You Yahoo!?
Yahoo! Health - Feel better, live better
http://health.yahoo.com