Re: [HACKERS] [GENERAL] Clang 3.3 Analyzer Results
On Wednesday, November 13, 2013, Tom Lane wrote: Kevin Grittner kgri...@ymail.com javascript:; writes: If nobody objects, I'll fix that small memory leak in the regression test driver. Hopefully someone more familiar with pg_basebackup will fix the double-free (and related problems mentioned by Tom) in streamutil.c. Here's a less convoluted (IMHO) approach to the password management logic in streamutil.c. One thing I really didn't care for about the existing coding is that the loop-for-password included all the rest of the function, even though there's no intention to retry for any purpose except collecting a password. So I moved up the bottom of the loop. For ease of review, I've not reindented the code below the new loop bottom, but would do so before committing. Any objections to this version? Nope, looks good to me. That code was originally stolen from psql, and then whacked around a number of times. The part about looping and passwords, for example, is in startup.c in psql as well. We probably want to fix it there as well (even if it doesn't have the same problem, it has the same general design). Or perhaps even put that function somewhere shared between the two? It's also in pg_dump/pg_backup_db.c, there's a version of it in pg_dumpall.c, etc. Which I think is a good argument for fixing them all by sharing the code somewhere? In fact, we already have some in script/common.c - but it's only used by the tools that are in script/. //Magnus -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] [GENERAL] Clang 3.3 Analyzer Results
Magnus Hagander mag...@hagander.net writes: That code was originally stolen from psql, and then whacked around a number of times. The part about looping and passwords, for example, is in startup.c in psql as well. We probably want to fix it there as well (even if it doesn't have the same problem, it has the same general design). Or perhaps even put that function somewhere shared between the two? It's also in pg_dump/pg_backup_db.c, there's a version of it in pg_dumpall.c, etc. Which I think is a good argument for fixing them all by sharing the code somewhere? In fact, we already have some in script/common.c - but it's only used by the tools that are in script/. Hm, maybe, but where? It's inappropriate for libpgcommon (we don't want that calling libpq), so I'm not real sure what to do with it. Also it's not clear to me that all these tools would have the same requirements for the non-password parameters for the connection request. BTW, I realized while fooling with this that although the code looks like it's intended to iterate till a correct password is obtained, actually it cannot prompt more than once, because of the way PQconnectionNeedsPassword is coded. Therefore, the double free that clang is worried about can't really happen. It's still worth fixing IMO. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
