r.terraflow: Handle memory reallocation errors gracefully

[ShubhamDesai]

Description
This pull request addresses two issues in r.terraflow/unionFind.h related to the use of realloc for memory reallocation. The changes ensure that memory is properly handled when realloc fails.

Issues

raster/r.terraflow/unionFind.h:129:9: error: Common realloc mistake: 'parent' nulled but not freed upon failure [memleakOnRealloc]
        parent = (T *)realloc(parent, 2 * maxsize * sizeof(T));
        ^
raster/r.terraflow/unionFind.h:133:9: error: Common realloc mistake: 'rank' nulled but not freed upon failure [memleakOnRealloc]
        rank = (T *)realloc(rank, 2 * maxsize * sizeof(T));

Changes Made

• Added temporary variables to store the results of realloc for parent and rank.
• Included checks to ensure that realloc succeeded before updating the original pointers.
• Implemented error handling to free the original memory if realloc fails.

[nilason]

Bear in mind this header file unionFind.h is a C++ file. This is how I would implement this (diff against main):

diff --git a/raster/r.terraflow/unionFind.h b/raster/r.terraflow/unionFind.h
index 698dcfc0c9..fdd084b4ae 100644
--- a/raster/r.terraflow/unionFind.h
+++ b/raster/r.terraflow/unionFind.h
@@ -20,10 +20,14 @@
 #define __UNION_FIND
 
 #include <assert.h>
-#include <stdlib.h>
-#include <string.h>
+#include <cstdlib>
+#include <cstring>
 #include <iostream>
 
+extern "C" {
+#include <grass/glocale.h>
+}
+
 /* initial range guesstimate */
 #define UNION_INITIAL_SIZE 2000
 
@@ -126,13 +130,21 @@ inline void unionFind<T>::makeSet(T x)
     if (x >= maxsize) {
         /* reallocate parent */
         cout << "UnionFind::makeSet: reallocate double " << maxsize << "\n";
-        parent = (T *)realloc(parent, 2 * maxsize * sizeof(T));
-        assert(parent);
-        memset(parent + maxsize, 0, maxsize * sizeof(T));
-        /*reallocate rank */
-        rank = (T *)realloc(rank, 2 * maxsize * sizeof(T));
-        assert(rank);
-        memset(rank + maxsize, 0, maxsize * sizeof(T));
+        if (void *new_parent = std::realloc(parent, 2 * maxsize * sizeof(T))) {
+            parent = static_cast<T *>(new_parent);
+            std::memset(parent + maxsize, 0, maxsize * sizeof(T));
+        }
+        else {
+            G_fatal_error(_("Not enough memory for %s"), "parent");
+        }
+        /* reallocate rank */
+        if (void *new_rank = std::realloc(rank, 2 * maxsize * sizeof(T))) {
+            rank = static_cast<T *>(new_rank);
+            std::memset(rank + maxsize, 0, maxsize * sizeof(T));
+        }
+        else {
+            G_fatal_error(_("Not enough memory for %s"), "rank");
+        }
         /*update maxsize */
         maxsize *= 2;
     }

Some notes why:

• #include <cstdlib> style include is preferred to#include <stdlib.h>
• C-style casting should be avoided, hence the static_cast
• G_fatal_error() is the GRASS way to "exit(EXIT_FAILURE)" with a message
• a string surrounded with _() will produce a localised string (with gettext)

[marisn]

a string surrounded with _() will produce a localised string (with gettext)

With the latest change to Weblate, I do not know how string extraction is done, but at least in our Make file strings from headers are not extracted

grass/locale/Makefile

Line 34 in 559e088

LIB_POTFILES = find ../lib \( -name "*.c" -o -name "*.py" \) | xargs grep -l "_(\"\|n_(\""

[nilason]

a string surrounded with _() will produce a localised string (with gettext)

With the latest change to Weblate, I do not know how string extraction is done, but at least in our Make file strings from headers are not extracted

grass/locale/Makefile

Line 34 in 559e088

LIB_POTFILES = find ../lib \( -name "*.c" -o -name "*.py" \) | xargs grep -l "_(\"\|n_(\""

Good point, but that is a completely separate issue.

[neteler]

