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,