On Thu, Jun 14, 2012 at 1:48 PM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote: > On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> Committed. > > > A minor gripe: > > + /* > + * Close the backup label file. > + */ > + if (ferror(lfp) || FreeFile(lfp)) { > + ereport(ERROR, > + (errcode_for_file_access(), > + errmsg("could not read file \"%s\": %m", > + BACKUP_LABEL_FILE))); > + } > + > > If ferror(lfp) returns false, wouldn't we miss the FreeFile() and leak a > file pointer?
Well, according to the comments for AllocateFile:
* fd.c will automatically close all files opened with AllocateFile at * transaction commit or abort; this prevents FD leakage if a routine * that calls AllocateFile is terminated prematurely by ereport(ERROR).
I bet anyone else looking at this code, who is not in the know, will trip over this again.
Another problem with that code block is that it will throw "could not read" even though read has succeeded but FreeFile() failed.
I say we break it into two blocks, one to handle read error, and then close the file separately. Also, either make sure FreeFile() is called in all code paths, or not call FreeFile() at all and reference to the comment above AllocateFile().
Patch attached.
Regards,
--
Gurjeet Singh EnterpriseDB Corporation The Enterprise PostgreSQL Company