Thread: Two coverity non-bugs
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. Currently they've fixed the bogus errors relating to elog(), but not yet the ones relating to ereport(). There's still mountains of crap to wade through and not a single real bug found yet. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > From each according to his ability. To each according to his ability to litigate.
Attachment
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/ > From each according to his ability. To each according to his ability to litigate.
Attachment
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();