[Bug] Unix to Windows Portability bug in g.tempfile

[girishnand]

Describe the bug
g.tempfile on Windows 11 generates the following input/output -

g.tempfile pid=235123 --verbose
<LOCATION><MAPSET>.tmp/unknown\235123.0

To Reproduce
As above.

Expected behavior
Windows file paths should use backward and not forward slashes.

System description (please complete the following information):

• Operating System:
  Windows 11

• GRASS GIS version -

g.version
GRASS 8.3.dev (2023)
(Tue Dec 12 00:33:01 2023) Command finished (0 sec)

Additional context
The bug can be traced to line 166 of file grass\lib\gis\tempfile.c in the function G__temp_element(), which uses '/' where HOST_DIRSEP would perhaps be more appropriate.

[girishnand]

In this function -

/*!
 * \brief Populates element with a path string (internal use only!)
 *
 * \param[out] element element name
 * \param tmp TRUE to use G_make_mapset_element_tmp() instead of
 * G_make_mapset_element()
 */
void G__temp_element(char *element, int tmp)
{
    const char *machine;

    strcpy(element, ".tmp");
    machine = G__machine_name();
    if (machine != NULL && *machine != 0) {
        strcat(element, "/");
        strcat(element, machine);
    }

    if (!tmp)
        G_make_mapset_object_group(element);
    else
        G_make_mapset_object_group_tmp(element);

    G_debug(2, "G__temp_element(): %s (tmp=%d)", element, tmp);
}

strcat(element, "/");
should be

strcat(element, HOST_DIRSEP );

[hellik]

Windows file paths should use backward and not forward slashes.

since ages, MS Windows itself supports both, backward and forward slashes:

C:\>cd d://wd
C:\>d:
d:\wd>cd ..
d:\>c:
C:\>cd d:\\wd
C:\>d:
d:\wd>

[girishnand]

Apparently, not on my machine.

C:\Users\Girish>dir c:\mydata\grassscripts\script1.txt
 Volume in drive C is OS
 Volume Serial Number is C84E-E522

 Directory of c:\mydata\grassscripts

12-12-2023  14:35            29,373 script1.txt
               1 File(s)         29,373 bytes
               0 Dir(s)  234,900,754,432 bytes free

C:\Users\Girish>dir c:/mydata/grassscripts\script1.txt
Invalid switch - "mydata".

C:\Users\Girish>dir c:/mydata/grassscripts/script1.txt
Invalid switch - "mydata".

C:\Users\Girish>dir c:\mydata\grassscripts/script1.txt
Parameter format not correct - "script1.txt".

C:\Users\Girish>ver

Microsoft Windows [Version 10.0.22621.2715]

C:\Users\Girish>

It appears to parse correctly as long as it sees backslashes, but fails at the first forward slash in the path.

regards,

[neteler]

strcat(element, "/"); should be

strcat(element, HOST_DIRSEP );

Would it harm to change this, @hellik (others) ?

[nilason]

strcat(element, "/"); should be
strcat(element, HOST_DIRSEP );

Would it harm to change this, @hellik (others) ?

Wouldn't do any harm I think, it is also used elsewhere without complications. Probably safer that way. I'm curious however why this hasn't been an issue so far (with g.tempfile).

[hellik]

Apparently, not on my machine.

C:\Users\Girish>dir c:\mydata\grassscripts\script1.txt
 Volume in drive C is OS
 Volume Serial Number is C84E-E522

 Directory of c:\mydata\grassscripts

12-12-2023  14:35            29,373 script1.txt
               1 File(s)         29,373 bytes
               0 Dir(s)  234,900,754,432 bytes free

C:\Users\Girish>dir c:/mydata/grassscripts\script1.txt
Invalid switch - "mydata".

C:\Users\Girish>dir c:/mydata/grassscripts/script1.txt
Invalid switch - "mydata".

C:\Users\Girish>dir c:\mydata\grassscripts/script1.txt
Parameter format not correct - "script1.txt".

C:\Users\Girish>ver

Microsoft Windows [Version 10.0.22621.2715]

C:\Users\Girish>

It appears to parse correctly as long as it sees backslashes, but fails at the first forward slash in the path.

regards,

After c: there should be // or \ (2 slashes) then it should work.

[hellik]

strcat(element, "/"); should be
strcat(element, HOST_DIRSEP );

Would it harm to change this, @hellik (others) ?

Should work, and save.

[neteler]

There are at least four instances to be updated then::

ag 'element, "/"'
lib/display/r_raster.c
111:            strcat(element, "/");
113:            strcat(element, "/");

lib/gis/tempfile.c
166:        strcat(element, "/");
192:        strcat(element, "/");

[hellik]

anything to do here?

[neteler]

I gave it a try but...:

We have these definitions:

grep HOST_DIRSEP include/grass/gis.h
include/grass/gis.h:#define HOST_DIRSEP '\\'
include/grass/gis.h:#define HOST_DIRSEP '/'

Next changing the code in question locally, as discussed above:

diff --git a/lib/display/r_raster.c b/lib/display/r_raster.c
index 381fbf640e..500e7dcdff 100644
--- a/lib/display/r_raster.c
+++ b/lib/display/r_raster.c
@@ -108,9 +108,9 @@ int D_open_driver(void)
             char element[GPATH_MAX];
 
             G_temp_element(element);
-            strcat(element, "/");
+            strcat(element, HOST_DIRSEP);
             strcat(element, "MONITORS");
