lib/raster: Add larger buffers, error messages to reclass header reader (OSGeo#6341)

This increases the size of buffer the reclass header reader code (reading header of the header file) uses for reading raster and mapset names. It also adds an additional check for reading the whole line and errors if not read. A message can now be optionally reported for each of the already handled error states.

The maximum length of files is now checked by a test which fails with the original buffer size combined with both the original code and the new error handling code, but the individual error states are not tested.

Using 50 as the name length on Windows to avoid 'MAPSET m... not found at' error from r.mapcalc call which occurs with length 255.

Author: wenzeslaus

lib/raster/reclass.c

@@ -19,7 +19,7 @@
 #include <grass/glocale.h>
 
 static const char NULL_STRING[] = "null";
-static int reclass_type(FILE *, char **, char **);
+static int reclass_type(FILE *, char **, char **, char **);
 static FILE *fopen_cellhd_old(const char *, const char *);
 static FILE *fopen_cellhd_new(const char *);
 static int get_reclass_table(FILE *, struct Reclass *, char **);
@@ -51,7 +51,7 @@ int Rast_is_reclass(const char *name, const char *mapset, char *rname,
     if (fd == NULL)
         return -1;
 
-    type = reclass_type(fd, &rname, &rmapset);
+    type = reclass_type(fd, &rname, &rmapset, NULL);
     fclose(fd);
     if (type < 0)
         return -1;
@@ -135,8 +135,7 @@ int Rast_is_reclassed_to(const char *name, const char *mapset, int *nrmaps,
    \param mapset mapset name
    \param[out] reclass pointer to Reclass structure
 
-   \return -1 on error
-   \return type code
+   \return type code (>=1), 0 if no reclass, -1 on error
  */
 int Rast_get_reclass(const char *name, const char *mapset,
                      struct Reclass *reclass)
@@ -149,13 +148,24 @@ int Rast_get_reclass(const char *name, const char *mapset,
         return -1;
     reclass->name = NULL;
     reclass->mapset = NULL;
-    reclass->type = reclass_type(fd, &reclass->name, &reclass->mapset);
-    if (reclass->type <= 0) {
+    char *error_message = NULL;
+    reclass->type =
+        reclass_type(fd, &reclass->name, &reclass->mapset, &error_message);
+    if (reclass->type == 0) {
+        // no reclass
         fclose(fd);
         return reclass->type;
     }
+    if (reclass->type < 0) {
+        // error
+        fclose(fd);
+        G_warning(_("Error reading beginning of header file for <%s@%s>: %s"),
+                  name, mapset, error_message);
+        if (error_message != NULL)
+            G_free(error_message);
+        return reclass->type;
+    }
 
-    char *error_message = NULL;
     switch (reclass->type) {
     case RECLASS_TABLE:
         stat = get_reclass_table(fd, reclass, &error_message);
@@ -204,10 +214,22 @@ void Rast_free_reclass(struct Reclass *reclass)
     }
 }
 
-static int reclass_type(FILE *fd, char **rname, char **rmapset)
+/**
+ * \brief Get reclass type if it is a reclass file
+ *
+ * \param fd[in] file descriptor
+ * \param rname[out] name of the reclass from raster
+ * \param rmapset[out] name of the mapset of the raster
+ * \param error_message[out] will be assigned a newly error message if not NULL
+ *
+ * \returns RECLASS_TABLE if reclass, 0 if not, -1 on error
+ */
+static int reclass_type(FILE *fd, char **rname, char **rmapset,
+                        char **error_message)
 {
-    char buf[128];
-    char label[128], arg[128];
+    char
+        buf[GNAME_MAX + 128 + 1]; // name or mapset plus the label and separator
+    char label[128], arg[GNAME_MAX];
     int i;
     int type;
 
@@ -225,10 +247,26 @@ static int reclass_type(FILE *fd, char **rname, char **rmapset)
     if (*rmapset)
         **rmapset = '\0';
     for (i = 0; i < 2; i++) {
-        if (fgets(buf, sizeof buf, fd) == NULL)
+        if (fgets(buf, sizeof buf, fd) == NULL) {
+            if (error_message != NULL) {
+                G_asprintf(error_message, _("File too short, reading line %d"),
+                           i + 1);
+            }
+            return -1;
+        }
+        if (buf[strlen(buf) - 1] != '\n') {
+            if (error_message != NULL) {
+                G_asprintf(error_message, _("Line too long: %s..."), buf);
+            }
             return -1;
-        if (sscanf(buf, "%[^:]:%s", label, arg) != 2)
+        }
+        if (sscanf(buf, "%[^:]:%s", label, arg) != 2) {
+            if (error_message != NULL) {
+                G_asprintf(error_message, _("Format is not key:value: %s"),
+                           buf);
+            }
             return -1;
+        }
         if (strncmp(label, "maps", 4) == 0) {
             if (*rmapset)
                 strcpy(*rmapset, arg);
@@ -241,13 +279,30 @@ static int reclass_type(FILE *fd, char **rname, char **rmapset)
             else
                 *rname = G_store(arg);
         }
-        else
+        else {
+            if (error_message != NULL) {
+                G_asprintf(error_message, _("Unknown key at line: %s"), buf);
+            }
             return -1;
+        }
     }
     if (**rmapset && **rname)
         return type;
-    else
+    else {
+        // If they do not occur in the two lines we expect them.
+        if (**rname && error_message != NULL) {
+            G_asprintf(error_message,
+                       _("Mapset not read, only raster name: %s"), *rname);
+        }
+        else if (**rmapset && error_message != NULL) {
+            G_asprintf(error_message,
+                       _("Raster name not read, only mapset: %s"), *rmapset);
+        }
+        else if (error_message != NULL) {
+            *error_message = G_store(_("Raster name and mapset not read"));
+        }
         return -1;
+    }
 }
 
 static FILE *fopen_cellhd_old(const char *name, const char *mapset)

lib/raster/tests/lib_raster_reclass_test.py

@@ -1,10 +1,13 @@
+import os
 from pathlib import Path
+from io import StringIO
 
 import pytest
 
 from grass.script import MaskManager
 from grass.tools import Tools
 from grass.exceptions import CalledModuleError
+from grass.experimental import MapsetSession
 
 
 def test_reclass_as_mask_correct_state(session):
@@ -66,3 +69,45 @@ def test_reclass_as_mask_intermixed_hash_value_line(session):
         cellhd_file.write_text(content, encoding="utf-8")
         with pytest.raises(CalledModuleError, match=r"(?s)not read yet.*#1"):
             tools.r_univar(map="data")
+
+
+def get_filename_length_limit(path):
+    """Get maximum filename length for the given path"""
+    try:
+        # Expected to work on unix
+        return os.pathconf(path, "PC_NAME_MAX")
+    except (AttributeError, ValueError, OSError):
+        # On Windows, there is no os.pathconf
+        # and the other exceptions are from function doc.
+        # Getting actual value for Windows would be complicated,
+        # so we go with a low value rather than skipping the test.
+        return 50
+
+
+def test_long_names(session, tmp_path):
+    """Check that long names are handled correctly"""
+    # Using 255 because that's GNAME_MAX is 256 minus the terminator.
+    name_size = min(get_filename_length_limit(tmp_path), 255)
+    mapset_name = "m" * name_size
+    long_name = "l" * name_size
+    name = "r" * name_size
+    with (
+        MapsetSession(name=mapset_name, create=True, env=session.env) as mapset_session,
+        Tools(session=mapset_session) as tools,
+    ):
+        # This one is the one to actually break when the names are too long for the
+        # internal buffer - the raster being used in r.reclass has a long name.
+        tools.r_mapcalc(expression=f"{long_name} = raster_mask")
+        tools.r_reclass(input=long_name, output=name, rules=StringIO("1 = 5\n"))
+        tools.r_univar(map=name)
+
+        tools.r_reclass(input=name, output="short_name", rules=StringIO("1 = 5\n"))
+        tools.r_univar(map="short_name")
+
+        with MaskManager(env=mapset_session.env) as mask:
+            tools.r_mask(raster=long_name, env=mask.env)
+            tools.r_univar(map=name, env=mask.env)
+
+        with MaskManager(env=mapset_session.env, mask_name="n" * name_size) as mask:
+            tools.r_mask(raster=long_name, env=mask.env)
+            tools.r_univar(map=name, env=mask.env)