Fix multiple bugs and infelicities in pg_rewind.

Bugs all spotted by Coverity, including wrong realloc() size request
and memory leaks.  Cosmetic improvements by me.

The usage of the global variable "filemap" here is still pretty awful,
but at least I got rid of the gratuitous aliasing in several routines
(which was helping to annoy Coverity, as well as being a bug risk).

diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index 4e026473066b599a41a50e5b8635976dbeae3ace..ee6e6db9b02b6b71ffb6f89b92371cd7f092be3c 100644 (file)
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -30,12 +30,12 @@ static char *datasegpath(RelFileNode rnode, ForkNumber forknum,
            BlockNumber segno);
 static int path_cmp(const void *a, const void *b);
 static int final_filemap_cmp(const void *a, const void *b);
-static void filemap_list_to_array(void);
+static void filemap_list_to_array(filemap_t *map);
 
 /*
- * Create a new file map.
+ * Create a new file map (stored in the global pointer "filemap").
  */
-filemap_t *
+void
 filemap_create(void)
 {
    filemap_t  *map;
@@ -48,8 +48,6 @@ filemap_create(void)
 
    Assert(filemap == NULL);
    filemap = map;
-
-   return map;
 }
 
 /*
@@ -271,7 +269,10 @@ process_local_file(const char *path, file_type_t type, size_t oldsize,
            pg_fatal("remote file list is empty\n");
        }
 
-       filemap_list_to_array();
+       filemap_list_to_array(map);
+
+       Assert(map->array != NULL);
+
        qsort(map->array, map->narray, sizeof(file_entry_t *), path_cmp);
    }
 
@@ -284,8 +285,8 @@ process_local_file(const char *path, file_type_t type, size_t oldsize,
 
    key.path = (char *) path;
    key_ptr = &key;
-   exists = bsearch(&key_ptr, map->array, map->narray, sizeof(file_entry_t *),
-                    path_cmp) != NULL;
+   exists = (bsearch(&key_ptr, map->array, map->narray, sizeof(file_entry_t *),
+                     path_cmp) != NULL);
 
    /* Remove any file or folder that doesn't exist in the remote system. */
    if (!exists)
@@ -335,7 +336,7 @@ process_block_change(ForkNumber forknum, RelFileNode rnode, BlockNumber blkno)
    filemap_t  *map = filemap;
    file_entry_t **e;
 
-   Assert(filemap->array);
+   Assert(map->array);
 
    segno = blkno / RELSEG_SIZE;
    blkno_inseg = blkno % RELSEG_SIZE;
@@ -395,38 +396,40 @@ process_block_change(ForkNumber forknum, RelFileNode rnode, BlockNumber blkno)
 }
 
 /*
- * Convert the linked list of entries in filemap->first/last to the array,
- * filemap->array.
+ * Convert the linked list of entries in map->first/last to the array,
+ * map->array.
  */
 static void
