Possible NULL dereferencing (src/backend/tcop/pquery.c)

Started by Ranier Vilelaover 5 years ago3 messages

ranier.vf@gmail.com

over 5 years ago

1 attachment(s)

Hi,

Per Coverity.
Perhaps it is excessive caution.
Probably assertion check has already caught all possible errors.
But, redundancy may not cost as much and is worth it.

1.Assertion check
/* Caller messed up if we have neither a ready query nor held data. */
Assert(queryDesc || portal->holdStore);

But in release, if QueryDesc is NULL and portal->holdStore is NULL too,
when Call PushActiveSnapshot *deference* NULL check can happen.

2. if (portal->atEnd || count <= 0) is True
No need to recheck count against FETCH_ALL.

Is it worth correcting them?

regards,
Ranier Vilela

Attachments:

fix_null_deference_pquery.patchapplication/octet-stream; name=fix_null_deference_pquery.patchDownload

diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 96ea74f118..6329756f3a 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -878,6 +878,11 @@ PortalRunSelect(Portal portal,
 	 */
 	if (queryDesc)
 		queryDesc->dest = dest;
+    else if (!portal->holdStore)
+        ereport(ERROR,
+		   (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+		    errmsg("cursor can not scan"),
+		    errhint("Not ready held data, to scan.")));
 
 	/*
 	 * Determine which direction to go in, and check to see if we're already
@@ -898,11 +903,12 @@ PortalRunSelect(Portal portal,
 			count = 0;			/* don't pass negative count to executor */
 		}
 		else
+		{
 			direction = ForwardScanDirection;
-
-		/* In the executor, zero count processes all rows */
-		if (count == FETCH_ALL)
-			count = 0;
+		    /* In the executor, zero count processes all rows */
+		    if (count == FETCH_ALL)
+			    count = 0;
+		}
 
 		if (portal->holdStore)
 			nprocessed = RunFromStore(portal, direction, (uint64) count, dest);
@@ -938,11 +944,12 @@ PortalRunSelect(Portal portal,
 			count = 0;			/* don't pass negative count to executor */
 		}
 		else
+		{
 			direction = BackwardScanDirection;
-
-		/* In the executor, zero count processes all rows */
-		if (count == FETCH_ALL)
-			count = 0;
+		    /* In the executor, zero count processes all rows */
+		    if (count == FETCH_ALL)
+			    count = 0;
+		}
 
 		if (portal->holdStore)
 			nprocessed = RunFromStore(portal, direction, (uint64) count, dest);

Tom Lane

tgl@sss.pgh.pa.us

over 5 years ago

In reply to: Ranier Vilela (#1)

Re: Possible NULL dereferencing (src/backend/tcop/pquery.c)

Ranier Vilela <ranier.vf@gmail.com> writes:

1.Assertion check
/* Caller messed up if we have neither a ready query nor held data. */
Assert(queryDesc || portal->holdStore);

But in release, if QueryDesc is NULL and portal->holdStore is NULL too,
when Call PushActiveSnapshot *deference* NULL check can happen.

2. if (portal->atEnd || count <= 0) is True
No need to recheck count against FETCH_ALL.

Is it worth correcting them?

No.

The assertion already says that that's a case that cannot happen.
Or to look at it another way: if the case were to occur in a devel
build, you'd get a core dump at the assertion. If the case were
to occur in a production build, you'd get a core dump at the
dereference. Not much difference. Either way, it's a *caller*
bug, because the caller is supposed to make sure this cannot happen.
If we thought that it could possibly happen, we would use an ereport
but not an assertion; having both for the same condition is quite
misguided.

(If Coverity is whining about this for you, there's something wrong
with your Coverity settings. In the project's instance, Coverity
accepts assertions as assertions.)

I'm unimpressed with the other proposed change too; it's making the logic
more complicated and fragile for a completely negligible "performance
gain". Moreover the compiler could probably make the same optimization.

regards, tom lane

Ranier Vilela

ranier.vf@gmail.com

over 5 years ago

In reply to: Tom Lane (#2)

Re: Possible NULL dereferencing (src/backend/tcop/pquery.c)

Em sex., 26 de jun. de 2020 às 18:24, Tom Lane <tgl@sss.pgh.pa.us> escreveu:

Ranier Vilela <ranier.vf@gmail.com> writes:

1.Assertion check
/* Caller messed up if we have neither a ready query nor held data. */
Assert(queryDesc || portal->holdStore);

But in release, if QueryDesc is NULL and portal->holdStore is NULL too,
when Call PushActiveSnapshot *deference* NULL check can happen.

2. if (portal->atEnd || count <= 0) is True
No need to recheck count against FETCH_ALL.

Is it worth correcting them?

No.

The assertion already says that that's a case that cannot happen.
Or to look at it another way: if the case were to occur in a devel
build, you'd get a core dump at the assertion. If the case were
to occur in a production build, you'd get a core dump at the
dereference. Not much difference. Either way, it's a *caller*
bug, because the caller is supposed to make sure this cannot happen.
If we thought that it could possibly happen, we would use an ereport
but not an assertion; having both for the same condition is quite
misguided.

Ok, thats a job of Assertion.
But I still worry that, in some rare cases, portal-> holdStore might be
corrupted in some way
and the function is called, causing a segmentation fault.

(If Coverity is whining about this for you, there's something wrong
with your Coverity settings. In the project's instance, Coverity
accepts assertions as assertions.)

Probable, because reports this:
CID 10127 (#2 of 2): Dereference after null check (FORWARD_NULL)8.
var_deref_op: Dereferencing null pointer queryDesc.

I'm unimpressed with the other proposed change too; it's making the logic
more complicated and fragile for a completely negligible "performance
gain". Moreover the compiler could probably make the same optimization.

Ok.

Anyway, thank you for by responding, your observations are always valuable
and help learn "the postgres way" to develop.
It's not easy.

best regards,
Ranier Vilela