Fix issue 2093: pfree() called with a NULL pointer (#2095) (#2103)

Fixed issue 2093 where pfree() was called with a NULL pointer. The issue is due to some confusion with pfree(). There are 2 definitions for it, one that checks for a passed NULL and the other which does not. Created a function, pfree_if_not_null(), to check for NULL and call pfree() if a NULL wasn't passed. Modified the pfree references in the following files - src/backend/commands/label_commands.c src/backend/executor/cypher_merge.c src/backend/executor/cypher_set.c src/backend/parser/ag_scanner.l src/backend/parser/cypher_analyze.c src/backend/parser/cypher_expr.c src/backend/parser/cypher_gram.y src/backend/parser/cypher_parse_agg.c src/backend/utils/adt/age_global_graph.c src/backend/utils/adt/age_graphid_ds.c src/backend/utils/adt/age_session_info.c src/backend/utils/adt/age_vle.c src/backend/utils/adt/agtype.c src/backend/utils/adt/agtype_gin.c src/backend/utils/adt/agtype_raw.c src/backend/utils/adt/agtype_util.c src/backend/utils/load/ag_load_edges.c src/backend/utils/load/ag_load_labels.c src/include/utils/age_graphid_ds.h src/include/utils/age_session_info.h src/include/utils/agtype.h Added regression tests for the original issue.
apache · Sep 11, 2024 · 71dee0d · 71dee0d
1 parent c454503
commit 71dee0d
Show file tree

Hide file tree

Showing 23 changed files with 141 additions and 100 deletions.
diff --git a/regress/expected/expr.out b/regress/expected/expr.out
@@ -8715,6 +8715,21 @@ SELECT * FROM cypher('fuzzystrmatch', $$ RETURN difference("hello world!", "hell
 ERROR:  extension fuzzystrmatch is not installed for function difference
 LINE 1: SELECT * FROM cypher('fuzzystrmatch', $$ RETURN difference("...
                                                ^
+--
+-- Issue 2093: Server crashes when executing SELECT agtype_hash_cmp(agtype_in('[null, null, null, null, null]'));
+--
+SELECT agtype_access_operator(agtype_in('[null, null]'));
+ agtype_access_operator 
+------------------------
+
+(1 row)
+
+SELECT agtype_hash_cmp(agtype_in('[null, null, null, null, null]'));
+ agtype_hash_cmp 
+-----------------
+      -505290721
+(1 row)
+
 --
 -- Cleanup
 --

diff --git a/regress/sql/expr.sql b/regress/sql/expr.sql
@@ -3517,6 +3517,12 @@ SELECT * FROM create_graph('fuzzystrmatch');
 SELECT * FROM cypher('fuzzystrmatch', $$ RETURN soundex("hello world!") $$) AS (result agtype);
 SELECT * FROM cypher('fuzzystrmatch', $$ RETURN difference("hello world!", "hello world!") $$) AS (result agtype);
 
+--
+-- Issue 2093: Server crashes when executing SELECT agtype_hash_cmp(agtype_in('[null, null, null, null, null]'));
+--
+SELECT agtype_access_operator(agtype_in('[null, null]'));
+SELECT agtype_hash_cmp(agtype_in('[null, null, null, null, null]'));
+
 --
 -- Cleanup
 --

diff --git a/src/backend/commands/label_commands.c b/src/backend/commands/label_commands.c
@@ -131,7 +131,7 @@ Datum age_is_valid_label_name(PG_FUNCTION_ARGS)
                           agtv_value->val.string.len);
 
     is_valid = is_valid_label_name(label_name, 0);
-    pfree(label_name);
+    pfree_if_not_null(label_name);
 
     if (is_valid)
     {

diff --git a/src/backend/executor/cypher_merge.c b/src/backend/executor/cypher_merge.c
@@ -346,7 +346,7 @@ static void free_path_entry_array(path_entry **path_array, int length)
 
     for (index = 0; index < length; index++)
     {
-        pfree(path_array[index]);
+        pfree_if_not_null(path_array[index]);
     }
 }
 
@@ -892,10 +892,10 @@ static void end_cypher_merge(CustomScanState *node)
         free_path_entry_array(entry, path_length);
 
         /* free up the array container */
-        pfree(entry);
+        pfree_if_not_null(entry);
 
         /* free up the created_path container */
-        pfree(css->created_paths_list);
+        pfree_if_not_null(css->created_paths_list);
 
         css->created_paths_list = next;
     }

diff --git a/src/backend/executor/cypher_set.c b/src/backend/executor/cypher_set.c
@@ -595,7 +595,7 @@ static void process_update_list(CustomScanState *node)
         lidx++;
     }
     /* free our lookup array */
-    pfree(luindex);
+    pfree_if_not_null(luindex);
 }
 
 static TupleTableSlot *exec_cypher_set(CustomScanState *node)

diff --git a/src/backend/parser/ag_scanner.l b/src/backend/parser/ag_scanner.l
@@ -34,6 +34,7 @@
 #include "mb/pg_wchar.h"
 
 #include "parser/ag_scanner.h"
+#include "utils/agtype.h"
 }
 
 %option 8bit
