Hackers,
I have been auditing the v12 source code for places
which inappropriately ignore the return value of a function
and have found another example which seems to me
a fertile source of future bugs.
In src/backend/nodes/list.c, list_delete_cell frees the list
and returns NIL when you delete the last element of a
list, placing a responsibility on any caller to check the
return value.
In tablecmds.c, MergeAttributes fails to do this. My
inspection of the surrounding code leads me to suspect
that logically the cell being deleted can never be the
last cell, and hence the failure to check the return value
does not manifest as a bug. But the surrounding
code is rather large and convoluted, and I have
little confidence that the code couldn't be changed such
that the return value would be NIL, possibly leading
to memory bugs.
What to do about this is harder to say. In the following
patch, I'm just doing what I think is standard for callers
of list_delete_cell, and assigning the return value back
to the list (similar to how a call to repalloc should do).
But since there is an implicit assumption that the list
is never emptied by this operation, perhaps checking
against NIL and elog'ing makes more sense?
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 602a8dbd1c..96d6833274 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2088,7 +2088,7 @@ MergeAttributes(List *schema, List *supers, char
relpersistence,
coldef->cooked_default =
restdef->cooked_default;
coldef->constraints =
restdef->constraints;
coldef->is_from_type = false;
- list_delete_cell(schema, rest, prev);
+ schema =
list_delete_cell(schema, rest, prev);
}
else
ereport(ERROR,
