Two coverity non-bugs

Martijn van Oosterhout <kleptog@svana.org> writes:

Attached is a patch that fixes two non-bugs. There's plenty of
redundant NULL checks around the place but these were just so silly I
figure they're worth fixing.

The first checks 'file' the line after having dereferenced it. The
second checks 'currSchema' where the code is designed to never allow it
to be NULL.

The first patch looks reasonable but I object to the second. Coverity
is not going to dictate coding conventions to us --- it is not *nearly*
bright enough for that. In this case, the code is locally allowing for
the possibility that AH->currSchema was NULL, and I don't consider it
good style to remove that allowance. (I take it BTW that this allegedly
bulletproof tool fails to consider the possibility that strdup fails
and returns NULL ...)

regards, tom lane

On Wed, Apr 19, 2006 at 10:32:21AM -0400, Tom Lane wrote:

The first patch looks reasonable but I object to the second. Coverity
is not going to dictate coding conventions to us --- it is not *nearly*
bright enough for that. In this case, the code is locally allowing for
the possibility that AH->currSchema was NULL, and I don't consider it
good style to remove that allowance. (I take it BTW that this allegedly
bulletproof tool fails to consider the possibility that strdup fails
and returns NULL ...)

Sure. These are just high on the scale of silliness. There are many
more that are less silly which I'm just ignoring.

Note, what coverity actually picked up was that in the latter case in
_selectOutputSchema that the free will always execute because if
AH->currSchema was NULL it would have died at the beginning of the
function (line 2205). It's me who misinterpreted the fix. We should
probably be adding checks to all the strdups and to line 2205 instead.

It doesn't directly complain about strdup itself because nowhere does
any code do a strdup and dereference it in the same function.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Martijn van Oosterhout <kleptog@svana.org> writes:

Note, what coverity actually picked up was that in the latter case in
_selectOutputSchema that the free will always execute because if
AH->currSchema was NULL it would have died at the beginning of the
function (line 2205). It's me who misinterpreted the fix. We should
probably be adding checks to all the strdups and to line 2205 instead.

Ah, OK, so it wasn't doing this on the basis of a global determination
that currSchema couldn't be NULL. That makes more sense then. I agree
that line 2205 would be more consistent if it checked for null currSchema.

regards, tom lane

Martijn van Oosterhout <kleptog@svana.org> writes:

Attached is a patch that fixes two non-bugs. There's plenty of
redundant NULL checks around the place but these were just so silly I
figure they're worth fixing.

Applied in the following modified form.

regards, tom lane

Index: src/bin/psql/prompt.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/prompt.c,v
retrieving revision 1.43
diff -c -r1.43 prompt.c
*** src/bin/psql/prompt.c	5 Mar 2006 15:58:52 -0000	1.43
--- src/bin/psql/prompt.c	19 Apr 2006 16:00:32 -0000
***************
*** 250,263 ****
  					/* execute command */
  				case '`':
  					{
! 						FILE	   *fd = NULL;
  						char	   *file = pg_strdup(p + 1);
  						int			cmdend;

  						cmdend = strcspn(file, "`");
  						file[cmdend] = '\0';
! 						if (file)
! 							fd = popen(file, "r");
  						if (fd)
  						{
  							fgets(buf, MAX_PROMPT_SIZE - 1, fd);
--- 250,262 ----
  					/* execute command */
  				case '`':
  					{
! 						FILE	   *fd;
  						char	   *file = pg_strdup(p + 1);
  						int			cmdend;

  						cmdend = strcspn(file, "`");
  						file[cmdend] = '\0';
! 						fd = popen(file, "r");
  						if (fd)
  						{
  							fgets(buf, MAX_PROMPT_SIZE - 1, fd);
Index: src/bin/pg_dump/pg_backup_archiver.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_backup_archiver.c,v
retrieving revision 1.126
diff -c -r1.126 pg_backup_archiver.c
*** src/bin/pg_dump/pg_backup_archiver.c	12 Apr 2006 22:18:48 -0000	1.126
--- src/bin/pg_dump/pg_backup_archiver.c	19 Apr 2006 16:00:32 -0000
***************
*** 2203,2209 ****
  	PQExpBuffer qry;

if (!schemaName || *schemaName == '\0' ||
! strcmp(AH->currSchema, schemaName) == 0)
return; /* no need to do anything */

  	qry = createPQExpBuffer();
--- 2203,2209 ----
  	PQExpBuffer qry;

if (!schemaName || *schemaName == '\0' ||
! (AH->currSchema && strcmp(AH->currSchema, schemaName) == 0))
return; /* no need to do anything */

qry = createPQExpBuffer();