@@ -794,7 +795,7 @@ void *ag_yyrealloc(void *ptr, yy_size_t size, yyscan_t yyscanner)
     {
         if (size == 0)
         {
-            pfree(ptr);
+            pfree_if_not_null(ptr);
             return NULL;
         }
         else
@@ -811,7 +812,7 @@ void *ag_yyrealloc(void *ptr, yy_size_t size, yyscan_t yyscanner)
 void ag_yyfree(void *ptr, yyscan_t yyscanner)
 {
     if (ptr)
-        pfree(ptr);
+        pfree_if_not_null(ptr);
 }
 
 static void strbuf_init(strbuf *sb, int capacity)
@@ -824,7 +825,7 @@ static void strbuf_init(strbuf *sb, int capacity)
 static void strbuf_cleanup(strbuf *sb)
 {
     if (sb->buffer)
-        pfree(sb->buffer);
+        pfree_if_not_null(sb->buffer);
 }
 
 static void strbuf_append_buf(strbuf *sb, const char *b, const int len)
@@ -1119,8 +1120,8 @@ static void _numstr_to_decimal(const char *numstr, const int base, strbuf *sb)
         strbuf_append_buf(sb, &buf[buf_i], NDIGITS_PER_REMAINDER - buf_i);
     }
 
-    pfree(remainders);
-    pfree(words);
+    pfree_if_not_null(remainders);
+    pfree_if_not_null(words);
 }
 
 static uint32 hexdigit_value(const char c)

diff --git a/src/backend/parser/cypher_analyze.c b/src/backend/parser/cypher_analyze.c
@@ -113,7 +113,7 @@ static void post_parse_analyze(ParseState *pstate, Query *query)
         }
 
         /* reset extra_node */
-        pfree(extra_node);
+        pfree_if_not_null(extra_node);
         extra_node = NULL;
     }
 }
@@ -303,7 +303,7 @@ static void build_explain_query(Query *query, Node *explain_node)
     ((ExplainStmt *)explain_node)->options = NULL;
 
     /* we need to free query_node as it is no longer needed */
-    pfree(query_node);
+    pfree_if_not_null(query_node);
 }
 
 static bool is_rte_cypher(RangeTblEntry *rte)

diff --git a/src/backend/parser/cypher_expr.c b/src/backend/parser/cypher_expr.c
@@ -1700,7 +1700,7 @@ static List *cast_agtype_input_to_other_type(cypher_parsestate *cpstate,
             }
 
             /* free the old args and replace them with the new ones */
-            pfree(targs);
+            pfree_if_not_null(targs);
             targs = new_targs;
             break;
         }
@@ -1755,7 +1755,7 @@ static void check_for_extension_functions(char *extension, FuncCall *fn)
             if (procform->pronamespace == oid &&
                 isTempNamespace(procform->pronamespace) == false)
             {
-                pfree(asp);
+                pfree_if_not_null(asp);
                 found = true;
                 break;
             }
@@ -1766,7 +1766,7 @@ static void check_for_extension_functions(char *extension, FuncCall *fn)
             break;
         }
 
-        pfree(asp);
+        pfree_if_not_null(asp);
     }
 
     /* if we didn't find it, it isn't in the search path */

diff --git a/src/backend/parser/cypher_gram.y b/src/backend/parser/cypher_gram.y
@@ -26,6 +26,7 @@
 #include "parser/cypher_gram.h"
 #include "parser/cypher_parse_node.h"
 #include "parser/scansup.h"
