Problems with Crafty's "pers load" command

[maschmdt]

Hello!

I just wanted to experiment with crafty's personality feature and ran in subsequent crashes under Windows

It was reproducible with the following command sequence:

pers save t
pers load t

In option.c found this glitch:

Code: Select all

      Print(128, "Loading personality file %s\n", filename);
      if ((file = fopen(filename, "rw"))) {

maybe the mode "rw" is undefined for windows libs (should be "r+" to aquire read and write rights).

Anyway, I changed it to:

Code: Select all

      Print(128, "Loading personality file %s\n", filename);
      if ((file = fopen(filename, "r"))) {

This compiles and works fine under Windows (XP and Vista)

Michael.

[Sven]

In option.c found this glitch:

Code: Select all

      Print(128, "Loading personality file %s\n", filename);
      if ((file = fopen(filename, "rw"))) {

maybe the mode "rw" is undefined for windows libs (should be "r+" to aquire read and write rights).

I would consider a call to fopen(..., "rw") to be invalid on any platform. POSIX at least does not allow "rw". But in theory, fopen() should not crash by itself, not even on Windows. In case of an invalid mode argument fopen() shall set errno to EINVAL and return a null pointer. Obviously this is not the case on Windows.

Btw I guess that fopen(..., "r") is fully sufficient here since there is no write access required IMO.

To be safe (on platforms where wrong mode does not crash), I would add error checking code like this, similar to the "save" case some 60 lines above but even slightly extended:

Code: Select all

// and don't forget to add these at the top for strerror() and errno:
// #include <string.h>
// #include <errno.h>

      if (&#40;file = fopen&#40;filename, "r"))) &#123;
        // ...
      &#125; else &#123;
        printf&#40;"ERROR.  Unable to open personality file %s for reading&#58; %s\n",
            args&#91;2&#93;, strerror&#40;errno&#41;);
        return &#40;1&#41;;
      &#125;

and probably I would also add the strerror(errno) stuff at some other places where system library functions may fail and the system's internal error message may help to analyze the problem. Of course it's a matter of style.

Sven

[bob]

In option.c found this glitch:

Code: Select all

      Print(128, "Loading personality file %s\n", filename);
      if ((file = fopen(filename, "rw"))) {

maybe the mode "rw" is undefined for windows libs (should be "r+" to aquire read and write rights).

I would consider a call to fopen(..., "rw") to be invalid on any platform. POSIX at least does not allow "rw". But in theory, fopen() should not crash by itself, not even on Windows. In case of an invalid mode argument fopen() shall set errno to EINVAL and return a null pointer. Obviously this is not the case on Windows.

Btw I guess that fopen(..., "r") is fully sufficient here since there is no write access required IMO.

To be safe (on platforms where wrong mode does not crash), I would add error checking code like this, similar to the "save" case some 60 lines above but even slightly extended:

Code: Select all

// and don't forget to add these at the top for strerror() and errno:
// #include <string.h>
// #include <errno.h>

      if (&#40;file = fopen&#40;filename, "r"))) &#123;
        // ...
      &#125; else &#123;
        printf&#40;"ERROR.  Unable to open personality file %s for reading&#58; %s\n",
            args&#91;2&#93;, strerror&#40;errno&#41;);
        return &#40;1&#41;;
      &#125;

and probably I would also add the strerror(errno) stuff at some other places where system library functions may fail and the system's internal error message may help to analyze the problem. Of course it's a matter of style.

Sven

No idea how that crept in, but "r+" is correct rather than "rw". Although "r" will probably work just fine there since it is reading anyway.