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

[Jason Short <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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?)
>
> = = 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.
This would probably still fail under FREECIV_PATH=" ".  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.
jason