+#include "utils/agtype.h"
 
 /* override the default action for locations */
 #define YYLLOC_DEFAULT(current, rhs, n) \
@@ -2929,7 +2930,7 @@ static char *create_unique_name(char *prefix_name)
     /* if we created the prefix, we need to free it */
     if (prefix_name == NULL || strlen(prefix_name) <= 0)
     {
-        pfree(prefix);
+        pfree_if_not_null(prefix);
     }
 
     return name;

diff --git a/src/backend/parser/cypher_parse_agg.c b/src/backend/parser/cypher_parse_agg.c
@@ -31,6 +31,7 @@
 #include "parser/cypher_parse_agg.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteManip.h"
+#include "utils/agtype.h"
 
 typedef struct
 {
@@ -807,7 +808,7 @@ static List * expand_grouping_sets(List *groupingSets, int limit)
         result = lappend(result, list_union_int(NIL, (List *) lfirst(lc)));
     }
 
-    for_each_cell(lc, expanded_groups, 
+    for_each_cell(lc, expanded_groups,
                   lnext(expanded_groups, list_head(expanded_groups)))
     {
         List *p = lfirst(lc);
@@ -847,7 +848,7 @@ static List * expand_grouping_sets(List *groupingSets, int limit)
         while (result_len-- > 0)
             result = lappend(result, *ptr++);
 
-        pfree(buf);
+        pfree_if_not_null(buf);
     }
 
     return result;

diff --git a/src/backend/utils/adt/age_global_graph.c b/src/backend/utils/adt/age_global_graph.c
@@ -182,7 +182,7 @@ static void create_GRAPH_global_hashtables(GRAPH_global_context *ggctx)
     ggctx->vertex_hashtable = hash_create(vhn, VERTEX_HTAB_INITIAL_SIZE,
                                           &vertex_ctl,
                                           HASH_ELEM | HASH_FUNCTION);
-    pfree(vhn);
+    pfree_if_not_null(vhn);
 
     /* initialize the edge hashtable */
     MemSet(&edge_ctl, 0, sizeof(edge_ctl));
@@ -191,7 +191,7 @@ static void create_GRAPH_global_hashtables(GRAPH_global_context *ggctx)
     edge_ctl.hash = tag_hash;
     ggctx->edge_hashtable = hash_create(ehn, EDGE_HTAB_INITIAL_SIZE, &edge_ctl,
                                         HASH_ELEM | HASH_FUNCTION);
-    pfree(ehn);
+    pfree_if_not_null(ehn);
 }
 
 /* helper function to get a List of all label names for the specified graph */
@@ -709,7 +709,7 @@ static bool free_specific_GRAPH_global_context(GRAPH_global_context *ggctx)
     }
 
     /* free the graph name */
-    pfree(ggctx->graph_name);
+    pfree_if_not_null(ggctx->graph_name);
     ggctx->graph_name = NULL;
 
     ggctx->graph_oid = InvalidOid;
@@ -741,7 +741,7 @@ static bool free_specific_GRAPH_global_context(GRAPH_global_context *ggctx)
         }
 
         /* free the vertex's datumCopy properties */
-        pfree(DatumGetPointer(value->vertex_properties));
+        pfree_if_not_null(DatumGetPointer(value->vertex_properties));
         value->vertex_properties = 0;
 
         /* free the edge list associated with this vertex */
@@ -783,7 +783,7 @@ static bool free_specific_GRAPH_global_context(GRAPH_global_context *ggctx)
         }
 
         /* free the edge's datumCopy properties */
-        pfree(DatumGetPointer(value->edge_properties));
+        pfree_if_not_null(DatumGetPointer(value->edge_properties));
         value->edge_properties = 0;
 
         /* move to the next edge */
@@ -806,7 +806,7 @@ static bool free_specific_GRAPH_global_context(GRAPH_global_context *ggctx)
     ggctx->edge_hashtable = NULL;
 
     /* free the context */
-    pfree(ggctx);
+    pfree_if_not_null(ggctx);
     ggctx = NULL;
 
     return true;
@@ -1309,7 +1309,7 @@ Datum age_vertex_stats(PG_FUNCTION_ARGS)
     ggctx = manage_GRAPH_global_contexts(graph_name, graph_oid);
 
     /* free the graph name */