-            strcat(element, "/");
+            strcat(element, HOST_DIRSEP);
             strcat(element, m);
             G_file_name(progname, element, "render.py", G_mapset());
         }
diff --git a/lib/gis/tempfile.c b/lib/gis/tempfile.c
index 2c20c21cce..9597646ccd 100644
--- a/lib/gis/tempfile.c
+++ b/lib/gis/tempfile.c
@@ -163,7 +163,7 @@ void G__temp_element(char *element, int tmp)
     strcpy(element, ".tmp");
     machine = G__machine_name();
     if (machine != NULL && *machine != 0) {
-        strcat(element, "/");
+        strcat(element, HOST_DIRSEP);
         strcat(element, machine);
     }
 
@@ -189,7 +189,7 @@ void G__temp_element_basedir(char *element, const char *basedir)
     strcpy(element, ".tmp");
     machine = G__machine_name();
     if (machine != NULL && *machine != 0) {
-        strcat(element, "/");
+        strcat(element, HOST_DIRSEP);
         strcat(element, machine);
     }

leads to:

make[1]: Entering directory '/home/mneteler/software/grass_main/lib/display'
gcc  -g -Wall -Wstringop-truncation -Wshadow -Wlogical-op -Werror-implicit-function-declaration -fPIC -fno-common -fno-omit-frame-pointer -fexceptions -Wextra -Wunused -Wreturn-type -Wfatal-errors -march=native -std=gnu99 -fexceptions -fstack-protector -m64 -fdiagnostics-color  -fPIC  -I/home/mneteler/software/grass_main/dist.x86_64-pc-linux-gnu/include -I/home/mneteler/software/grass_main/dist.x86_64-pc-linux-gnu/include   -DUSE_CAIRO -DPACKAGE=\""grasslibs"\"  -I../driver -I/home/mneteler/software/grass_main/dist.x86_64-pc-linux-gnu/include -I/home/mneteler/software/grass_main/dist.x86_64-pc-linux-gnu/include -DRELDIR=\"lib/display\" -o OBJ.x86_64-pc-linux-gnu/r_raster.o -c r_raster.c
In file included from r_raster.c:24:
r_raster.c: In function ‘D_open_driver’:
/home/mneteler/software/grass_main/dist.x86_64-pc-linux-gnu/include/grass/gis.h:235:21: warning: passing argument 2 of ‘strcat’ makes pointer from integer without a cast [-Wint-conversion]
  235 | #define HOST_DIRSEP '/'
      |                     ^~~
      |                     |
      |                     int
r_raster.c:111:29: note: in expansion of macro ‘HOST_DIRSEP’
  111 |             strcat(element, HOST_DIRSEP);
      |                             ^~~~~~~~~~~
In file included from r_raster.c:21:
/usr/include/string.h:149:70: note: expected ‘const char * restrict’ but argument is of type ‘int’
  149 | extern char *strcat (char *__restrict __dest, const char *__restrict __src)
      |                                               ~~~~~~~~~~~~~~~~~~~~~~~^~~~~
/home/mneteler/software/grass_main/dist.x86_64-pc-linux-gnu/include/grass/gis.h:235:21: warning: passing argument 2 of ‘strcat’ makes pointer from integer without a cast [-Wint-conversion]
  235 | #define HOST_DIRSEP '/'
      |                     ^~~
      |                     |
      |                     int
r_raster.c:113:29: note: in expansion of macro ‘HOST_DIRSEP’
  113 |             strcat(element, HOST_DIRSEP);
      |                             ^~~~~~~~~~~
/usr/include/string.h:149:70: note: expected ‘const char * restrict’ but argument is of type ‘int’
  149 | extern char *strcat (char *__restrict __dest, const char *__restrict __src)
      |                                               ~~~~~~~~~~~~~~~~~~~~~~~^~~~~
r_raster.c:111:13: warning: ‘strcat’ reading 1 or more bytes from a region of size 0 [-Wstringop-overread]
  111 |             strcat(element, HOST_DIRSEP);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: note: source object is likely at address zero
r_raster.c:113:13: warning: ‘strcat’ reading 1 or more bytes from a region of size 0 [-Wstringop-overread]
  113 |             strcat(element, HOST_DIRSEP);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: note: source object is likely at address zero

and eventually segfaults:

GRASS nc_basic_spm_grass7/user1:grass_main > g.tempfile pid=235123 --verbose
Segmentation fault (core dumped)

Apparently, not the correct way.
Does maybe @nilason have a hint how the pointer magic is done correctly?

[nilason]

Apparently, not the correct way. Does maybe @nilason have a hint how the pointer magic is done correctly?

HOST_DIRSEP is defined as char, whereas strcat() expects a string.

[neteler]

Thanks for the hint. What about this code (only showing excerpt here):

--- a/lib/gis/tempfile.c
+++ b/lib/gis/tempfile.c
@@ -159,11 +159,12 @@ void G_temp_element(char *element)
 void G__temp_element(char *element, int tmp)
 {
     const char *machine;
+    char sep[2] = {HOST_DIRSEP, '\0'};
 
     strcpy(element, ".tmp");
     machine = G__machine_name();
     if (machine != NULL && *machine != 0) {
-        strcat(element, "/");
+        strcat(element, sep);
         strcat(element, machine);
     }

it seems to work (untested for Windows as I don't have it):

GRASS nc_basic_spm_grass7/user1:grass_main > g.tempfile pid=235123 --verbose
/home/mneteler/grassdata/nc_basic_spm_grass7/user1/.tmp/caddy/235123.0

[nilason]

That looks good to me, sounds like a plan for a new PR :-).