-filemap_list_to_array(void)
+filemap_list_to_array(filemap_t *map)
 {
    int         narray;
    file_entry_t *entry,
               *next;
 
-   filemap->array =
-       pg_realloc(filemap->array,
-                  (filemap->nlist + filemap->narray) * sizeof(file_entry_t));
+   map->array = (file_entry_t **)
+       pg_realloc(map->array,
+                  (map->nlist + map->narray) * sizeof(file_entry_t *));
 
-   narray = filemap->narray;
-   for (entry = filemap->first; entry != NULL; entry = next)
+   narray = map->narray;
+   for (entry = map->first; entry != NULL; entry = next)
    {
-       filemap->array[narray++] = entry;
+       map->array[narray++] = entry;
        next = entry->next;
        entry->next = NULL;
    }
-   Assert(narray == filemap->nlist + filemap->narray);
-   filemap->narray = narray;
-   filemap->nlist = 0;
-   filemap->first = filemap->last = NULL;
+   Assert(narray == map->nlist + map->narray);
+   map->narray = narray;
+   map->nlist = 0;
+   map->first = map->last = NULL;
 }
 
 void
 filemap_finalize(void)
 {
-   filemap_list_to_array();
-   qsort(filemap->array, filemap->narray, sizeof(file_entry_t *),
+   filemap_t  *map = filemap;
+
+   filemap_list_to_array(map);
+   qsort(map->array, map->narray, sizeof(file_entry_t *),
          final_filemap_cmp);
 }
 
@@ -466,9 +469,9 @@ calculate_totals(void)
    map->total_size = 0;
    map->fetch_size = 0;
 
-   for (i = 0; i < filemap->narray; i++)
+   for (i = 0; i < map->narray; i++)
    {
-       entry = filemap->array[i];
+       entry = map->array[i];
 
        if (entry->type != FILE_TYPE_REGULAR)
            continue;
@@ -501,12 +504,13 @@ calculate_totals(void)
 void
 print_filemap(void)
 {
+   filemap_t  *map = filemap;
    file_entry_t *entry;
    int         i;
 
-   for (i = 0; i < filemap->narray; i++)
+   for (i = 0; i < map->narray; i++)
    {
-       entry = filemap->array[i];
+       entry = map->array[i];
        if (entry->action != FILE_ACTION_NONE ||
            entry->pagemap.bitmapsize > 0)
        {

diff --git a/src/bin/pg_rewind/filemap.h b/src/bin/pg_rewind/filemap.h
index 57f0f92fb90dcc9a6db10286f9d2cd450bab39d9..8fa19390bc3231f8e30a0e3844c8faca247384fb 100644 (file)
--- a/src/bin/pg_rewind/filemap.h
+++ b/src/bin/pg_rewind/filemap.h
@@ -29,8 +29,7 @@ typedef enum
    FILE_ACTION_NONE,       /* no action (we might still copy modified blocks
                             * based on the parsed WAL) */
    FILE_ACTION_TRUNCATE,   /* truncate local file to 'newsize' bytes */
-   FILE_ACTION_REMOVE,     /* remove local file / directory / symlink */
-
+   FILE_ACTION_REMOVE      /* remove local file / directory / symlink */
 } file_action_t;
 
 typedef enum
@@ -40,7 +39,7 @@ typedef enum
    FILE_TYPE_SYMLINK
 } file_type_t;
 
-struct file_entry_t
+typedef struct file_entry_t
 {
    char       *path;
    file_type_t type;
@@ -58,11 +57,9 @@ struct file_entry_t
    char        *link_target;
 
    struct file_entry_t *next;
-};
-
-typedef struct file_entry_t file_entry_t;
+} file_entry_t;
 
-struct filemap_t
+typedef struct filemap_t
 {
    /*
     * New entries are accumulated to a linked list, in process_remote_file
@@ -70,7 +67,7 @@ struct filemap_t
     */
    file_entry_t *first;
    file_entry_t *last;
-   int         nlist;
+   int         nlist;          /* number of entries currently in list */
 
    /*
     * After processing all the remote files, the entries in the linked list
@@ -80,7 +77,7 @@ struct filemap_t
     * the array, and the linked list is empty.
     */
    file_entry_t **array;
-   int         narray;
+   int         narray;         /* current length of array */
 
    /*
     * Summary information. total_size is the total size of the source cluster,
@@ -88,14 +85,11 @@ struct filemap_t
     */
    uint64      total_size;
    uint64      fetch_size;
-};
-
-typedef struct filemap_t filemap_t;
-
-extern filemap_t * filemap;
+} filemap_t;
 
-extern filemap_t *filemap_create(void);
+extern filemap_t *filemap;
 
+extern void filemap_create(void);
 extern void calculate_totals(void);
 extern void print_filemap(void);
 

diff --git a/src/bin/pg_rewind/libpq_fetch.c b/src/bin/pg_rewind/libpq_fetch.c
index 0c9d46d209d8679b44428a21d03f87f1102bb427..23c971ab1c2ca7524355d7329594bd5b9c5bbea8 100644 (file)
--- a/src/bin/pg_rewind/libpq_fetch.c
+++ b/src/bin/pg_rewind/libpq_fetch.c
@@ -285,6 +285,10 @@ receiveFileChunks(const char *sql)
        open_target_file(filename, false);
 
        write_target_range(chunk, chunkoff, chunksize);
+
+       pg_free(filename);
+
+       PQclear(res);
    }
 }
 

diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 6d458b034c1a78d963dce5005bf81724de307305..dda3a7988b1e1def19560ffaaffa65bc90398dcc 100644 (file)
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -248,7 +248,7 @@ main(int argc, char **argv)
    /*
     * Build the filemap, by comparing the remote and local data directories.
     */
-   (void) filemap_create();
+   filemap_create();
    pg_log(PG_PROGRESS, "reading source file list\n");
    fetchRemoteFileList();
    pg_log(PG_PROGRESS, "reading target file list\n");