-    pfree(graph_name);
+    pfree_if_not_null(graph_name);
 
     /* get the id */
     agtv_temp = GET_AGTYPE_VALUE_OBJECT_VALUE(agtv_vertex, "id");
@@ -1415,7 +1415,7 @@ Datum age_graph_stats(PG_FUNCTION_ARGS)
     ggctx = manage_GRAPH_global_contexts(graph_name, graph_oid);
 
     /* free the graph name */
-    pfree(graph_name);
+    pfree_if_not_null(graph_name);
 
     /* zero the state */
     memset(&result, 0, sizeof(agtype_in_state));

diff --git a/src/backend/utils/adt/age_graphid_ds.c b/src/backend/utils/adt/age_graphid_ds.c
@@ -144,14 +144,14 @@ void free_ListGraphId(ListGraphId *container)
     {
         next_node = curr_node->next;
         /* we can do this because this is just a list of ints */
-        pfree(curr_node);
+        pfree_if_not_null(curr_node);
         container->size--;
         curr_node = next_node;
     }
 
     Assert(container->size == 0);
     /* free the container */
-    pfree(container);
+    pfree_if_not_null(container);
 }
 
 /* helper function to create a new, empty, graphid stack */
@@ -188,7 +188,7 @@ void free_graphid_stack(ListGraphId *stack)
         GraphIdNode *next = stack->head->next;
 
         /* free the head element */
-        pfree(stack->head);
+        pfree_if_not_null(stack->head);
         /* move the head to the next */
         stack->head = next;
     }
@@ -253,7 +253,7 @@ graphid pop_graphid_stack(ListGraphId *stack)
     stack->head = stack->head->next;
     stack->size--;
     /* free the element */
-    pfree(node);
+    pfree_if_not_null(node);
 
     /* return the id */
     return id;

diff --git a/src/backend/utils/adt/age_session_info.c b/src/backend/utils/adt/age_session_info.c
@@ -125,12 +125,12 @@ void reset_session_info(void)
     {
         if (session_info_graph_name != NULL)
         {
-            pfree(session_info_graph_name);
+            pfree_if_not_null(session_info_graph_name);
         }
 
         if (session_info_cypher_statement != NULL)
         {
-            pfree(session_info_cypher_statement);
+            pfree_if_not_null(session_info_cypher_statement);
         }
     }
 

diff --git a/src/backend/utils/adt/age_vle.c b/src/backend/utils/adt/age_vle.c
@@ -320,7 +320,7 @@ static void create_VLE_local_state_hashtable(VLE_local_context *vlelctx)
                                                 EDGE_STATE_HTAB_INITIAL_SIZE,
                                                 &edge_state_ctl,
                                                 HASH_ELEM | HASH_FUNCTION);
-    pfree(eshn);
+    pfree_if_not_null(eshn);
 }
 
 /*
@@ -434,14 +434,14 @@ static void free_VLE_local_context(VLE_local_context *vlelctx)
     /* free the stored graph name */
     if (vlelctx->graph_name != NULL)
     {
-        pfree(vlelctx->graph_name);
+        pfree_if_not_null(vlelctx->graph_name);
         vlelctx->graph_name = NULL;
     }
 
     /* free the stored edge label name */
     if (vlelctx->edge_label_name != NULL)
     {
-        pfree(vlelctx->edge_label_name);
+        pfree_if_not_null(vlelctx->edge_label_name);
         vlelctx->edge_label_name = NULL;
     }
 
@@ -463,15 +463,15 @@ static void free_VLE_local_context(VLE_local_context *vlelctx)
     }
 
     /* free the containers */
-    pfree(vlelctx->dfs_vertex_stack);
-    pfree(vlelctx->dfs_edge_stack);
-    pfree(vlelctx->dfs_path_stack);
+    pfree_if_not_null(vlelctx->dfs_vertex_stack);
+    pfree_if_not_null(vlelctx->dfs_edge_stack);
+    pfree_if_not_null(vlelctx->dfs_path_stack);
     vlelctx->dfs_vertex_stack = NULL;
     vlelctx->dfs_edge_stack = NULL;
     vlelctx->dfs_path_stack = NULL;
 
     /* and finally the context itself */
-    pfree(vlelctx);
+    pfree_if_not_null(vlelctx);
     vlelctx = NULL;
 }