Re: [PATCHES] Two coverity non-bugs
Martijn van Oosterhout 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 - 1.43 --- src/bin/psql/prompt.c 19 Apr 2006 16:00:32 - *** *** 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.c12 Apr 2006 22:18:48 - 1.126 --- src/bin/pg_dump/pg_backup_archiver.c19 Apr 2006 16:00:32 - *** *** 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(); ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] Two coverity non-bugs
Martijn van Oosterhout 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 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] Two coverity non-bugs
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 http://svana.org/kleptog/ > From each according to his ability. To each according to his ability to > litigate. signature.asc Description: Digital signature
Re: [PATCHES] Two coverity non-bugs
Martijn van Oosterhout 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
[PATCHES] 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 http://svana.org/kleptog/ > From each according to his ability. To each according to his ability to > litigate. Index: src/bin/psql/prompt.c === RCS file: /projects/cvsroot/pgsql/src/bin/psql/prompt.c,v retrieving revision 1.43 diff -u -r1.43 prompt.c --- src/bin/psql/prompt.c 5 Mar 2006 15:58:52 - 1.43 +++ src/bin/psql/prompt.c 19 Apr 2006 11:58:37 - @@ -256,8 +256,7 @@ cmdend = strcspn(file, "`"); file[cmdend] = '\0'; - if (file) - fd = popen(file, "r"); + fd = popen(file, "r"); if (fd) { fgets(buf, MAX_PROMPT_SIZE - 1, fd); Index: src/bin/pg_dump/pg_backup_archiver.c === RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_backup_archiver.c,v retrieving revision 1.126 diff -u -r1.126 pg_backup_archiver.c --- src/bin/pg_dump/pg_backup_archiver.c12 Apr 2006 22:18:48 - 1.126 +++ src/bin/pg_dump/pg_backup_archiver.c19 Apr 2006 11:58:37 - @@ -2130,8 +2130,7 @@ AH->currUser = strdup(""); /* don't assume we still know the output schema */ - if (AH->currSchema) - free(AH->currSchema); + free(AH->currSchema); AH->currSchema = strdup(""); AH->currWithOids = -1; @@ -2229,8 +2228,7 @@ else ahprintf(AH, "%s;\n\n", qry->data); - if (AH->currSchema) - free(AH->currSchema); + free(AH->currSchema); AH->currSchema = strdup(schemaName); destroyPQExpBuffer(qry); signature.asc Description: Digital signature