[Freeciv-Dev] Re: Assertion failure during savegame load when FREECIV_PATH is set but empty (PR#1341)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sat, Mar 23, 2002 at 12:23:00PM -0800, Jason Short wrote:
> Steven Taschuk wrote:
> > = = SYMPTOM = =
> > 
> > When the environment variable FREECIV_PATH is set to an empty string,
> > and one tries to start the server with a saved game, an assertion fails.
> > 
> >     $ export FREECIV_PATH=
> >     $ civserver -f civgame-3000.sav
> >     This is the server for Freeciv version 1.12.0
> >     You can learn a lot about Freeciv at http://www.freeciv.org/
> >     2: Loading saved game: civgame-3000.sav
> >     2: Loading rulesets
> >     civserver: shared.c:311: skip_leading_spaces: Assertion `s!=((void 
> > *)0)' failed.
> >     Aborted
> > 
> > = = DISEASE = =
> > 
> > The problem arises in common/shared.c:602-612.  (Line numbers refer to
> > the 1.12.0 release.)
> > 
> >     char *path = getenv("FREECIV_PATH");   /* path is "" */
> >     if (!path) {
> >       path = DEFAULT_DATA_PATH;
> >     }
> >     path = mystrdup(path);
> > 
> >     tok = strtok(path, PATH_SEPARATOR);    /* so strtok returns NULL here */
> >     do {
> >       int i;
> > 
> >       tok = skip_leading_spaces(tok);      /* tripping the assertion here */
> > 

> > (Digression:  Being a conscientious bug reporter, I checked these
> > lines in the current CVS, and found this:
> >     path = getenv("FREECIV_PATH");
> >     if (!path) {
> >       path = DEFAULT_DATA_PATH;
> >     }
> >     assert(path != NULL);
> > This assertion seems a bit strange, since it can only fail if the
> > compile-time constant DEFAULT_DATA_PATH is NULL.  But why not check
> > this at compile time?)

This assertion is indeed a bit strange.

> > = = SUGGESTED CURE = =
> > 
> > Obviously having FREECIV_PATH set but empty is a misconfiguration;
> > nevertheless there is a bug here, to wit, the server's graceless
> > reaction to the situation.  I suggest the following patch.
> > 
> > --- common/shared.c.distribution        Sat Mar 23 11:12:02 2002
> > +++ common/shared.c     Sat Mar 23 12:38:22 2002
> > @@ -602,6 +602,10 @@
> >      char *path = getenv("FREECIV_PATH");
> >      if (!path) {
> >        path = DEFAULT_DATA_PATH;
> > +    } else if (*path == '\0') {
> > +      freelog( LOG_ERROR, _("FREECIV_PATH is set but empty; "
> > +                            "using default path instead.") );
> > +      path = DEFAULT_DATA_PATH;
> >      }
> >      path = mystrdup(path);     /* something we can strtok */
> > 
> > This code notices the misconfiguration and adapts in a sensible way,
> > producing an informative error message in the log.

The patch is ok.

> This would probably still fail under FREECIV_PATH=" ".

No.

> Instead, what about changing
> 
>    if (!path) {
> 
> to
> 
>    if (!path || !strtok(path, PATH_SEPARATOR)) {
> 
> ?  If someone's worried about calling strtok twice, the code could be 
> changed to hold onto this value.

If you handle this you would also have to handle " ; \t;; "

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "We've all heard that a million monkeys banging on a million typewriters
  will eventually reproduce the entire works of Shakespeare.
  Now, thanks to the Internet, we know this is not true."