[Freeciv-Dev] Re: [PATCH] Buglet in server/savegame.c

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> On Sun, Oct 14, 2001 at 08:40:50PM +0100, Gaute B Strokkenes wrote:
> >
> > I just spotted the following while going through some of Jason's
> > recent changes (though the bug is not Jason's).
> >
> > strchr() has a misfeature in that if the character you are looking for
> > is 0, then it will return a pointer to the string's terminating NUL.
> > This causes us to do wrong things in the savegame loading code.
> 
> > As an aside, I believe that PR 974 may be related to this bug, but I
> > do not have time to investigate.
> 
> Yes this would catch the error a bit earlier in teh loading. Still
> unknown is what caused it in the first place. I have thought about
> doing:
> 
>   for (y = 0; y < map.ysize; y++) {                     \
>     for (x = 0; x < map.xsize; x++) {                   \
>       if (is_normal_map_pos(x, y)) {                    \
>         line[x] = get_xy_char;                          \
> +       assert(line[x]!='\0');
>       } else {                                          \
>         /* skipped over in loading */                   \
>         line[x] = '#';                                  \
>       }                                                 \
>     }                                                   \
> 
> So far I haven't implemented it. Comments?

Seems sensible.

On a related note, we should do a similar check while loading:

  for (y = 0; y < map.ysize; y++) {                           \
    char *line = secfile_lookup_line;                         \
    int x;                                                    \
-    if (!line) {                                              \
+    if (!line || strlen(line) < map.xsize) {                  \
      freelog(LOG_FATAL, "Invalid map line for y=%d.  "       \
             "This may be a bug in FreeCiv; see "             \
             "http://www.freeciv.org/ if you think so.", y);  \
      exit(1);                                                \
    }                                                         \
    for(x = 0; x < map.xsize; x++) {                          \
      char ch = line[x];                                      \
      if (is_normal_map_pos(x, y)) {                          \
       set_xy_char;                                           \
      } else {                                                \
       assert(ch == '#');                                     \
      }                                                       \
    }                                                         \
  }                                                           \

This is the equivalent of making sure that ch != 0 each time through the
x loop.  On loading, it must be checked when in NDEBUG mode as well; we
don't want to load a bogus save file.

I'll send a new LOAD_MAP_POS patch with this change.

jason