[PATCH] Dereference null return value (NULL_RETURNS) (src/backend/commands/statscmds.c)

Started by Ranier Vilelaover 5 years ago2 messages
#1Ranier Vilela
ranier.vf@gmail.com
1 attachment(s)

Hi Tom,

Per Coverity.

The SearchSysCache1 allows return NULL and at function AlterStatistics,
has one mistake, lack of, check of return, which enables a dereference NULL
pointer,
at function heap_modify_tuple.

While there is room for improvement.
Avoid calling SearchSysCache1 and table_open if the user "is not the owner
of the existing statistics object".

regards,
Ranier Vilela

Attachments:

fix_dereference_null_statscmds.patchapplication/octet-stream; name=fix_dereference_null_statscmds.patchDownload
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index 3057d89d50..6fd4bd9f6d 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -482,16 +482,19 @@ AlterStatistics(AlterStatsStmt *stmt)
 		return InvalidObjectAddress;
 	}
 
-	/* Search pg_statistic_ext */
-	rel = table_open(StatisticExtRelationId, RowExclusiveLock);
-
-	oldtup = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(stxoid));
-
 	/* Must be owner of the existing statistics object */
 	if (!pg_statistics_object_ownercheck(stxoid, GetUserId()))
 		aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_STATISTIC_EXT,
 					   NameListToString(stmt->defnames));
 
+	oldtup = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(stxoid));
+
+	if (!HeapTupleIsValid(oldtup)) /* should not happen */
+		elog(ERROR, "cache lookup failed for statistics object %u", stxoid);
+
+	/* Search pg_statistic_ext */
+	rel = table_open(StatisticExtRelationId, RowExclusiveLock);
+
 	/* Build new tuple. */
 	memset(repl_val, 0, sizeof(repl_val));
 	memset(repl_null, false, sizeof(repl_null));
#2Ranier Vilela
ranier.vf@gmail.com
In reply to: Ranier Vilela (#1)
Re: [PATCH] Dereference null return value (NULL_RETURNS) (src/backend/commands/statscmds.c)

Em ter., 25 de ago. de 2020 às 12:42, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

Hi Tom,

Per Coverity.

The SearchSysCache1 allows return NULL and at function AlterStatistics,
has one mistake, lack of, check of return, which enables a dereference
NULL pointer,
at function heap_modify_tuple.

While there is room for improvement.
Avoid calling SearchSysCache1 and table_open if the user "is not the owner
of the existing statistics object".

After a long time, finally this bug has been fixed.
https://github.com/postgres/postgres/commit/6d554e3fcd6fb8be2dbcbd3521e2947ed7a552cb

regards,
Ranier Vilela