(for the record: https://github.com/OSGeo/grass/blob/main/doc/infrastructure.md#user-message-translation-management-i18n )

[echoix]

I made another issue to track string extraction in header files, so this PR can continue

[ShubhamDesai]

Bear in mind this header file unionFind.h is a C++ file. This is how I would implement this (diff against main):

diff --git a/raster/r.terraflow/unionFind.h b/raster/r.terraflow/unionFind.h
index 698dcfc0c9..fdd084b4ae 100644
--- a/raster/r.terraflow/unionFind.h
+++ b/raster/r.terraflow/unionFind.h
@@ -20,10 +20,14 @@
 #define __UNION_FIND
 
 #include <assert.h>
-#include <stdlib.h>
-#include <string.h>
+#include <cstdlib>
+#include <cstring>
 #include <iostream>
 
+extern "C" {
+#include <grass/glocale.h>
+}
+
 /* initial range guesstimate */
 #define UNION_INITIAL_SIZE 2000
 
@@ -126,13 +130,21 @@ inline void unionFind<T>::makeSet(T x)
     if (x >= maxsize) {
         /* reallocate parent */
         cout << "UnionFind::makeSet: reallocate double " << maxsize << "\n";
-        parent = (T *)realloc(parent, 2 * maxsize * sizeof(T));
-        assert(parent);
-        memset(parent + maxsize, 0, maxsize * sizeof(T));
-        /*reallocate rank */
-        rank = (T *)realloc(rank, 2 * maxsize * sizeof(T));
-        assert(rank);
-        memset(rank + maxsize, 0, maxsize * sizeof(T));
+        if (void *new_parent = std::realloc(parent, 2 * maxsize * sizeof(T))) {
+            parent = static_cast<T *>(new_parent);
+            std::memset(parent + maxsize, 0, maxsize * sizeof(T));
+        }
+        else {
+            G_fatal_error(_("Not enough memory for %s"), "parent");
+        }
+        /* reallocate rank */
+        if (void *new_rank = std::realloc(rank, 2 * maxsize * sizeof(T))) {
+            rank = static_cast<T *>(new_rank);
+            std::memset(rank + maxsize, 0, maxsize * sizeof(T));
+        }
+        else {
+            G_fatal_error(_("Not enough memory for %s"), "rank");
+        }
         /*update maxsize */
         maxsize *= 2;
     }

Some notes why:

• #include <cstdlib> style include is preferred to#include <stdlib.h>
• C-style casting should be avoided, hence the static_cast
• G_fatal_error() is the GRASS way to "exit(EXIT_FAILURE)" with a message
• a string surrounded with _() will produce a localised string (with gettext)

Doing these changes leads to following issue
unionFind.h:39:1: error: Code 'template<...' is invalid C code. Use --std or --language to configure the language. [syntaxError]
template

I think it is treating the file as C code instead of C++

[echoix]

Who gives that error? Our compilation or your tool? It's quite possible that you'd have to configure your tool properly

[ShubhamDesai]

Who gives that error? Our compilation or your tool? It's quite possible that you'd have to configure your tool properly

yeah mostly i think this issue is at my end since i don't have the setup.
But just wanted to confirm about it so that i will do the changes.
Because until now i used to fix small issues.

[nilason]

Doing these changes leads to following issue unionFind.h:39:1: error: Code 'template<...' is invalid C code. Use --std or --language to configure the language. [syntaxError] template

I think it is treating the file as C code instead of C++

Do you set CXX configure flag?
What does your configure log say regarding C++ compiler? See e.g.:

GRASS is now configured for:  x86_64-pc-linux-gnu

  Source directory:           /home/runner/work/grass/grass
  Build directory:            /home/runner/work/grass/grass
  Installation directory:     ${prefix}/grass85
  Startup script in directory:${exec_prefix}/bin
  C compiler:                 gcc -std=gnu17 -fPIC -Wall -Wextra 
  C++ compiler:               g++ -std=c++17 -fPIC -Wall -Wextra

It is more informative if you copy and post the whole log text part related to the build error, where we can see exactly what command resulted in the error.

[wenzeslaus]

I'm confused here. Is the issue discussed local or is adding the C calls to C++ header?

Generally speaking, while this is C++ header file, there is no reason not to do the normal thing we do elsewhere, i.e., G_fatal_error which is how these are handled elsewhere.

[nilason]

I'm confused here. Is the issue discussed local or is adding the C calls to C++ header?

Generally speaking, while this is C++ header file, there is no reason not to do the normal thing we do elsewhere, i.e., G_fatal_error which is how these are handled elsewhere.

My suggested solution #3970 (comment) works just fine. @ShubhamDesai updated this PR with only a part of that.

[ShubhamDesai]

Doing these changes leads to following issue unionFind.h:39:1: error: Code 'template<...' is invalid C code. Use --std or --language to configure the language. [syntaxError] template
I think it is treating the file as C code instead of C++

Do you set CXX configure flag? What does your configure log say regarding C++ compiler? See e.g.:

GRASS is now configured for:  x86_64-pc-linux-gnu

  Source directory:           /home/runner/work/grass/grass
  Build directory:            /home/runner/work/grass/grass
  Installation directory:     ${prefix}/grass85
  Startup script in directory:${exec_prefix}/bin
  C compiler:                 gcc -std=gnu17 -fPIC -Wall -Wextra 
  C++ compiler:               g++ -std=c++17 -fPIC -Wall -Wextra

It is more informative if you copy and post the whole log text part related to the build error, where we can see exactly what command resulted in the error.

Previously i was using a remote machine where i did not had the entire setup for compilation.
But now I have the setup with WSL just trying fix the new line character issue.
Once it is done i will compile it and see.
Correct me if I am wrong i need to use this three commands for compilation
make clean
./configure
make
inside that particular directory right

Also one more doubt I have since I am exploring this grass GIS
what are the commands that I should use so that the print statements get displayed It will help me for debugging. Is it like the make command and the G_fatal or G_warning or printf gets displayed in software or in cmd.

[nilason]

Previously i was using a remote machine where i did not had the entire setup for compilation. But now I have the setup with WSL just trying fix the new line character issue. Once it is done i will compile it and see. Correct me if I am wrong i need to use this three commands for compilation
make clean
./configure
make
Inside that particular directory right

In root you should make distclean to clean up everything (that is the safest), followed by ./configure and make.
If you want to recompile a subdirectory make clean is usually enough.

./configure --help will give you configure options.

Also one more doubt I have since I am exploring this grass GIS what are the commands that I should use so that the print statements get displayed It will help me for debugging. Is it like the make command and the G_fatal or G_warning or printf gets displayed in software or in cmd.

You can start GRASS without installing it, in root: ./bin.aarch64-apple-darwin23.5.0/grass (bin.* dir name may vary), once there you may set eg. g.gisenv set="DEBUG=1" for debug messages (see GRASS_Debugging).

[nilason]

See also https://grasswiki.osgeo.org/wiki/Compile_and_Install

[ShubhamDesai]

I'm confused here. Is the issue discussed local or is adding the C calls to C++ header?
Generally speaking, while this is C++ header file, there is no reason not to do the normal thing we do elsewhere, i.e., G_fatal_error which is how these are handled elsewhere.

My suggested solution #3970 (comment) works just fine. @ShubhamDesai updated this PR with only a part of that.

The changes you suggested I tried and the build was successful. The issue also got fixed.

Also with cppcheck version 2.14.2 two more issues of static style casting got highlighted.
So fixed both of them as well.
Following were the issues:
`unionFind.h:75:14: style: C-style pointer casting [cstyleCast]
parent = (T *)calloc(maxsize, sizeof(T));

unionFind.h:78:12: style: C-style pointer casting [cstyleCast]
rank = (T *)calloc(maxsize, sizeof(T)); `

[nilason]

I'm confused here. Is the issue discussed local or is adding the C calls to C++ header?
Generally speaking, while this is C++ header file, there is no reason not to do the normal thing we do elsewhere, i.e., G_fatal_error which is how these are handled elsewhere.

My suggested solution #3970 (comment) works just fine. @ShubhamDesai updated this PR with only a part of that.

The changes you suggested I tried and the build was successful. The issue also got fixed.

Also with cppcheck version 2.14.2 two more issues of static style casting got highlighted. So fixed both of them as well. Following were the issues: `unionFind.h:75:14: style: C-style pointer casting [cstyleCast] parent = (T *)calloc(maxsize, sizeof(T));

unionFind.h:78:12: style: C-style pointer casting [cstyleCast] rank = (T *)calloc(maxsize, sizeof(T)); `

Good! The new issue you found is closely related to the initial issue you addressed so this is perfectly appropriate. You should however replace the assert() with a similar solution to handle error as the previous ones.

[ShubhamDesai]

I'm confused here. Is the issue discussed local or is adding the C calls to C++ header?
Generally speaking, while this is C++ header file, there is no reason not to do the normal thing we do elsewhere, i.e., G_fatal_error which is how these are handled elsewhere.

My suggested solution #3970 (comment) works just fine. @ShubhamDesai updated this PR with only a part of that.

The changes you suggested I tried and the build was successful. The issue also got fixed.
Also with cppcheck version 2.14.2 two more issues of static style casting got highlighted. So fixed both of them as well. Following were the issues: unionFind.h:75:14: style: C-style pointer casting [cstyleCast] parent = (T *)calloc(maxsize, sizeof(T)); unionFind.h:78:12: style: C-style pointer casting [cstyleCast] rank = (T *)calloc(maxsize, sizeof(T));

Good! The new issue you found is closely related to the initial issue you addressed so this is perfectly appropriate. You should however replace the assert() with a similar solution to handle error as the previous ones.

Replaced all the assert statements and remove assert.h header as well.
Also just one question will it be a good practice to replace assert with grass statements wherever possible in project

[nilason]

This looks good to me now, thanks!