ccs-flatten: fix possible memleak

...found by static analysis

Account for simplistic logic of the call site (our goal is not
"read as many bytes as you can, stop on first allocation error,
but be fine with what was read so far, if any" -- that is not
intended when you need to read whole parseable XML).

There are more considerations:
- realloc(p, 0) may or may not free p, one shouldn't free it manually
  to prevent double-free (do I understand "man 3p realloc" right?)
- length (size_t) can overflow, leading realloc to actually decrease
  the allocated memory, perhaps even to zero, i.e., possible free
  as mentioned above)
- realloc(NULL, size) == malloc(size)
- check in "if (x) free(x);" is redundant
- if read_pipe returns OK, we can now safely assume: size > 0

Also following principles are followed:
- there should be just one cleanup on error
- notable code branches and otherwise silent assumptions commented upon

Signed-off-by: Jan Pokorný <jpokorny@redhat.com>

1 files changed, 25 insertions, 18 deletions

diff --git a/__root__/ccs-flatten/resrules.c b/__root__/ccs-flatten/resrules.c
index 2f4d924..7afccd0 100644
--- a/__root__/ccs-flatten/resrules.c
+++ b/__root__/ccs-flatten/resrules.c
@@ -693,12 +693,16 @@ _get_childtypes(xmlDocPtr doc, xmlXPathContextPtr ctx, char *base, resource_rule
 }
 
 /**
-  Read a file from a stdout pipe.
+   Read a file from a stdout pipe.
+
+   @return              0 on success (file and length set/alloc'd accordingly;
+                        caller responsible for freeing file), nonzero on
+                        error/failure (file is free'd and length set to zero)
  */
 static int
 read_pipe(int fd, char **file, size_t * length)
 {
-    char buf[4096];
+    char buf[4096], *tmp_file;
     int n, done = 0;
 
     *file = NULL;
@@ -708,29 +712,35 @@ read_pipe(int fd, char **file, size_t * length)
 
         n = read(fd, buf, sizeof(buf));
         if (n < 0) {
-
             if (errno == EINTR)
                 continue;
+            tmp_file = NULL;
+        }
 
-            if (*file)
-                free(*file);
-            return -1;
+        else if (n == 0 && (!*length)) {
+            return 0;  /* nothing to do */
         }
 
-        if (n == 0 && (!*length))
-            return 0;
+        else {
+            if (n == 0)
+                done = 1;
 
-        if (n == 0) {
-            done = 1;
+            if ((size_t)(*length + n + done) <= *length)
+                /* size_t overflow case, we assume: n + done > 0 */
+                tmp_file = NULL;
+            else
+                tmp_file = realloc(*file, (*length) + n + done);
         }
 
-        if (*file)
-            *file = realloc(*file, (*length) + n + done);
-        else
-            *file = malloc(n + done);
+        if (!tmp_file) {
+            *length = 0;
+            free(*file);  /* prevent possible memleak */
+        }
+
+        *file = tmp_file;
 
         if (!*file)
-            return -1;
+            return -1;  /* error case, we cleaned up everything */
 
         memcpy((*file) + (*length), buf, n);
         *length += (done + n);
@@ -785,9 +795,6 @@ read_resource_agent_metadata(char *filename)
     waitpid(pid, NULL, 0);
     close(_pipe[0]);
 
-    if (!size)
-        return NULL;
-
     doc = xmlParseMemory(data, size);
     free(data);
     return doc;