[PATCH] pgcrypto: Test for NULL before dereferencing pointer
Hi pgsql-hackers,
Currently contrib/pgcrypto/pgp-pubenc.c contains code like:
uint8 algo = pk->algo;
if (pk == NULL)
...
However, if pk was NULL, then the if() condition would never be
reached because the pk->algo dereference would segfault.
This patch moves the dereference to below the condition which was the
intended behavior.
Regards,
Marti
Attachments:
0001-pgcrypto-Test-for-NULL-before-dereferencing-pointer.patchtext/x-patch; charset=US-ASCII; name=0001-pgcrypto-Test-for-NULL-before-dereferencing-pointer.patchDownload
From a2500cda9e0e82883854a412ea12942e174e3dd2 Mon Sep 17 00:00:00 2001
From: Marti Raudsepp <marti@juffo.org>
Date: Wed, 20 Oct 2010 18:32:17 +0300
Subject: [PATCH] pgcrypto: Test for NULL before dereferencing pointer
If pk is NULL, the backend would segfault when accessing ->algo and the
following NULL check was never reached.
This problem was found by Coccinelle (null_ref.cocci from coccicheck)
---
contrib/pgcrypto/pgp-pubenc.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/contrib/pgcrypto/pgp-pubenc.c b/contrib/pgcrypto/pgp-pubenc.c
index 4b4d1bf..943d2e4 100644
--- a/contrib/pgcrypto/pgp-pubenc.c
+++ b/contrib/pgcrypto/pgp-pubenc.c
@@ -199,7 +199,7 @@ pgp_write_pubenc_sesskey(PGP_Context *ctx, PushFilter *dst)
PGP_PubKey *pk = ctx->pub_key;
uint8 ver = 3;
PushFilter *pkt = NULL;
- uint8 algo = pk->algo;
+ uint8 algo;
if (pk == NULL)
{
@@ -207,6 +207,8 @@ pgp_write_pubenc_sesskey(PGP_Context *ctx, PushFilter *dst)
return PXE_BUG;
}
+ algo = pk->algo;
+
/*
* now write packet
*/
--
1.7.3.1
On 20.10.2010 18:44, Marti Raudsepp wrote:
Hi pgsql-hackers,
Currently contrib/pgcrypto/pgp-pubenc.c contains code like:
uint8 algo = pk->algo;
if (pk == NULL)
...However, if pk was NULL, then the if() condition would never be
reached because the pk->algo dereference would segfault.This patch moves the dereference to below the condition which was the
intended behavior.
Thanks, applied. Did coccicheck find anything else interesting?
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Wed, Oct 20, 2010 at 22:34, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
Did coccicheck find anything else interesting?
There's a file descriptor leak in psql/command.c function
process_file() -- on errors it just returns without closing the file.
But since it's quitting anyway, there's no practical impact. Should I
submit a patch for this as well?
Then there are a few more cases found by null_ref (same check as the
original patch). But on closer inspection, these are false positives,
because the variable is actually modified in between dereferencing and
the NULL check.
Then there's the 'badzero' check that finds a dozen cases where
pointers are compared to a literal 0, not a NULL. This is a only a
coding style check, as far as I can tell, so I thought it's not worth
it.
Regards,
Marti
Marti Raudsepp <marti@juffo.org> writes:
There's a file descriptor leak in psql/command.c function
process_file() -- on errors it just returns without closing the file.
But since it's quitting anyway, there's no practical impact. Should I
submit a patch for this as well?
Might as well. It's the kind of thing that could turn into a real
bug given some rearrangement of the code.
Then there's the 'badzero' check that finds a dozen cases where
pointers are compared to a literal 0, not a NULL. This is a only a
coding style check, as far as I can tell, so I thought it's not worth
it.
I'd be in favor of fixing those too. I have no particular problem with
either "if (ptr)" or "if (ptr != NULL)", but I think that "if (ptr != 0)"
gives the reader entirely the wrong impression about the datatype of ptr.
Just because C fails to distinguish doesn't make it good style.
regards, tom lane