[Freeciv-Dev] Re: Unsafe assertions. (PR#864)

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

Gaute B Strokkenes wrote:

> The reason this is dangerous is that normaliz_map_pos() will sometimes
> change the value of x.  If we compile with NDEBUG, this does not
> happen, and a crash may occur.
> 
> I think we should changes these uses to is_real_tile() instead, and
> use normalize_map_pos() explicitly if we rely on the value of x being
> normalised afterwards.

Ouch!  That's bad!

The assertion is there no doubt because the tile is supposed to be
normalized to begin with - however, as you point normalize_map_pos
doesn't check that for the X value.  Unless there's time to debug things
more, the safest solution for the release would be to move
normalize_map_pos out of the assertion.  After the release, this can be
replaced by an assert(is_real_tile(...)) call.

The problem with replacing normalize_map_pos by is_real_tile is that it
doesn't change the behavior of the program if it's NDEBUG - which it
will be for the release.  Since the assertion hasn't been doing it's job
so far, this is dangerous.  How about changing it as follows:

  assert(normalize_map_pos(&x, &y));

---change to---

  assert(is_normal_map_pos(x, y));
  normalize_map_pos(&x, &y);

This will be the correct fix when debugging, and will also be safe when
not debugging.  Later, the extra normalize_map_pos calls can be removed.

(BTW, I think is_normal_map_pos would be a better name than
is_real_tile.)


There are a number of other places where functions are called within
assertions.  This is safe so long as the function has no side effects,
but it's harder to verify that this is the case with a function than
with a simple comparison.  I think it's better to avoid this where
possible, for instance the following code in goto.c line 650 could be
changed:

void decrement_drawn(int x, int y, int dir)
{
  assert(*get_drawn_char(x, y, dir) > 0);
  *get_drawn_char(x, y, dir) -= 1;
}

---change to---

void decrement_drawn(...)
{
  char *drawn_char = get_drawn_char(x, y, dir);
  assert (*drawn_char > 0);
  *drawn_char -= 1;
}

In this case, not only does the change avoid duplicating the function
call, it also makes it instantly obvious that the assertion is safe.

jason