Thread

1. [PATCH] pgcrypto: Test for NULL before dereferencing pointer

   Marti Raudsepp <marti@juffo.org> — 2010-10-20T15:44:40Z

   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
   

2. Re: [PATCH] pgcrypto: Test for NULL before dereferencing pointer

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-10-20T19:34:06Z

   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
   
   

3. Re: [PATCH] pgcrypto: Test for NULL before dereferencing pointer

   Marti Raudsepp <marti@juffo.org> — 2010-10-20T20:18:11Z

   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
   
   

4. Re: [PATCH] pgcrypto: Test for NULL before dereferencing pointer

   Tom Lane <tgl@sss.pgh.pa.us> — 2010-10-20T20:36:39Z

   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