[refpolicy] Pull Request for fixing memory leak warning

[refpolicy] Pull Request for fixing memory leak warning

[William Roberts]

Can we get someone to take a look at:
https://github.com/TresysTechnology/refpolicy/pull/125

It essentially fixes an inaccurate static analysis warning on fc_sort
and will get the Android tree back in-sync with upstream.

-- 
Respectfully,

William C Roberts

[refpolicy] [PATCH] fc_sort: memory leakages (was: Pull Request for fixing memory leak warning)

[Guido Trentalancia]

Hello.

I have tried applying the changes you suggested, but valgrind still
reports memory leakages in the fc_sort executable.

So, I have created a simple patch which avoids valgrind errors
completely and also carries out some extra memory consistency checks.

The patch is attached below. Feel free to use it, if it proves useful.

On Thu, 28/09/2017 at 09.05 -0700, William Roberts via
refpolicy wrote:
> Can we get someone to take a look at:
> https://github.com/TresysTechnology/refpolicy/pull/125
> 
> It essentially fixes an inaccurate static analysis warning on fc_sort
> and will get the Android tree back in-sync with upstream.
> 

Avoid memory leakages in the fc_sort executable (now passes
all valgrind tests fine).

Some NULL pointer checks with or without associated error
reporting.

Some white space and comment formatting fixes.

Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
---
 support/fc_sort.c |  110 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 89 insertions(+), 21 deletions(-)

--- a/support/fc_sort.c	2017-09-28 20:52:44.900145312 +0200
+++ b/support/fc_sort.c	2017-09-29 01:38:53.600075122 +0200
@@ -46,6 +46,9 @@ typedef struct file_context_node {
 
 void file_context_node_destroy(file_context_node_t *x)
 {
+	if (!x)
+		return;
+
 	free(x->path);
 	free(x->file_type);
 	free(x->context);
@@ -307,6 +310,40 @@ void fc_fill_data(file_context_node_t *f
 	}
 }
 
+
+
+/* fc_free_file_context_node_list
+ * Free the memory allocated to the linked list and its elements.
+ */
+void fc_free_file_context_node_list(struct file_context_node *node)
+{
+	struct file_context_node *next;
+
+	while (node) {
+		next = node->next;
+		file_context_node_destroy(node);
+		free(node);
+		node = next;
+	}
+}
+
+
+
+/* fc_free_file_context_bucket_list
+ * Free the memory allocated to the linked list but not its elements
+ * as they are deallocated when freeing the old file context list.
+ */
+void fc_free_file_context_bucket_list(struct file_context_bucket *element)
+{
+	struct file_context_bucket *next;
+
+	while (element) {
+		next = element->next;
+		free(element);
+		element = next;
+	}
+}
+
 /* main
  * This program takes in two arguments, the input filename and the
  *  output filename. The input file should be syntactically correct.
@@ -317,7 +354,7 @@ int main(int argc, char *argv[])
 {
 	int lines;
 	size_t start, finish, regex_len, context_len;
-	size_t line_len, buf_len, i;
+	size_t line_len, i;
 	char *input_name, *output_name, *line_buf;
 
 	file_context_node_t *temp;
@@ -328,7 +365,6 @@ int main(int argc, char *argv[])
 
 	FILE *in_file, *out_file;
 
-
 	/* Check for the correct number of command line arguments. */
 	if (argc < 2 || argc > 3) {
 		fprintf(stderr, "Usage: %s <infile> [<outfile>]\n",argv[0]);
@@ -348,15 +384,26 @@ int main(int argc, char *argv[])
 
 	/* Initialize the head of the linked list. */
 	head = current = (file_context_node_t*)malloc(sizeof(file_context_node_t));
+	if (!head) {
+		fprintf(stderr, "Error: failure allocating memory.\n");
+		return 1;
+	}
 	head->next = NULL;
+	head->path = NULL;
+	head->file_type = NULL;
+	head->context = NULL;
 
 	/* Parse the file into a file_context linked list. */
 	line_buf = NULL;
 
-	while ( getline(&line_buf, &buf_len, in_file) != -1 ){
+	while ( getline(&line_buf, NULL, in_file) != -1 ){
 		line_len = strlen(line_buf);
-		if( line_len == 0 || line_len == 1)
+		if( line_len == 0 || line_len == 1) {
+			free(line_buf);
+			line_buf = NULL;
 			continue;
+		}
+
 		/* Get rid of whitespace from the front of the line. */
 		for (i = 0; i < line_len; i++) {
 			if (!isspace(line_buf[i]))
@@ -364,16 +411,25 @@ int main(int argc, char *argv[])
 		}
 
 
-		if (i >= line_len)
+		if (i >= line_len) {
+			free(line_buf);
+			line_buf = NULL;
 			continue;
+		}
+
 		/* Check if the line isn't empty and isn't a comment */
-		if (line_buf[i] == '#')
+		if (line_buf[i] == '#') {
+			free(line_buf);
+			line_buf = NULL;
 			continue;
+		}
 
 		/* We have a valid line - allocate a new node. */
 		temp = (file_context_node_t *)malloc(sizeof(file_context_node_t));
 		if (!temp) {
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 		temp->next = NULL;
@@ -393,8 +449,8 @@ int main(int argc, char *argv[])
 		if (regex_len == 0) {
 			file_context_node_destroy(temp);
 			free(temp);
-
-
+			free(line_buf);
+			line_buf = NULL;
 			continue;
 		}
 
@@ -402,7 +458,9 @@ int main(int argc, char *argv[])
 		if (!temp->path) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 
@@ -416,22 +474,29 @@ int main(int argc, char *argv[])
 		if (i == line_len) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
+			line_buf = NULL;
 			continue;
 		}
 
 		/* Parse out the type from the line (if it
-			*  is there). */
+		 *  is there). */
 		if (line_buf[i] == '-') {
 			temp->file_type = (char *)malloc(sizeof(char) * 3);
 			if (!(temp->file_type)) {
+				file_context_node_destroy(temp);
+				free(temp);
+				free(line_buf);
 				fprintf(stderr, "Error: failure allocating memory.\n");
+				fc_free_file_context_node_list(head);
 				return 1;
 			}
 
 			if( i + 2 >= line_len ) {
 				file_context_node_destroy(temp);
 				free(temp);
-
+				free(line_buf);
+				line_buf = NULL;
 				continue;
 			}
 
@@ -448,9 +513,10 @@ int main(int argc, char *argv[])
 			}
 
 			if (i == line_len) {
-
 				file_context_node_destroy(temp);
 				free(temp);
+				free(line_buf);
+				line_buf = NULL;
 				continue;
 			}
 		}
@@ -467,21 +533,22 @@ int main(int argc, char *argv[])
 		if (!temp->context) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 
 		/* Set all the data about the regular
-			*  expression. */
+		 * expression. */
 		fc_fill_data(temp);
 
 		/* Link this line of code at the end of
-			*  the linked list. */
+		 * the linked list. */
 		current->next = temp;
 		current = current->next;
 		lines++;
 
-
 		free(line_buf);
 		line_buf = NULL;
 	}
@@ -495,6 +562,7 @@ int main(int argc, char *argv[])
 	if (!bcurrent) {
 		printf
 		    ("Error: failure allocating memory.\n"); 
+		fc_free_file_context_node_list(head);
 		return -1;
 	}
 	bcurrent->next = NULL;
@@ -517,6 +585,8 @@ int main(int argc, char *argv[])
 			if (!(bcurrent->next)) {
 				printf
 				    ("Error: failure allocating memory.\n");
+				fc_free_file_context_node_list(head);
+				fc_free_file_context_bucket_list(master);
 				return -1;
 			}
 
@@ -536,6 +606,8 @@ int main(int argc, char *argv[])
 	if (output_name) {
 		if (!(out_file = fopen(output_name, "w"))) {
 			printf("Error: failure opening output file for write.\n");
+			fc_free_file_context_node_list(head);
+			fc_free_file_context_bucket_list(master);
 			return -1;
 		}
 	} else {
@@ -556,15 +628,11 @@ int main(int argc, char *argv[])
 		/* Output the context. */
 		fprintf(out_file, "%s\n", current->context);
 
-		/* Remove the node. */
-		temp = current;
 		current = current->next;
-
-		file_context_node_destroy(temp);
-		free(temp);
-
 	}
-	free(master);
+
+	fc_free_file_context_node_list(head);
+	fc_free_file_context_bucket_list(master);
 
 	if (output_name) {
 		fclose(out_file);

[refpolicy] [PATCH] fc_sort: memory leakages (was: Pull Request for fixing memory leak warning)

[William Roberts]

On Thu, Sep 28, 2017 at 5:12 PM, Guido Trentalancia via refpolicy
<refpolicy@oss.tresys.com> wrote:
> Hello.
>
> I have tried applying the changes you suggested, but valgrind still
> reports memory leakages in the fc_sort executable.
>
> So, I have created a simple patch which avoids valgrind errors
> completely and also carries out some extra memory consistency checks.
>
> The patch is attached below. Feel free to use it, if it proves useful.
>
> On Thu, 28/09/2017 at 09.05 -0700, William Roberts via
> refpolicy wrote:
>> Can we get someone to take a look at:
>> https://github.com/TresysTechnology/refpolicy/pull/125
>>
>> It essentially fixes an inaccurate static analysis warning on fc_sort
>> and will get the Android tree back in-sync with upstream.
>>
>
> Avoid memory leakages in the fc_sort executable (now passes
> all valgrind tests fine).
>
> Some NULL pointer checks with or without associated error
> reporting.
>
> Some white space and comment formatting fixes.
>
> Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
> ---
>  support/fc_sort.c |  110 +++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 89 insertions(+), 21 deletions(-)
>
> --- a/support/fc_sort.c 2017-09-28 20:52:44.900145312 +0200
> +++ b/support/fc_sort.c 2017-09-29 01:38:53.600075122 +0200
> @@ -46,6 +46,9 @@ typedef struct file_context_node {
>
>  void file_context_node_destroy(file_context_node_t *x)
>  {
> +       if (!x)
> +               return;
> +
>         free(x->path);
>         free(x->file_type);
>         free(x->context);
> @@ -307,6 +310,40 @@ void fc_fill_data(file_context_node_t *f
>         }
>  }
>
> +
> +
> +/* fc_free_file_context_node_list
> + * Free the memory allocated to the linked list and its elements.
> + */
> +void fc_free_file_context_node_list(struct file_context_node *node)
> +{
> +       struct file_context_node *next;
> +
> +       while (node) {
> +               next = node->next;
> +               file_context_node_destroy(node);
> +               free(node);
> +               node = next;
> +       }
> +}
> +
> +
> +
> +/* fc_free_file_context_bucket_list
> + * Free the memory allocated to the linked list but not its elements
> + * as they are deallocated when freeing the old file context list.
> + */
> +void fc_free_file_context_bucket_list(struct file_context_bucket *element)
> +{
> +       struct file_context_bucket *next;
> +
> +       while (element) {
> +               next = element->next;
> +               free(element);
> +               element = next;
> +       }
> +}
> +
>  /* main
>   * This program takes in two arguments, the input filename and the
>   *  output filename. The input file should be syntactically correct.
> @@ -317,7 +354,7 @@ int main(int argc, char *argv[])
>  {
>         int lines;
>         size_t start, finish, regex_len, context_len;
> -       size_t line_len, buf_len, i;
> +       size_t line_len, i;
>         char *input_name, *output_name, *line_buf;
>
>         file_context_node_t *temp;
> @@ -328,7 +365,6 @@ int main(int argc, char *argv[])
>
>         FILE *in_file, *out_file;
>
> -
>         /* Check for the correct number of command line arguments. */
>         if (argc < 2 || argc > 3) {
>                 fprintf(stderr, "Usage: %s <infile> [<outfile>]\n",argv[0]);
> @@ -348,15 +384,26 @@ int main(int argc, char *argv[])
>
>         /* Initialize the head of the linked list. */
>         head = current = (file_context_node_t*)malloc(sizeof(file_context_node_t));
> +       if (!head) {
> +               fprintf(stderr, "Error: failure allocating memory.\n");
> +               return 1;
> +       }
>         head->next = NULL;
> +       head->path = NULL;
> +       head->file_type = NULL;
> +       head->context = NULL;
>
>         /* Parse the file into a file_context linked list. */
>         line_buf = NULL;
>
> -       while ( getline(&line_buf, &buf_len, in_file) != -1 ){
> +       while ( getline(&line_buf, NULL, in_file) != -1 ){
>                 line_len = strlen(line_buf);
> -               if( line_len == 0 || line_len == 1)
> +               if( line_len == 0 || line_len == 1) {
> +                       free(line_buf);
> +                       line_buf = NULL;
>                         continue;
> +               }
> +
>                 /* Get rid of whitespace from the front of the line. */
>                 for (i = 0; i < line_len; i++) {
>                         if (!isspace(line_buf[i]))
> @@ -364,16 +411,25 @@ int main(int argc, char *argv[])
>                 }
>
>
> -               if (i >= line_len)
> +               if (i >= line_len) {
> +                       free(line_buf);
> +                       line_buf = NULL;
>                         continue;
> +               }
> +
>                 /* Check if the line isn't empty and isn't a comment */
> -               if (line_buf[i] == '#')
> +               if (line_buf[i] == '#') {
> +                       free(line_buf);
> +                       line_buf = NULL;
>                         continue;
> +               }
>
>                 /* We have a valid line - allocate a new node. */
>                 temp = (file_context_node_t *)malloc(sizeof(file_context_node_t));
>                 if (!temp) {
> +                       free(line_buf);
>                         fprintf(stderr, "Error: failure allocating memory.\n");
> +                       fc_free_file_context_node_list(head);
>                         return 1;
>                 }
>                 temp->next = NULL;
> @@ -393,8 +449,8 @@ int main(int argc, char *argv[])
>                 if (regex_len == 0) {
>                         file_context_node_destroy(temp);
>                         free(temp);
> -
> -
> +                       free(line_buf);
> +                       line_buf = NULL;
>                         continue;
>                 }
>
> @@ -402,7 +458,9 @@ int main(int argc, char *argv[])
>                 if (!temp->path) {
>                         file_context_node_destroy(temp);
>                         free(temp);
> +                       free(line_buf);
>                         fprintf(stderr, "Error: failure allocating memory.\n");
> +                       fc_free_file_context_node_list(head);
>                         return 1;
>                 }
>
> @@ -416,22 +474,29 @@ int main(int argc, char *argv[])
>                 if (i == line_len) {
>                         file_context_node_destroy(temp);
>                         free(temp);
> +                       free(line_buf);
> +                       line_buf = NULL;
>                         continue;
>                 }
>
>                 /* Parse out the type from the line (if it
> -                       *  is there). */
> +                *  is there). */
>                 if (line_buf[i] == '-') {
>                         temp->file_type = (char *)malloc(sizeof(char) * 3);
>                         if (!(temp->file_type)) {
> +                               file_context_node_destroy(temp);
> +                               free(temp);
> +                               free(line_buf);
>                                 fprintf(stderr, "Error: failure allocating memory.\n");
> +                               fc_free_file_context_node_list(head);
>                                 return 1;
>                         }
>
>                         if( i + 2 >= line_len ) {
>                                 file_context_node_destroy(temp);
>                                 free(temp);
> -
> +                               free(line_buf);
> +                               line_buf = NULL;
>                                 continue;
>                         }
>
> @@ -448,9 +513,10 @@ int main(int argc, char *argv[])
>                         }
>
>                         if (i == line_len) {
> -
>                                 file_context_node_destroy(temp);
>                                 free(temp);
> +                               free(line_buf);
> +                               line_buf = NULL;
>                                 continue;
>                         }
>                 }
> @@ -467,21 +533,22 @@ int main(int argc, char *argv[])
>                 if (!temp->context) {
>                         file_context_node_destroy(temp);
>                         free(temp);
> +                       free(line_buf);
>                         fprintf(stderr, "Error: failure allocating memory.\n");
> +                       fc_free_file_context_node_list(head);
>                         return 1;
>                 }
>
>                 /* Set all the data about the regular
> -                       *  expression. */
> +                * expression. */
>                 fc_fill_data(temp);
>
>                 /* Link this line of code at the end of
> -                       *  the linked list. */
> +                * the linked list. */
>                 current->next = temp;
>                 current = current->next;
>                 lines++;
>
> -
>                 free(line_buf);
>                 line_buf = NULL;
>         }
> @@ -495,6 +562,7 @@ int main(int argc, char *argv[])
>         if (!bcurrent) {
>                 printf
>                     ("Error: failure allocating memory.\n");
> +               fc_free_file_context_node_list(head);
>                 return -1;
>         }
>         bcurrent->next = NULL;
> @@ -517,6 +585,8 @@ int main(int argc, char *argv[])
>                         if (!(bcurrent->next)) {
>                                 printf
>                                     ("Error: failure allocating memory.\n");
> +                               fc_free_file_context_node_list(head);
> +                               fc_free_file_context_bucket_list(master);
>                                 return -1;
>                         }
>
> @@ -536,6 +606,8 @@ int main(int argc, char *argv[])
>         if (output_name) {
>                 if (!(out_file = fopen(output_name, "w"))) {
>                         printf("Error: failure opening output file for write.\n");
> +                       fc_free_file_context_node_list(head);
> +                       fc_free_file_context_bucket_list(master);
>                         return -1;
>                 }
>         } else {
> @@ -556,15 +628,11 @@ int main(int argc, char *argv[])
>                 /* Output the context. */
>                 fprintf(out_file, "%s\n", current->context);
>
> -               /* Remove the node. */
> -               temp = current;
>                 current = current->next;
> -
> -               file_context_node_destroy(temp);
> -               free(temp);
> -
>         }
> -       free(master);
> +
> +       fc_free_file_context_node_list(head);
> +       fc_free_file_context_bucket_list(master);
>
>         if (output_name) {
>                 fclose(out_file);
> _______________________________________________
> refpolicy mailing list
> refpolicy at oss.tresys.com
> http://oss.tresys.com/mailman/listinfo/refpolicy

The PR was mostly to avoid warnings with a static analysis tool IIUC.
I was trying to avoid any fork of that code between upstream and Google.

Do you plan on applying this change to the repo?

I've reached out to the google engineers here:
https://android-review.googlesource.com/#/c/platform/system/sepolicy/+/465218/

and in the PR
https://github.com/TresysTechnology/refpolicy/pull/125

So hopefully they can try it and let us know if it avoids the errors
with their analysis tools,
if valgrind reports 0 leaks, it likely will satisfy the static analysis.

Thanks,
Bill

[refpolicy] [PATCH] fc_sort: memory leakages (was: Pull Request for fixing memory leak warning)

[Guido Trentalancia]

On Thu, 28/09/2017 at 15.24 -0700, William Roberts wrote:
> On Thu, Sep 28, 2017 at 5:12 PM, Guido Trentalancia via refpolicy
> <refpolicy@oss.tresys.com> wrote:
> > Hello.
> > 
> > I have tried applying the changes you suggested, but valgrind still
> > reports memory leakages in the fc_sort executable.
> > 
> > So, I have created a simple patch which avoids valgrind errors
> > completely and also carries out some extra memory consistency
> > checks.
> > 
> > The patch is attached below. Feel free to use it, if it proves
> > useful.
> > 
> > On Thu, 28/09/2017 at 09.05 -0700, William Roberts via
> > refpolicy wrote:
> > > Can we get someone to take a look at:
> > > https://github.com/TresysTechnology/refpolicy/pull/125
> > > 
> > > It essentially fixes an inaccurate static analysis warning on
> > > fc_sort
> > > and will get the Android tree back in-sync with upstream.
> > > 
> > 
> > Avoid memory leakages in the fc_sort executable (now passes
> > all valgrind tests fine).
> > 
> > Some NULL pointer checks with or without associated error
> > reporting.
> > 
> > Some white space and comment formatting fixes.
> > 
> > Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
> > ---
> >  support/fc_sort.c |  110
> > +++++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 89 insertions(+), 21 deletions(-)
> > 
> > --- a/support/fc_sort.c 2017-09-28 20:52:44.900145312 +0200
> > +++ b/support/fc_sort.c 2017-09-29 01:38:53.600075122 +0200
> > @@ -46,6 +46,9 @@ typedef struct file_context_node {
> > 
> >  void file_context_node_destroy(file_context_node_t *x)
> >  {
> > +       if (!x)
> > +               return;
> > +
> >         free(x->path);
> >         free(x->file_type);
> >         free(x->context);
> > @@ -307,6 +310,40 @@ void fc_fill_data(file_context_node_t *f
> >         }
> >  }
> > 
> > +
> > +
> > +/* fc_free_file_context_node_list
> > + * Free the memory allocated to the linked list and its elements.
> > + */
> > +void fc_free_file_context_node_list(struct file_context_node
> > *node)
> > +{
> > +       struct file_context_node *next;
> > +
> > +       while (node) {
> > +               next = node->next;
> > +               file_context_node_destroy(node);
> > +               free(node);
> > +               node = next;
> > +       }
> > +}
> > +
> > +
> > +
> > +/* fc_free_file_context_bucket_list
> > + * Free the memory allocated to the linked list but not its
> > elements
> > + * as they are deallocated when freeing the old file context list.
> > + */
> > +void fc_free_file_context_bucket_list(struct file_context_bucket
> > *element)
> > +{
> > +       struct file_context_bucket *next;
> > +
> > +       while (element) {
> > +               next = element->next;
> > +               free(element);
> > +               element = next;
> > +       }
> > +}
> > +
> >  /* main
> >   * This program takes in two arguments, the input filename and the
> >   *  output filename. The input file should be syntactically
> > correct.
> > @@ -317,7 +354,7 @@ int main(int argc, char *argv[])
> >  {
> >         int lines;
> >         size_t start, finish, regex_len, context_len;
> > -       size_t line_len, buf_len, i;
> > +       size_t line_len, i;
> >         char *input_name, *output_name, *line_buf;
> > 
> >         file_context_node_t *temp;
> > @@ -328,7 +365,6 @@ int main(int argc, char *argv[])
> > 
> >         FILE *in_file, *out_file;
> > 
> > -
> >         /* Check for the correct number of command line arguments.
> > */
> >         if (argc < 2 || argc > 3) {
> >                 fprintf(stderr, "Usage: %s <infile>
> > [<outfile>]\n",argv[0]);
> > @@ -348,15 +384,26 @@ int main(int argc, char *argv[])
> > 
> >         /* Initialize the head of the linked list. */
> >         head = current =
> > (file_context_node_t*)malloc(sizeof(file_context_node_t));
> > +       if (!head) {
> > +               fprintf(stderr, "Error: failure allocating
> > memory.\n");
> > +               return 1;
> > +       }
> >         head->next = NULL;
> > +       head->path = NULL;
> > +       head->file_type = NULL;
> > +       head->context = NULL;
> > 
> >         /* Parse the file into a file_context linked list. */
> >         line_buf = NULL;
> > 
> > -       while ( getline(&line_buf, &buf_len, in_file) != -1 ){
> > +       while ( getline(&line_buf, NULL, in_file) != -1 ){
> >                 line_len = strlen(line_buf);
> > -               if( line_len == 0 || line_len == 1)
> > +               if( line_len == 0 || line_len == 1) {
> > +                       free(line_buf);
> > +                       line_buf = NULL;
> >                         continue;
> > +               }
> > +
> >                 /* Get rid of whitespace from the front of the
> > line. */
> >                 for (i = 0; i < line_len; i++) {
> >                         if (!isspace(line_buf[i]))
> > @@ -364,16 +411,25 @@ int main(int argc, char *argv[])
> >                 }
> > 
> > 
> > -               if (i >= line_len)
> > +               if (i >= line_len) {
> > +                       free(line_buf);
> > +                       line_buf = NULL;
> >                         continue;
> > +               }
> > +
> >                 /* Check if the line isn't empty and isn't a
> > comment */
> > -               if (line_buf[i] == '#')
> > +               if (line_buf[i] == '#') {
> > +                       free(line_buf);
> > +                       line_buf = NULL;
> >                         continue;
> > +               }
> > 
> >                 /* We have a valid line - allocate a new node. */
> >                 temp = (file_context_node_t
> > *)malloc(sizeof(file_context_node_t));
> >                 if (!temp) {
> > +                       free(line_buf);
> >                         fprintf(stderr, "Error: failure allocating
> > memory.\n");
> > +                       fc_free_file_context_node_list(head);
> >                         return 1;
> >                 }
> >                 temp->next = NULL;
> > @@ -393,8 +449,8 @@ int main(int argc, char *argv[])
> >                 if (regex_len == 0) {
> >                         file_context_node_destroy(temp);
> >                         free(temp);
> > -
> > -
> > +                       free(line_buf);
> > +                       line_buf = NULL;
> >                         continue;
> >                 }
> > 
> > @@ -402,7 +458,9 @@ int main(int argc, char *argv[])
> >                 if (!temp->path) {
> >                         file_context_node_destroy(temp);
> >                         free(temp);
> > +                       free(line_buf);
> >                         fprintf(stderr, "Error: failure allocating
> > memory.\n");
> > +                       fc_free_file_context_node_list(head);
> >                         return 1;
> >                 }
> > 
> > @@ -416,22 +474,29 @@ int main(int argc, char *argv[])
> >                 if (i == line_len) {
> >                         file_context_node_destroy(temp);
> >                         free(temp);
> > +                       free(line_buf);
> > +                       line_buf = NULL;
> >                         continue;
> >                 }
> > 
> >                 /* Parse out the type from the line (if it
> > -                       *  is there). */
> > +                *  is there). */
> >                 if (line_buf[i] == '-') {
> >                         temp->file_type = (char
> > *)malloc(sizeof(char) * 3);
> >                         if (!(temp->file_type)) {
> > +                               file_context_node_destroy(temp);
> > +                               free(temp);
> > +                               free(line_buf);
> >                                 fprintf(stderr, "Error: failure
> > allocating memory.\n");
> > +                               fc_free_file_context_node_list(head
> > );
> >                                 return 1;
> >                         }
> > 
> >                         if( i + 2 >= line_len ) {
> >                                 file_context_node_destroy(temp);
> >                                 free(temp);
> > -
> > +                               free(line_buf);
> > +                               line_buf = NULL;
> >                                 continue;
> >                         }
> > 
> > @@ -448,9 +513,10 @@ int main(int argc, char *argv[])
> >                         }
> > 
> >                         if (i == line_len) {
> > -
> >                                 file_context_node_destroy(temp);
> >                                 free(temp);
> > +                               free(line_buf);
> > +                               line_buf = NULL;
> >                                 continue;
> >                         }
> >                 }
> > @@ -467,21 +533,22 @@ int main(int argc, char *argv[])
> >                 if (!temp->context) {
> >                         file_context_node_destroy(temp);
> >                         free(temp);
> > +                       free(line_buf);
> >                         fprintf(stderr, "Error: failure allocating
> > memory.\n");
> > +                       fc_free_file_context_node_list(head);
> >                         return 1;
> >                 }
> > 
> >                 /* Set all the data about the regular
> > -                       *  expression. */
> > +                * expression. */
> >                 fc_fill_data(temp);
> > 
> >                 /* Link this line of code at the end of
> > -                       *  the linked list. */
> > +                * the linked list. */
> >                 current->next = temp;
> >                 current = current->next;
> >                 lines++;
> > 
> > -
> >                 free(line_buf);
> >                 line_buf = NULL;
> >         }
> > @@ -495,6 +562,7 @@ int main(int argc, char *argv[])
> >         if (!bcurrent) {
> >                 printf
> >                     ("Error: failure allocating memory.\n");
> > +               fc_free_file_context_node_list(head);
> >                 return -1;
> >         }
> >         bcurrent->next = NULL;
> > @@ -517,6 +585,8 @@ int main(int argc, char *argv[])
> >                         if (!(bcurrent->next)) {
> >                                 printf
> >                                     ("Error: failure allocating
> > memory.\n");
> > +                               fc_free_file_context_node_list(head
> > );
> > +                               fc_free_file_context_bucket_list(ma
> > ster);
> >                                 return -1;
> >                         }
> > 
> > @@ -536,6 +606,8 @@ int main(int argc, char *argv[])
> >         if (output_name) {
> >                 if (!(out_file = fopen(output_name, "w"))) {
> >                         printf("Error: failure opening output file
> > for write.\n");
> > +                       fc_free_file_context_node_list(head);
> > +                       fc_free_file_context_bucket_list(master);
> >                         return -1;
> >                 }
> >         } else {
> > @@ -556,15 +628,11 @@ int main(int argc, char *argv[])
> >                 /* Output the context. */
> >                 fprintf(out_file, "%s\n", current->context);
> > 
> > -               /* Remove the node. */
> > -               temp = current;
> >                 current = current->next;
> > -
> > -               file_context_node_destroy(temp);
> > -               free(temp);
> > -
> >         }
> > -       free(master);
> > +
> > +       fc_free_file_context_node_list(head);
> > +       fc_free_file_context_bucket_list(master);
> > 
> >         if (output_name) {
> >                 fclose(out_file);
> > _______________________________________________
> > refpolicy mailing list
> > refpolicy at oss.tresys.com
> > http://oss.tresys.com/mailman/listinfo/refpolicy
> 
> The PR was mostly to avoid warnings with a static analysis tool IIUC.
> I was trying to avoid any fork of that code between upstream and
> Google.
> 
> Do you plan on applying this change to the repo?
> 
> I've reached out to the google engineers here:
> https://android-review.googlesource.com/#/c/platform/system/sepolicy/
> +/465218/
> 
> and in the PR
> https://github.com/TresysTechnology/refpolicy/pull/125
> 
> So hopefully they can try it and let us know if it avoids the errors
> with their analysis tools,
> if valgrind reports 0 leaks, it likely will satisfy the static
> analysis.

The patch that I attached does not work properly and it needs further
work, because it leads to a failure in getline() and an empty output
file.

I hope it helps.

Regards,

Guido

[refpolicy] [PATCH] fc_sort: memory leakages (was: Pull Request for fixing memory leak warning)

[William Roberts]

On Fri, Sep 29, 2017 at 9:52 AM, Guido Trentalancia via refpolicy
<refpolicy@oss.tresys.com> wrote:
> On Thu, 28/09/2017 at 15.24 -0700, William Roberts wrote:
>> On Thu, Sep 28, 2017 at 5:12 PM, Guido Trentalancia via refpolicy
>> <refpolicy@oss.tresys.com> wrote:
>> > Hello.
>> >
>> > I have tried applying the changes you suggested, but valgrind still
>> > reports memory leakages in the fc_sort executable.
>> >
>> > So, I have created a simple patch which avoids valgrind errors
>> > completely and also carries out some extra memory consistency
>> > checks.
>> >
>> > The patch is attached below. Feel free to use it, if it proves
>> > useful.
>> >
>> > On Thu, 28/09/2017 at 09.05 -0700, William Roberts via
>> > refpolicy wrote:
>> > > Can we get someone to take a look at:
>> > > https://github.com/TresysTechnology/refpolicy/pull/125
>> > >
>> > > It essentially fixes an inaccurate static analysis warning on
>> > > fc_sort
>> > > and will get the Android tree back in-sync with upstream.
>> > >
>> >
>> > Avoid memory leakages in the fc_sort executable (now passes
>> > all valgrind tests fine).
>> >
>> > Some NULL pointer checks with or without associated error
>> > reporting.
>> >
>> > Some white space and comment formatting fixes.
>> >
>> > Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
>> > ---
>> >  support/fc_sort.c |  110
>> > +++++++++++++++++++++++++++++++++++++++++++-----------
>> >  1 file changed, 89 insertions(+), 21 deletions(-)
>> >
>> > --- a/support/fc_sort.c 2017-09-28 20:52:44.900145312 +0200
>> > +++ b/support/fc_sort.c 2017-09-29 01:38:53.600075122 +0200
>> > @@ -46,6 +46,9 @@ typedef struct file_context_node {
>> >
>> >  void file_context_node_destroy(file_context_node_t *x)
>> >  {
>> > +       if (!x)
>> > +               return;
>> > +
>> >         free(x->path);
>> >         free(x->file_type);
>> >         free(x->context);
>> > @@ -307,6 +310,40 @@ void fc_fill_data(file_context_node_t *f
>> >         }
>> >  }
>> >
>> > +
>> > +
>> > +/* fc_free_file_context_node_list
>> > + * Free the memory allocated to the linked list and its elements.
>> > + */
>> > +void fc_free_file_context_node_list(struct file_context_node
>> > *node)
>> > +{
>> > +       struct file_context_node *next;
>> > +
>> > +       while (node) {
>> > +               next = node->next;
>> > +               file_context_node_destroy(node);
>> > +               free(node);
>> > +               node = next;
>> > +       }
>> > +}
>> > +
>> > +
>> > +
>> > +/* fc_free_file_context_bucket_list
>> > + * Free the memory allocated to the linked list but not its
>> > elements
>> > + * as they are deallocated when freeing the old file context list.
>> > + */
>> > +void fc_free_file_context_bucket_list(struct file_context_bucket
>> > *element)
>> > +{
>> > +       struct file_context_bucket *next;
>> > +
>> > +       while (element) {
>> > +               next = element->next;
>> > +               free(element);
>> > +               element = next;
>> > +       }
>> > +}
>> > +
>> >  /* main
>> >   * This program takes in two arguments, the input filename and the
>> >   *  output filename. The input file should be syntactically
>> > correct.
>> > @@ -317,7 +354,7 @@ int main(int argc, char *argv[])
>> >  {
>> >         int lines;
>> >         size_t start, finish, regex_len, context_len;
>> > -       size_t line_len, buf_len, i;
>> > +       size_t line_len, i;
>> >         char *input_name, *output_name, *line_buf;
>> >
>> >         file_context_node_t *temp;
>> > @@ -328,7 +365,6 @@ int main(int argc, char *argv[])
>> >
>> >         FILE *in_file, *out_file;
>> >
>> > -
>> >         /* Check for the correct number of command line arguments.
>> > */
>> >         if (argc < 2 || argc > 3) {
>> >                 fprintf(stderr, "Usage: %s <infile>
>> > [<outfile>]\n",argv[0]);
>> > @@ -348,15 +384,26 @@ int main(int argc, char *argv[])
>> >
>> >         /* Initialize the head of the linked list. */
>> >         head = current =
>> > (file_context_node_t*)malloc(sizeof(file_context_node_t));
>> > +       if (!head) {
>> > +               fprintf(stderr, "Error: failure allocating
>> > memory.\n");
>> > +               return 1;
>> > +       }
>> >         head->next = NULL;
>> > +       head->path = NULL;
>> > +       head->file_type = NULL;
>> > +       head->context = NULL;
>> >
>> >         /* Parse the file into a file_context linked list. */
>> >         line_buf = NULL;
>> >
>> > -       while ( getline(&line_buf, &buf_len, in_file) != -1 ){
>> > +       while ( getline(&line_buf, NULL, in_file) != -1 ){
>> >                 line_len = strlen(line_buf);
>> > -               if( line_len == 0 || line_len == 1)
>> > +               if( line_len == 0 || line_len == 1) {
>> > +                       free(line_buf);
>> > +                       line_buf = NULL;
>> >                         continue;
>> > +               }
>> > +
>> >                 /* Get rid of whitespace from the front of the
>> > line. */
>> >                 for (i = 0; i < line_len; i++) {
>> >                         if (!isspace(line_buf[i]))
>> > @@ -364,16 +411,25 @@ int main(int argc, char *argv[])
>> >                 }
>> >
>> >
>> > -               if (i >= line_len)
>> > +               if (i >= line_len) {
>> > +                       free(line_buf);
>> > +                       line_buf = NULL;
>> >                         continue;
>> > +               }
>> > +
>> >                 /* Check if the line isn't empty and isn't a
>> > comment */
>> > -               if (line_buf[i] == '#')
>> > +               if (line_buf[i] == '#') {
>> > +                       free(line_buf);
>> > +                       line_buf = NULL;
>> >                         continue;
>> > +               }
>> >
>> >                 /* We have a valid line - allocate a new node. */
>> >                 temp = (file_context_node_t
>> > *)malloc(sizeof(file_context_node_t));
>> >                 if (!temp) {
>> > +                       free(line_buf);
>> >                         fprintf(stderr, "Error: failure allocating
>> > memory.\n");
>> > +                       fc_free_file_context_node_list(head);
>> >                         return 1;
>> >                 }
>> >                 temp->next = NULL;
>> > @@ -393,8 +449,8 @@ int main(int argc, char *argv[])
>> >                 if (regex_len == 0) {
>> >                         file_context_node_destroy(temp);
>> >                         free(temp);
>> > -
>> > -
>> > +                       free(line_buf);
>> > +                       line_buf = NULL;
>> >                         continue;
>> >                 }
>> >
>> > @@ -402,7 +458,9 @@ int main(int argc, char *argv[])
>> >                 if (!temp->path) {
>> >                         file_context_node_destroy(temp);
>> >                         free(temp);
>> > +                       free(line_buf);
>> >                         fprintf(stderr, "Error: failure allocating
>> > memory.\n");
>> > +                       fc_free_file_context_node_list(head);
>> >                         return 1;
>> >                 }
>> >
>> > @@ -416,22 +474,29 @@ int main(int argc, char *argv[])
>> >                 if (i == line_len) {
>> >                         file_context_node_destroy(temp);
>> >                         free(temp);
>> > +                       free(line_buf);
>> > +                       line_buf = NULL;
>> >                         continue;
>> >                 }
>> >
>> >                 /* Parse out the type from the line (if it
>> > -                       *  is there). */
>> > +                *  is there). */
>> >                 if (line_buf[i] == '-') {
>> >                         temp->file_type = (char
>> > *)malloc(sizeof(char) * 3);
>> >                         if (!(temp->file_type)) {
>> > +                               file_context_node_destroy(temp);
>> > +                               free(temp);
>> > +                               free(line_buf);
>> >                                 fprintf(stderr, "Error: failure
>> > allocating memory.\n");
>> > +                               fc_free_file_context_node_list(head
>> > );
>> >                                 return 1;
>> >                         }
>> >
>> >                         if( i + 2 >= line_len ) {
>> >                                 file_context_node_destroy(temp);
>> >                                 free(temp);
>> > -
>> > +                               free(line_buf);
>> > +                               line_buf = NULL;
>> >                                 continue;
>> >                         }
>> >
>> > @@ -448,9 +513,10 @@ int main(int argc, char *argv[])
>> >                         }
>> >
>> >                         if (i == line_len) {
>> > -
>> >                                 file_context_node_destroy(temp);
>> >                                 free(temp);
>> > +                               free(line_buf);
>> > +                               line_buf = NULL;
>> >                                 continue;
>> >                         }
>> >                 }
>> > @@ -467,21 +533,22 @@ int main(int argc, char *argv[])
>> >                 if (!temp->context) {
>> >                         file_context_node_destroy(temp);
>> >                         free(temp);
>> > +                       free(line_buf);
>> >                         fprintf(stderr, "Error: failure allocating
>> > memory.\n");
>> > +                       fc_free_file_context_node_list(head);
>> >                         return 1;
>> >                 }
>> >
>> >                 /* Set all the data about the regular
>> > -                       *  expression. */
>> > +                * expression. */
>> >                 fc_fill_data(temp);
>> >
>> >                 /* Link this line of code at the end of
>> > -                       *  the linked list. */
>> > +                * the linked list. */
>> >                 current->next = temp;
>> >                 current = current->next;
>> >                 lines++;
>> >
>> > -
>> >                 free(line_buf);
>> >                 line_buf = NULL;
>> >         }
>> > @@ -495,6 +562,7 @@ int main(int argc, char *argv[])
>> >         if (!bcurrent) {
>> >                 printf
>> >                     ("Error: failure allocating memory.\n");
>> > +               fc_free_file_context_node_list(head);
>> >                 return -1;
>> >         }
>> >         bcurrent->next = NULL;
>> > @@ -517,6 +585,8 @@ int main(int argc, char *argv[])
>> >                         if (!(bcurrent->next)) {
>> >                                 printf
>> >                                     ("Error: failure allocating
>> > memory.\n");
>> > +                               fc_free_file_context_node_list(head
>> > );
>> > +                               fc_free_file_context_bucket_list(ma
>> > ster);
>> >                                 return -1;
>> >                         }
>> >
>> > @@ -536,6 +606,8 @@ int main(int argc, char *argv[])
>> >         if (output_name) {
>> >                 if (!(out_file = fopen(output_name, "w"))) {
>> >                         printf("Error: failure opening output file
>> > for write.\n");
>> > +                       fc_free_file_context_node_list(head);
>> > +                       fc_free_file_context_bucket_list(master);
>> >                         return -1;
>> >                 }
>> >         } else {
>> > @@ -556,15 +628,11 @@ int main(int argc, char *argv[])
>> >                 /* Output the context. */
>> >                 fprintf(out_file, "%s\n", current->context);
>> >
>> > -               /* Remove the node. */
>> > -               temp = current;
>> >                 current = current->next;
>> > -
>> > -               file_context_node_destroy(temp);
>> > -               free(temp);
>> > -
>> >         }
>> > -       free(master);
>> > +
>> > +       fc_free_file_context_node_list(head);
>> > +       fc_free_file_context_bucket_list(master);
>> >
>> >         if (output_name) {
>> >                 fclose(out_file);
>> > _______________________________________________
>> > refpolicy mailing list
>> > refpolicy at oss.tresys.com
>> > http://oss.tresys.com/mailman/listinfo/refpolicy
>>
>> The PR was mostly to avoid warnings with a static analysis tool IIUC.
>> I was trying to avoid any fork of that code between upstream and
>> Google.
>>
>> Do you plan on applying this change to the repo?
>>
>> I've reached out to the google engineers here:
>> https://android-review.googlesource.com/#/c/platform/system/sepolicy/
>> +/465218/
>>
>> and in the PR
>> https://github.com/TresysTechnology/refpolicy/pull/125
>>
>> So hopefully they can try it and let us know if it avoids the errors
>> with their analysis tools,
>> if valgrind reports 0 leaks, it likely will satisfy the static
>> analysis.
>
> The patch that I attached does not work properly and it needs further
> work, because it leads to a failure in getline() and an empty output
> file.
>
> I hope it helps.

That's not very helpful IMHO, but thanks anyways. They took the PR
upstream, so that should fix there static analysis issue and avoid any
forking.

>
> Regards,
>
> Guido
> _______________________________________________
> refpolicy mailing list
> refpolicy at oss.tresys.com
> http://oss.tresys.com/mailman/listinfo/refpolicy



-- 
Respectfully,

William C Roberts

[refpolicy] [PATCH] fc_sort: memory leakages (was: Pull Request for fixing memory leak warning)

[Guido Trentalancia]

On Fri, 29/09/2017 at 09.30 -0700, William Roberts wrote:
> On Fri, Sep 29, 2017 at 9:52 AM, Guido Trentalancia via refpolicy
> <refpolicy@oss.tresys.com> wrote:
> > On Thu, 28/09/2017 at 15.24 -0700, William Roberts wrote:
> > > On Thu, Sep 28, 2017 at 5:12 PM, Guido Trentalancia via refpolicy
> > > <refpolicy@oss.tresys.com> wrote:

[...]

> > > So hopefully they can try it and let us know if it avoids the
> > > errors
> > > with their analysis tools,
> > > if valgrind reports 0 leaks, it likely will satisfy the static
> > > analysis.
> > 
> > The patch that I attached does not work properly and it needs
> > further
> > work, because it leads to a failure in getline() and an empty
> > output
> > file.
> > 
> > I hope it helps.
> 
> That's not very helpful IMHO, but thanks anyways. They took the PR
> upstream, so that should fix there static analysis issue and avoid
> any
> forking.

Yes, I know it's not particularly helpful. I have posted mainly because
the previous one was broken.

At the moment, I don't have time to look at it any further. Also, I do
not use the same analyzer that you have used, so I cannot reproduce the
same problem.

Regards,

Guido

[refpolicy] [PATCH] fc_sort: memory leakages (was: Pull Request for fixing memory leak warning)

[William Roberts]

On Fri, Sep 29, 2017 at 11:43 AM, Guido Trentalancia via refpolicy
<refpolicy@oss.tresys.com> wrote:
> On Fri, 29/09/2017 at 09.30 -0700, William Roberts wrote:
>> On Fri, Sep 29, 2017 at 9:52 AM, Guido Trentalancia via refpolicy
>> <refpolicy@oss.tresys.com> wrote:
>> > On Thu, 28/09/2017 at 15.24 -0700, William Roberts wrote:
>> > > On Thu, Sep 28, 2017 at 5:12 PM, Guido Trentalancia via refpolicy
>> > > <refpolicy@oss.tresys.com> wrote:
>
> [...]
>
>> > > So hopefully they can try it and let us know if it avoids the
>> > > errors
>> > > with their analysis tools,
>> > > if valgrind reports 0 leaks, it likely will satisfy the static
>> > > analysis.
>> >
>> > The patch that I attached does not work properly and it needs
>> > further
>> > work, because it leads to a failure in getline() and an empty
>> > output
>> > file.
>> >
>> > I hope it helps.
>>
>> That's not very helpful IMHO, but thanks anyways. They took the PR
>> upstream, so that should fix there static analysis issue and avoid
>> any
>> forking.
>
> Yes, I know it's not particularly helpful. I have posted mainly because
> the previous one was broken.
>
> At the moment, I don't have time to look at it any further. Also, I do
> not use the same analyzer that you have used, so I cannot reproduce the
> same problem.

Sure I get that. Actually it wasn't me that discovered the bug, I am just
middle manning something for others.

>
> Regards,
>
> Guido
> _______________________________________________
> refpolicy mailing list
> refpolicy at oss.tresys.com
> http://oss.tresys.com/mailman/listinfo/refpolicy



-- 
Respectfully,

William C Roberts

[refpolicy] [PATCH] fc_sort: memory leakages (was: Pull Request for fixing memory leak warning)

[Guido Trentalancia]

On Fri, 29/09/2017 at 09.30 -0700, William Roberts write:
> On Fri, Sep 29, 2017 at 9:52 AM, Guido Trentalancia via refpolicy
> <refpolicy@oss.tresys.com> wrote:
> > On Thu, 28/09/2017 at 15.24 -0700, William Roberts wrote:
> > > On Thu, Sep 28, 2017 at 5:12 PM, Guido Trentalancia via refpolicy
> > > <refpolicy@oss.tresys.com> wrote:

[...]

> > > The PR was mostly to avoid warnings with a static analysis tool
> > > IIUC.
> > > I was trying to avoid any fork of that code between upstream and
> > > Google.
> > > 
> > > Do you plan on applying this change to the repo?
> > > 
> > > I've reached out to the google engineers here:
> > > https://android-review.googlesource.com/#/c/platform/system/sepol
> > > icy/
> > > +/465218/
> > > 
> > > and in the PR
> > > https://github.com/TresysTechnology/refpolicy/pull/125
> > > 
> > > So hopefully they can try it and let us know if it avoids the
> > > errors
> > > with their analysis tools,
> > > if valgrind reports 0 leaks, it likely will satisfy the static
> > > analysis.
> > 
> > The patch that I attached does not work properly and it needs
> > further
> > work, because it leads to a failure in getline() and an empty
> > output
> > file.
> > 
> > I hope it helps.
> 
> That's not very helpful IMHO, but thanks anyways. They took the PR
> upstream, so that should fix there static analysis issue and avoid
> any
> forking.

I found some more time to look at this and the result is that I have
prepared a new patch version (v3), which should fix all problems
without introducing bugs.

The v3 patch passes all valgrind tests and also produces the expected
output file.

Regards,

Guido

[refpolicy] [PATCH v2] fc_sort: memory leakages

[Guido Trentalancia]

Might avoid memory leakages in the fc_sort executable in
particular it should take more care in error conditions.

Some NULL pointer checks with or without associated error
reporting.

Some white space and comment formatting fixes.

The previous version of this patch is invalid as it causes
a failure in getline() calls and hence produces empty
output files.

Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
---
 support/fc_sort.c |   85 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 72 insertions(+), 13 deletions(-)

--- a/support/fc_sort.c	2017-09-29 19:01:28.012455758 +0200
+++ b/support/fc_sort.c	2017-09-29 20:26:39.438434861 +0200
@@ -46,6 +46,9 @@ typedef struct file_context_node {
 
 void file_context_node_destroy(file_context_node_t *x)
 {
+	if (!x)
+		return;
+
 	free(x->path);
 	free(x->file_type);
 	free(x->context);
@@ -307,6 +310,25 @@ void fc_fill_data(file_context_node_t *f
 	}
 }
 
+
+
+/* fc_free_file_context_node_list
+ * Free the memory allocated to the linked list and its elements.
+ */
+void fc_free_file_context_node_list(struct file_context_node *node)
+{
+	struct file_context_node *next;
+
+	while (node) {
+		next = node->next;
+		file_context_node_destroy(node);
+		free(node);
+		node = next;
+	}
+}
+
+
+
 /* main
  * This program takes in two arguments, the input filename and the
  *  output filename. The input file should be syntactically correct.
@@ -328,7 +350,6 @@ int main(int argc, char *argv[])
 
 	FILE *in_file, *out_file;
 
-
 	/* Check for the correct number of command line arguments. */
 	if (argc < 2 || argc > 3) {
 		fprintf(stderr, "Usage: %s <infile> [<outfile>]\n",argv[0]);
@@ -348,15 +369,26 @@ int main(int argc, char *argv[])
 
 	/* Initialize the head of the linked list. */
 	head = current = (file_context_node_t*)malloc(sizeof(file_context_node_t));
+	if (!head) {
+		fprintf(stderr, "Error: failure allocating memory.\n");
+		return 1;
+	}
 	head->next = NULL;
+	head->path = NULL;
+	head->file_type = NULL;
+	head->context = NULL;
 
 	/* Parse the file into a file_context linked list. */
 	line_buf = NULL;
 
 	while ( getline(&line_buf, &buf_len, in_file) != -1 ){
 		line_len = strlen(line_buf);
-		if( line_len == 0 || line_len == 1)
+		if( line_len == 0 || line_len == 1) {
+			free(line_buf);
+			line_buf = NULL;
 			continue;
+		}
+
 		/* Get rid of whitespace from the front of the line. */
 		for (i = 0; i < line_len; i++) {
 			if (!isspace(line_buf[i]))
@@ -364,16 +396,25 @@ int main(int argc, char *argv[])
 		}
 
 
-		if (i >= line_len)
+		if (i >= line_len) {
+			free(line_buf);
+			line_buf = NULL;
 			continue;
+		}
+
 		/* Check if the line isn't empty and isn't a comment */
-		if (line_buf[i] == '#')
+		if (line_buf[i] == '#') {
+			free(line_buf);
+			line_buf = NULL;
 			continue;
+		}
 
 		/* We have a valid line - allocate a new node. */
 		temp = (file_context_node_t *)malloc(sizeof(file_context_node_t));
 		if (!temp) {
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 		temp->next = NULL;
@@ -393,8 +434,8 @@ int main(int argc, char *argv[])
 		if (regex_len == 0) {
 			file_context_node_destroy(temp);
 			free(temp);
-
-
+			free(line_buf);
+			line_buf = NULL;
 			continue;
 		}
 
@@ -402,7 +443,9 @@ int main(int argc, char *argv[])
 		if (!temp->path) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 
@@ -416,22 +459,29 @@ int main(int argc, char *argv[])
 		if (i == line_len) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
+			line_buf = NULL;
 			continue;
 		}
 
 		/* Parse out the type from the line (if it
-			*  is there). */
+		 *  is there). */
 		if (line_buf[i] == '-') {
 			temp->file_type = (char *)malloc(sizeof(char) * 3);
 			if (!(temp->file_type)) {
+				file_context_node_destroy(temp);
+				free(temp);
+				free(line_buf);
 				fprintf(stderr, "Error: failure allocating memory.\n");
+				fc_free_file_context_node_list(head);
 				return 1;
 			}
 
 			if( i + 2 >= line_len ) {
 				file_context_node_destroy(temp);
 				free(temp);
-
+				free(line_buf);
+				line_buf = NULL;
 				continue;
 			}
 
@@ -448,9 +498,10 @@ int main(int argc, char *argv[])
 			}
 
 			if (i == line_len) {
-
 				file_context_node_destroy(temp);
 				free(temp);
+				free(line_buf);
+				line_buf = NULL;
 				continue;
 			}
 		}
@@ -467,21 +518,22 @@ int main(int argc, char *argv[])
 		if (!temp->context) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 
 		/* Set all the data about the regular
-			*  expression. */
+		 * expression. */
 		fc_fill_data(temp);
 
 		/* Link this line of code at the end of
-			*  the linked list. */
+		 * the linked list. */
 		current->next = temp;
 		current = current->next;
 		lines++;
 
-
 		free(line_buf);
 		line_buf = NULL;
 	}
@@ -492,6 +544,12 @@ int main(int argc, char *argv[])
 	bcurrent = master =
 	    (file_context_bucket_t *)
 	    malloc(sizeof(file_context_bucket_t));
+	if (!bcurrent) {
+		printf
+		    ("Error: failure allocating memory.\n"); 
+		fc_free_file_context_node_list(head);
+		return -1;
+	}
 	bcurrent->next = NULL;
 	bcurrent->data = NULL;
 
@@ -512,6 +570,7 @@ int main(int argc, char *argv[])
 			if (!(bcurrent->next)) {
 				printf
 				    ("Error: failure allocating memory.\n");
+				fc_free_file_context_node_list(head);
 				exit(-1);
 			}
 
@@ -531,6 +590,7 @@ int main(int argc, char *argv[])
 	if (output_name) {
 		if (!(out_file = fopen(output_name, "w"))) {
 			printf("Error: failure opening output file for write.\n");
+			fc_free_file_context_node_list(head);
 			return -1;
 		}
 	} else {
@@ -557,7 +617,6 @@ int main(int argc, char *argv[])
 
 		file_context_node_destroy(temp);
 		free(temp);
-
 	}
 	free(master);
 

[refpolicy] [PATCH v3] fc_sort: memory leakages

[Guido Trentalancia]

Avoid memory leakages in the fc_sort executable (now passes
all valgrind tests fine).

Some NULL pointer checks with or without associated error
reporting.

Some white space and comment formatting fixes.

This is the third version of this patch. Please do not use
the first version as it introduces a serious bug.

Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
---
 support/fc_sort.c |  137 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 117 insertions(+), 20 deletions(-)

--- a/support/fc_sort.c	2017-09-29 19:01:28.012455758 +0200
+++ b/support/fc_sort.c	2017-09-30 01:21:59.316362417 +0200
@@ -46,6 +46,9 @@ typedef struct file_context_node {
 
 void file_context_node_destroy(file_context_node_t *x)
 {
+	if (!x)
+		return;
+
 	free(x->path);
 	free(x->file_type);
 	free(x->context);
@@ -307,6 +310,41 @@ void fc_fill_data(file_context_node_t *f
 	}
 }
 
+
+
+/* fc_free_file_context_node_list
+ * Free the memory allocated to the linked list and its elements.
+ */
+void fc_free_file_context_node_list(struct file_context_node *node)
+{
+	struct file_context_node *next;
+
+	while (node) {
+		next = node->next;
+		file_context_node_destroy(node);
+		free(node);
+		node = next;
+	}
+}
+
+
+
+/* fc_free_file_context_bucket_list
+ * Free the memory allocated to the linked list and its elements.
+ */
+void fc_free_file_context_bucket_list(struct file_context_bucket *element)
+{
+	struct file_context_bucket *next;
+
+	while (element) {
+		next = element->next;
+		file_context_node_destroy(element->data);
+		free(element->data);
+		free(element);
+		element = next;
+	}
+}
+
 /* main
  * This program takes in two arguments, the input filename and the
  *  output filename. The input file should be syntactically correct.
@@ -328,7 +366,6 @@ int main(int argc, char *argv[])
 
 	FILE *in_file, *out_file;
 
-
 	/* Check for the correct number of command line arguments. */
 	if (argc < 2 || argc > 3) {
 		fprintf(stderr, "Usage: %s <infile> [<outfile>]\n",argv[0]);
@@ -348,15 +385,26 @@ int main(int argc, char *argv[])
 
 	/* Initialize the head of the linked list. */
 	head = current = (file_context_node_t*)malloc(sizeof(file_context_node_t));
+	if (!head) {
+		fprintf(stderr, "Error: failure allocating memory.\n");
+		return 1;
+	}
 	head->next = NULL;
+	head->path = NULL;
+	head->file_type = NULL;
+	head->context = NULL;
 
 	/* Parse the file into a file_context linked list. */
 	line_buf = NULL;
 
 	while ( getline(&line_buf, &buf_len, in_file) != -1 ){
 		line_len = strlen(line_buf);
-		if( line_len == 0 || line_len == 1)
+		if( line_len == 0 || line_len == 1) {
+			free(line_buf);
+			line_buf = NULL;
 			continue;
+		}
+
 		/* Get rid of whitespace from the front of the line. */
 		for (i = 0; i < line_len; i++) {
 			if (!isspace(line_buf[i]))
@@ -364,16 +412,25 @@ int main(int argc, char *argv[])
 		}
 
 
-		if (i >= line_len)
+		if (i >= line_len) {
+			free(line_buf);
+			line_buf = NULL;
 			continue;
+		}
+
 		/* Check if the line isn't empty and isn't a comment */
-		if (line_buf[i] == '#')
+		if (line_buf[i] == '#') {
+			free(line_buf);
+			line_buf = NULL;
 			continue;
+		}
 
 		/* We have a valid line - allocate a new node. */
 		temp = (file_context_node_t *)malloc(sizeof(file_context_node_t));
 		if (!temp) {
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 		temp->next = NULL;
@@ -393,8 +450,8 @@ int main(int argc, char *argv[])
 		if (regex_len == 0) {
 			file_context_node_destroy(temp);
 			free(temp);
-
-
+			free(line_buf);
+			line_buf = NULL;
 			continue;
 		}
 
@@ -402,7 +459,9 @@ int main(int argc, char *argv[])
 		if (!temp->path) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 
@@ -416,22 +475,29 @@ int main(int argc, char *argv[])
 		if (i == line_len) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
+			line_buf = NULL;
 			continue;
 		}
 
 		/* Parse out the type from the line (if it
-			*  is there). */
+		 *  is there). */
 		if (line_buf[i] == '-') {
 			temp->file_type = (char *)malloc(sizeof(char) * 3);
 			if (!(temp->file_type)) {
+				file_context_node_destroy(temp);
+				free(temp);
+				free(line_buf);
 				fprintf(stderr, "Error: failure allocating memory.\n");
+				fc_free_file_context_node_list(head);
 				return 1;
 			}
 
 			if( i + 2 >= line_len ) {
 				file_context_node_destroy(temp);
 				free(temp);
-
+				free(line_buf);
+				line_buf = NULL;
 				continue;
 			}
 
@@ -448,9 +514,10 @@ int main(int argc, char *argv[])
 			}
 
 			if (i == line_len) {
-
 				file_context_node_destroy(temp);
 				free(temp);
+				free(line_buf);
+				line_buf = NULL;
 				continue;
 			}
 		}
@@ -467,24 +534,26 @@ int main(int argc, char *argv[])
 		if (!temp->context) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 
 		/* Set all the data about the regular
-			*  expression. */
+		 * expression. */
 		fc_fill_data(temp);
 
 		/* Link this line of code at the end of
-			*  the linked list. */
+		 * the linked list. */
 		current->next = temp;
 		current = current->next;
 		lines++;
 
-
 		free(line_buf);
 		line_buf = NULL;
 	}
+	free(line_buf);
 	fclose(in_file);
 
 	/* Create the bucket linked list from the earlier linked list. */
@@ -492,13 +561,41 @@ int main(int argc, char *argv[])
 	bcurrent = master =
 	    (file_context_bucket_t *)
 	    malloc(sizeof(file_context_bucket_t));
+	if (!bcurrent) {
+		printf
+		    ("Error: failure allocating memory.\n"); 
+		fc_free_file_context_node_list(head);
+		return -1;
+	}
 	bcurrent->next = NULL;
 	bcurrent->data = NULL;
 
 	/* Go until all the nodes have been put in individual buckets. */
 	while (current) {
+		bcurrent->data = (file_context_node_t *)malloc(sizeof(file_context_node_t));
+		if (!(bcurrent->data)) {
+			printf
+			    ("Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
+			fc_free_file_context_bucket_list(master);
+		}
 		/* Copy over the file context line into the bucket. */
-		bcurrent->data = current;
+		if (current->path)
+			bcurrent->data->path = strdup(current->path);
+		else
+			bcurrent->data->path = NULL;
+		if (current->file_type)
+			bcurrent->data->file_type = strdup(current->file_type);
+		else
+			bcurrent->data->file_type = NULL;
+		if (current->context)
+			bcurrent->data->context = strdup(current->context);
+		else
+			bcurrent->data->context = NULL;
+		bcurrent->data->meta = current->meta;
+		bcurrent->data->stem_len = current->stem_len;
+		bcurrent->data->str_len = current->str_len;
+
 		current = current->next;
 
 		/* Detach the node in the bucket from the old list. */
@@ -512,6 +609,8 @@ int main(int argc, char *argv[])
 			if (!(bcurrent->next)) {
 				printf
 				    ("Error: failure allocating memory.\n");
+				fc_free_file_context_node_list(head);
+				fc_free_file_context_bucket_list(master);
 				exit(-1);
 			}
 
@@ -521,7 +620,6 @@ int main(int argc, char *argv[])
 
 			bcurrent = bcurrent->next;
 		}
-
 	}
 
 	/* Sort the bucket list. */
@@ -531,6 +629,7 @@ int main(int argc, char *argv[])
 	if (output_name) {
 		if (!(out_file = fopen(output_name, "w"))) {
 			printf("Error: failure opening output file for write.\n");
+			fc_free_file_context_node_list(head);
 			return -1;
 		}
 	} else {
@@ -539,6 +638,7 @@ int main(int argc, char *argv[])
 
 	/* Output the sorted file_context linked list to the output file. */
 	current = master->data;
+
 	while (current) {
 		/* Output the path. */
 		fprintf(out_file, "%s\t\t", current->path);
@@ -551,14 +651,11 @@ int main(int argc, char *argv[])
 		/* Output the context. */
 		fprintf(out_file, "%s\n", current->context);
 
-		/* Remove the node. */
-		temp = current;
 		current = current->next;
-
-		file_context_node_destroy(temp);
-		free(temp);
-
 	}
+
+	fc_free_file_context_node_list(head);
+	fc_free_file_context_node_list(master->data);
 	free(master);
 
 	if (output_name) {

[refpolicy] [PATCH v4] fc_sort: memory leakages

[Guido Trentalancia]

Avoid memory leakages in the fc_sort executable (now passes
all valgrind tests fine).

Some NULL pointer checks with or without associated error
reporting.

Some white space and comment formatting fixes.

Optimization: avoid unnecessary operations (unnecessary
memory allocation/deallocation and list copying).

Reverts 7821eb6f37d785ab6ac3bbdc39282c799ad22393 as there
are good chances that it is no longer needed, given that
all memory leakages should now be fixed.

This is the fourth version of this patch. Please do not use
the first version as it introduces a serious bug.

Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
---
 support/fc_sort.c |  102 +++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 78 insertions(+), 24 deletions(-)

--- a/support/fc_sort.c	2017-09-30 02:44:18.137342226 +0200
+++ b/support/fc_sort.c	2017-09-30 07:47:46.141267786 +0200
@@ -46,6 +46,9 @@ typedef struct file_context_node {
 
 void file_context_node_destroy(file_context_node_t *x)
 {
+	if (!x)
+		return;
+
 	free(x->path);
 	free(x->file_type);
 	free(x->context);
@@ -307,6 +310,41 @@ void fc_fill_data(file_context_node_t *f
 	}
 }
 
+
+
+/* fc_free_file_context_node_list
+ * Free the memory allocated to the linked list and its elements.
+ */
+void fc_free_file_context_node_list(struct file_context_node *node)
+{
+	struct file_context_node *next;
+
+	while (node) {
+		next = node->next;
+		file_context_node_destroy(node);
+		free(node);
+		node = next;
+	}
+}
+
+
+
+/* fc_free_file_context_bucket_list
+ * Free the memory allocated to the linked list and its elements.
+ */
+void fc_free_file_context_bucket_list(struct file_context_bucket *element)
+{
+	struct file_context_bucket *next;
+
+	while (element) {
+		next = element->next;
+		file_context_node_destroy(element->data);
+		free(element->data);
+		free(element);
+		element = next;
+	}
+}
+
 /* main
  * This program takes in two arguments, the input filename and the
  *  output filename. The input file should be syntactically correct.
@@ -328,7 +366,6 @@ int main(int argc, char *argv[])
 
 	FILE *in_file, *out_file;
 
-
 	/* Check for the correct number of command line arguments. */
 	if (argc < 2 || argc > 3) {
 		fprintf(stderr, "Usage: %s <infile> [<outfile>]\n",argv[0]);
@@ -348,24 +385,33 @@ int main(int argc, char *argv[])
 
 	/* Initialize the head of the linked list. */
 	head = current = (file_context_node_t*)malloc(sizeof(file_context_node_t));
+	if (!head) {
+		fprintf(stderr, "Error: failure allocating memory.\n");
+		return 1;
+	}
 	head->next = NULL;
+	head->path = NULL;
+	head->file_type = NULL;
+	head->context = NULL;
 
 	/* Parse the file into a file_context linked list. */
 	line_buf = NULL;
 
 	while ( getline(&line_buf, &buf_len, in_file) != -1 ){
 		line_len = strlen(line_buf);
+
 		if( line_len == 0 || line_len == 1)
 			continue;
+
 		/* Get rid of whitespace from the front of the line. */
 		for (i = 0; i < line_len; i++) {
 			if (!isspace(line_buf[i]))
 				break;
 		}
 
-
 		if (i >= line_len)
 			continue;
+
 		/* Check if the line isn't empty and isn't a comment */
 		if (line_buf[i] == '#')
 			continue;
@@ -373,7 +419,9 @@ int main(int argc, char *argv[])
 		/* We have a valid line - allocate a new node. */
 		temp = (file_context_node_t *)malloc(sizeof(file_context_node_t));
 		if (!temp) {
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 		temp->next = NULL;
@@ -382,19 +430,15 @@ int main(int argc, char *argv[])
 		/* Parse out the regular expression from the line. */
 		start = i;
 
-
 		while (i < line_len && (!isspace(line_buf[i])))
 			i++;
 		finish = i;
 
-
 		regex_len = finish - start;
 
 		if (regex_len == 0) {
 			file_context_node_destroy(temp);
 			free(temp);
-
-
 			continue;
 		}
 
@@ -402,13 +446,14 @@ int main(int argc, char *argv[])
 		if (!temp->path) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 
 		/* Get rid of whitespace after the regular expression. */
 		for (; i < line_len; i++) {
-
 			if (!isspace(line_buf[i]))
 				break;
 		}
@@ -420,18 +465,21 @@ int main(int argc, char *argv[])
 		}
 
 		/* Parse out the type from the line (if it
-			*  is there). */
+		 *  is there). */
 		if (line_buf[i] == '-') {
 			temp->file_type = (char *)malloc(sizeof(char) * 3);
 			if (!(temp->file_type)) {
+				file_context_node_destroy(temp);
+				free(temp);
+				free(line_buf);
 				fprintf(stderr, "Error: failure allocating memory.\n");
+				fc_free_file_context_node_list(head);
 				return 1;
 			}
 
 			if( i + 2 >= line_len ) {
 				file_context_node_destroy(temp);
 				free(temp);
-
 				continue;
 			}
 
@@ -448,7 +496,6 @@ int main(int argc, char *argv[])
 			}
 
 			if (i == line_len) {
-
 				file_context_node_destroy(temp);
 				free(temp);
 				continue;
@@ -467,24 +514,23 @@ int main(int argc, char *argv[])
 		if (!temp->context) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 
 		/* Set all the data about the regular
-			*  expression. */
+		 * expression. */
 		fc_fill_data(temp);
 
 		/* Link this line of code at the end of
-			*  the linked list. */
+		 * the linked list. */
 		current->next = temp;
 		current = current->next;
 		lines++;
-
-
-		free(line_buf);
-		line_buf = NULL;
 	}
+	free(line_buf);
 	fclose(in_file);
 
 	/* Create the bucket linked list from the earlier linked list. */
@@ -492,6 +538,12 @@ int main(int argc, char *argv[])
 	bcurrent = master =
 	    (file_context_bucket_t *)
 	    malloc(sizeof(file_context_bucket_t));
+	if (!bcurrent) {
+		printf
+		    ("Error: failure allocating memory.\n"); 
+		fc_free_file_context_node_list(head);
+		return -1;
+	}
 	bcurrent->next = NULL;
 	bcurrent->data = NULL;
 
@@ -512,7 +564,9 @@ int main(int argc, char *argv[])
 			if (!(bcurrent->next)) {
 				printf
 				    ("Error: failure allocating memory.\n");
-				exit(-1);
+				free(head);
+				fc_free_file_context_bucket_list(master);
+				return -1;
 			}
 
 			/* Make sure the new bucket thinks it's the end of the
@@ -521,16 +575,19 @@ int main(int argc, char *argv[])
 
 			bcurrent = bcurrent->next;
 		}
-
 	}
 
 	/* Sort the bucket list. */
 	fc_merge_sort(master);
 
+	free(head);
+
 	/* Open the output file. */
 	if (output_name) {
 		if (!(out_file = fopen(output_name, "w"))) {
 			printf("Error: failure opening output file for write.\n");
+			fc_free_file_context_node_list(master->data);
+			free(master);
 			return -1;
 		}
 	} else {
@@ -539,6 +596,7 @@ int main(int argc, char *argv[])
 
 	/* Output the sorted file_context linked list to the output file. */
 	current = master->data;
+
 	while (current) {
 		/* Output the path. */
 		fprintf(out_file, "%s\t\t", current->path);
@@ -551,14 +609,10 @@ int main(int argc, char *argv[])
 		/* Output the context. */
 		fprintf(out_file, "%s\n", current->context);
 
-		/* Remove the node. */
-		temp = current;
 		current = current->next;
-
-		file_context_node_destroy(temp);
-		free(temp);
-
 	}
+
+	fc_free_file_context_node_list(master->data);
 	free(master);
 
 	if (output_name) {

[refpolicy] [PATCH v4] fc_sort: memory leakages

[Guido Trentalancia]

Hello again.

I have prepared a new version (v5) of this patch, which further
improves the previous ones.

I would like to test it with the LLVM analyzer, that I am now trying to
install, to see if it actually solves the original problems and then I
can post the new patch here...

Regards,

Guido

[refpolicy] [PATCH v5] fc_sort: memory leakages

[Guido Trentalancia]

Avoid memory leakages in the fc_sort executable (now passes
all valgrind AND Clang static analyzer tests fine).

Some NULL pointer checks with or without associated error
reporting.

Some white space and comment formatting fixes.

Optimization: avoid unnecessary operations (unnecessary
memory allocation/deallocation and list copying).

Reverts 7821eb6f37d785ab6ac3bbdc39282c799ad22393 as such
trick is no longer needed, given that all memory leakages
have now been fixed.

This is the fifth version of this patch. Please do not use
the first version as it introduces a serious bug.

For reference, the original issue reported by the Cland
static analyzer is as follows:

support/fc_sort.c:494:6: warning: Potential leak of memory
pointed to by 'head'
            malloc(sizeof(file_context_bucket_t));

Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
---
 support/fc_sort.c |  108 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 69 insertions(+), 39 deletions(-)

--- a/support/fc_sort.c	2017-09-30 02:44:18.137342226 +0200
+++ b/support/fc_sort.c	2017-09-30 21:16:04.899069510 +0200
@@ -46,6 +46,9 @@ typedef struct file_context_node {
 
 void file_context_node_destroy(file_context_node_t *x)
 {
+	if (!x)
+		return;
+
 	free(x->path);
 	free(x->file_type);
 	free(x->context);
@@ -135,8 +138,6 @@ file_context_node_t *fc_merge(file_conte
 	file_context_node_t *temp;
 	file_context_node_t *jumpto;
 
-
-
 	/* If a is a empty list, and b is not,
 	 *  set a as b and proceed to the end. */
 	if (!a && b)
@@ -164,7 +165,6 @@ file_context_node_t *fc_merge(file_conte
 			       fc_compare(a_current->next,
 					  b_current) != -1) {
 
-
 				temp = a_current->next;
 				a_current->next = b_current;
 				b_current = b_current->next;
@@ -177,7 +177,6 @@ file_context_node_t *fc_merge(file_conte
 			a_current = jumpto;
 		}
 
-
 		/* if there is anything left in b to be inserted,
 		   put it on the end */
 		if (b_current) {
@@ -209,11 +208,12 @@ file_context_node_t *fc_merge(file_conte
  */
 void fc_merge_sort(file_context_bucket_t *master)
 {
-
-
 	file_context_bucket_t *current;
 	file_context_bucket_t *temp;
 
+	if (!master)
+		return;
+
 	/* Loop until master is the only bucket left
 	 * so that this will stop when master contains
 	 * the sorted list. */
@@ -222,28 +222,20 @@ void fc_merge_sort(file_context_bucket_t
 
 		/* This loop merges buckets two-by-two. */
 		while (current) {
-
 			if (current->next) {
-
 				current->data =
 				    fc_merge(current->data,
 					     current->next->data);
 
-
-
 				temp = current->next;
 				current->next = current->next->next;
 
 				free(temp);
-
 			}
 
-
 			current = current->next;
 		}
 	}
-
-
 }
 
 
@@ -307,6 +299,25 @@ void fc_fill_data(file_context_node_t *f
 	}
 }
 
+
+
+/* fc_free_file_context_node_list
+ * Free the memory allocated to the linked list and its elements.
+ */
+void fc_free_file_context_node_list(struct file_context_node *node)
+{
+	struct file_context_node *next;
+
+	while (node) {
+		next = node->next;
+		file_context_node_destroy(node);
+		free(node);
+		node = next;
+	}
+}
+
+
+
 /* main
  * This program takes in two arguments, the input filename and the
  *  output filename. The input file should be syntactically correct.
@@ -328,7 +339,6 @@ int main(int argc, char *argv[])
 
 	FILE *in_file, *out_file;
 
-
 	/* Check for the correct number of command line arguments. */
 	if (argc < 2 || argc > 3) {
 		fprintf(stderr, "Usage: %s <infile> [<outfile>]\n",argv[0]);
@@ -348,24 +358,33 @@ int main(int argc, char *argv[])
 
 	/* Initialize the head of the linked list. */
 	head = current = (file_context_node_t*)malloc(sizeof(file_context_node_t));
+	if (!head) {
+		fprintf(stderr, "Error: failure allocating memory.\n");
+		return 1;
+	}
 	head->next = NULL;
+	head->path = NULL;
+	head->file_type = NULL;
+	head->context = NULL;
 
 	/* Parse the file into a file_context linked list. */
 	line_buf = NULL;
 
 	while ( getline(&line_buf, &buf_len, in_file) != -1 ){
 		line_len = strlen(line_buf);
+
 		if( line_len == 0 || line_len == 1)
 			continue;
+
 		/* Get rid of whitespace from the front of the line. */
 		for (i = 0; i < line_len; i++) {
 			if (!isspace(line_buf[i]))
 				break;
 		}
 
-
 		if (i >= line_len)
 			continue;
+
 		/* Check if the line isn't empty and isn't a comment */
 		if (line_buf[i] == '#')
 			continue;
@@ -373,7 +392,9 @@ int main(int argc, char *argv[])
 		/* We have a valid line - allocate a new node. */
 		temp = (file_context_node_t *)malloc(sizeof(file_context_node_t));
 		if (!temp) {
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 		temp->next = NULL;
@@ -382,19 +403,15 @@ int main(int argc, char *argv[])
 		/* Parse out the regular expression from the line. */
 		start = i;
 
-
 		while (i < line_len && (!isspace(line_buf[i])))
 			i++;
 		finish = i;
 
-
 		regex_len = finish - start;
 
 		if (regex_len == 0) {
 			file_context_node_destroy(temp);
 			free(temp);
-
-
 			continue;
 		}
 
@@ -402,13 +419,14 @@ int main(int argc, char *argv[])
 		if (!temp->path) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 
 		/* Get rid of whitespace after the regular expression. */
 		for (; i < line_len; i++) {
-
 			if (!isspace(line_buf[i]))
 				break;
 		}
@@ -420,18 +438,21 @@ int main(int argc, char *argv[])
 		}
 
 		/* Parse out the type from the line (if it
-			*  is there). */
+		 * is there). */
 		if (line_buf[i] == '-') {
 			temp->file_type = (char *)malloc(sizeof(char) * 3);
 			if (!(temp->file_type)) {
+				file_context_node_destroy(temp);
+				free(temp);
+				free(line_buf);
 				fprintf(stderr, "Error: failure allocating memory.\n");
+				fc_free_file_context_node_list(head);
 				return 1;
 			}
 
 			if( i + 2 >= line_len ) {
 				file_context_node_destroy(temp);
 				free(temp);
-
 				continue;
 			}
 
@@ -448,7 +469,6 @@ int main(int argc, char *argv[])
 			}
 
 			if (i == line_len) {
-
 				file_context_node_destroy(temp);
 				free(temp);
 				continue;
@@ -467,24 +487,23 @@ int main(int argc, char *argv[])
 		if (!temp->context) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 
 		/* Set all the data about the regular
-			*  expression. */
+		 * expression. */
 		fc_fill_data(temp);
 
 		/* Link this line of code at the end of
-			*  the linked list. */
+		 * the linked list. */
 		current->next = temp;
 		current = current->next;
 		lines++;
-
-
-		free(line_buf);
-		line_buf = NULL;
 	}
+	free(line_buf);
 	fclose(in_file);
 
 	/* Create the bucket linked list from the earlier linked list. */
@@ -492,6 +511,12 @@ int main(int argc, char *argv[])
 	bcurrent = master =
 	    (file_context_bucket_t *)
 	    malloc(sizeof(file_context_bucket_t));
+	if (!bcurrent) {
+		printf
+		    ("Error: failure allocating memory.\n"); 
+		fc_free_file_context_node_list(head);
+		return -1;
+	}
 	bcurrent->next = NULL;
 	bcurrent->data = NULL;
 
@@ -512,25 +537,33 @@ int main(int argc, char *argv[])
 			if (!(bcurrent->next)) {
 				printf
 				    ("Error: failure allocating memory.\n");
-				exit(-1);
+				free(head);
+				fc_free_file_context_node_list(current);
+				fc_merge_sort(master);
+				fc_free_file_context_node_list(master->data);
+				free(master);
+				return -1;
 			}
 
 			/* Make sure the new bucket thinks it's the end of the
-			 *  list. */
+			 * list. */
 			bcurrent->next->next = NULL;
 
 			bcurrent = bcurrent->next;
 		}
-
 	}
 
 	/* Sort the bucket list. */
 	fc_merge_sort(master);
 
+	free(head);
+
 	/* Open the output file. */
 	if (output_name) {
 		if (!(out_file = fopen(output_name, "w"))) {
 			printf("Error: failure opening output file for write.\n");
+			fc_free_file_context_node_list(master->data);
+			free(master);
 			return -1;
 		}
 	} else {
@@ -539,6 +572,7 @@ int main(int argc, char *argv[])
 
 	/* Output the sorted file_context linked list to the output file. */
 	current = master->data;
+
 	while (current) {
 		/* Output the path. */
 		fprintf(out_file, "%s\t\t", current->path);
@@ -551,14 +585,10 @@ int main(int argc, char *argv[])
 		/* Output the context. */
 		fprintf(out_file, "%s\n", current->context);
 
-		/* Remove the node. */
-		temp = current;
 		current = current->next;
-
-		file_context_node_destroy(temp);
-		free(temp);
-
 	}
+
+	fc_free_file_context_node_list(master->data);
 	free(master);
 
 	if (output_name) {

[refpolicy] [PATCH v5] fc_sort: memory leakages

[Chris PeBenito]

On 09/30/2017 06:44 PM, Guido Trentalancia via refpolicy wrote:
> Avoid memory leakages in the fc_sort executable (now passes
> all valgrind AND Clang static analyzer tests fine).
> 
> Some NULL pointer checks with or without associated error
> reporting.
> 
> Some white space and comment formatting fixes.
> 
> Optimization: avoid unnecessary operations (unnecessary
> memory allocation/deallocation and list copying).
> 
> Reverts 7821eb6f37d785ab6ac3bbdc39282c799ad22393 as such
> trick is no longer needed, given that all memory leakages
> have now been fixed.
> 
> This is the fifth version of this patch. Please do not use
> the first version as it introduces a serious bug.
> 
> For reference, the original issue reported by the Cland
> static analyzer is as follows:
> 
> support/fc_sort.c:494:6: warning: Potential leak of memory
> pointed to by 'head'
>              malloc(sizeof(file_context_bucket_t));

Bill, did your guys run this through their static analyzer? I'm inclined 
to merge this.


> Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
> ---
>   support/fc_sort.c |  108 ++++++++++++++++++++++++++++++++++--------------------
>   1 file changed, 69 insertions(+), 39 deletions(-)
> 
> --- a/support/fc_sort.c	2017-09-30 02:44:18.137342226 +0200
> +++ b/support/fc_sort.c	2017-09-30 21:16:04.899069510 +0200
> @@ -46,6 +46,9 @@ typedef struct file_context_node {
>   
>   void file_context_node_destroy(file_context_node_t *x)
>   {
> +	if (!x)
> +		return;
> +
>   	free(x->path);
>   	free(x->file_type);
>   	free(x->context);
> @@ -135,8 +138,6 @@ file_context_node_t *fc_merge(file_conte
>   	file_context_node_t *temp;
>   	file_context_node_t *jumpto;
>   
> -
> -
>   	/* If a is a empty list, and b is not,
>   	 *  set a as b and proceed to the end. */
>   	if (!a && b)
> @@ -164,7 +165,6 @@ file_context_node_t *fc_merge(file_conte
>   			       fc_compare(a_current->next,
>   					  b_current) != -1) {
>   
> -
>   				temp = a_current->next;
>   				a_current->next = b_current;
>   				b_current = b_current->next;
> @@ -177,7 +177,6 @@ file_context_node_t *fc_merge(file_conte
>   			a_current = jumpto;
>   		}
>   
> -
>   		/* if there is anything left in b to be inserted,
>   		   put it on the end */
>   		if (b_current) {
> @@ -209,11 +208,12 @@ file_context_node_t *fc_merge(file_conte
>    */
>   void fc_merge_sort(file_context_bucket_t *master)
>   {
> -
> -
>   	file_context_bucket_t *current;
>   	file_context_bucket_t *temp;
>   
> +	if (!master)
> +		return;
> +
>   	/* Loop until master is the only bucket left
>   	 * so that this will stop when master contains
>   	 * the sorted list. */
> @@ -222,28 +222,20 @@ void fc_merge_sort(file_context_bucket_t
>   
>   		/* This loop merges buckets two-by-two. */
>   		while (current) {
> -
>   			if (current->next) {
> -
>   				current->data =
>   				    fc_merge(current->data,
>   					     current->next->data);
>   
> -
> -
>   				temp = current->next;
>   				current->next = current->next->next;
>   
>   				free(temp);
> -
>   			}
>   
> -
>   			current = current->next;
>   		}
>   	}
> -
> -
>   }
>   
>   
> @@ -307,6 +299,25 @@ void fc_fill_data(file_context_node_t *f
>   	}
>   }
>   
> +
> +
> +/* fc_free_file_context_node_list
> + * Free the memory allocated to the linked list and its elements.
> + */
> +void fc_free_file_context_node_list(struct file_context_node *node)
> +{
> +	struct file_context_node *next;
> +
> +	while (node) {
> +		next = node->next;
> +		file_context_node_destroy(node);
> +		free(node);
> +		node = next;
> +	}
> +}
> +
> +
> +
>   /* main
>    * This program takes in two arguments, the input filename and the
>    *  output filename. The input file should be syntactically correct.
> @@ -328,7 +339,6 @@ int main(int argc, char *argv[])
>   
>   	FILE *in_file, *out_file;
>   
> -
>   	/* Check for the correct number of command line arguments. */
>   	if (argc < 2 || argc > 3) {
>   		fprintf(stderr, "Usage: %s <infile> [<outfile>]\n",argv[0]);
> @@ -348,24 +358,33 @@ int main(int argc, char *argv[])
>   
>   	/* Initialize the head of the linked list. */
>   	head = current = (file_context_node_t*)malloc(sizeof(file_context_node_t));
> +	if (!head) {
> +		fprintf(stderr, "Error: failure allocating memory.\n");
> +		return 1;
> +	}
>   	head->next = NULL;
> +	head->path = NULL;
> +	head->file_type = NULL;
> +	head->context = NULL;
>   
>   	/* Parse the file into a file_context linked list. */
>   	line_buf = NULL;
>   
>   	while ( getline(&line_buf, &buf_len, in_file) != -1 ){
>   		line_len = strlen(line_buf);
> +
>   		if( line_len == 0 || line_len == 1)
>   			continue;
> +
>   		/* Get rid of whitespace from the front of the line. */
>   		for (i = 0; i < line_len; i++) {
>   			if (!isspace(line_buf[i]))
>   				break;
>   		}
>   
> -
>   		if (i >= line_len)
>   			continue;
> +
>   		/* Check if the line isn't empty and isn't a comment */
>   		if (line_buf[i] == '#')
>   			continue;
> @@ -373,7 +392,9 @@ int main(int argc, char *argv[])
>   		/* We have a valid line - allocate a new node. */
>   		temp = (file_context_node_t *)malloc(sizeof(file_context_node_t));
>   		if (!temp) {
> +			free(line_buf);
>   			fprintf(stderr, "Error: failure allocating memory.\n");
> +			fc_free_file_context_node_list(head);
>   			return 1;
>   		}
>   		temp->next = NULL;
> @@ -382,19 +403,15 @@ int main(int argc, char *argv[])
>   		/* Parse out the regular expression from the line. */
>   		start = i;
>   
> -
>   		while (i < line_len && (!isspace(line_buf[i])))
>   			i++;
>   		finish = i;
>   
> -
>   		regex_len = finish - start;
>   
>   		if (regex_len == 0) {
>   			file_context_node_destroy(temp);
>   			free(temp);
> -
> -
>   			continue;
>   		}
>   
> @@ -402,13 +419,14 @@ int main(int argc, char *argv[])
>   		if (!temp->path) {
>   			file_context_node_destroy(temp);
>   			free(temp);
> +			free(line_buf);
>   			fprintf(stderr, "Error: failure allocating memory.\n");
> +			fc_free_file_context_node_list(head);
>   			return 1;
>   		}
>   
>   		/* Get rid of whitespace after the regular expression. */
>   		for (; i < line_len; i++) {
> -
>   			if (!isspace(line_buf[i]))
>   				break;
>   		}
> @@ -420,18 +438,21 @@ int main(int argc, char *argv[])
>   		}
>   
>   		/* Parse out the type from the line (if it
> -			*  is there). */
> +		 * is there). */
>   		if (line_buf[i] == '-') {
>   			temp->file_type = (char *)malloc(sizeof(char) * 3);
>   			if (!(temp->file_type)) {
> +				file_context_node_destroy(temp);
> +				free(temp);
> +				free(line_buf);
>   				fprintf(stderr, "Error: failure allocating memory.\n");
> +				fc_free_file_context_node_list(head);
>   				return 1;
>   			}
>   
>   			if( i + 2 >= line_len ) {
>   				file_context_node_destroy(temp);
>   				free(temp);
> -
>   				continue;
>   			}
>   
> @@ -448,7 +469,6 @@ int main(int argc, char *argv[])
>   			}
>   
>   			if (i == line_len) {
> -
>   				file_context_node_destroy(temp);
>   				free(temp);
>   				continue;
> @@ -467,24 +487,23 @@ int main(int argc, char *argv[])
>   		if (!temp->context) {
>   			file_context_node_destroy(temp);
>   			free(temp);
> +			free(line_buf);
>   			fprintf(stderr, "Error: failure allocating memory.\n");
> +			fc_free_file_context_node_list(head);
>   			return 1;
>   		}
>   
>   		/* Set all the data about the regular
> -			*  expression. */
> +		 * expression. */
>   		fc_fill_data(temp);
>   
>   		/* Link this line of code at the end of
> -			*  the linked list. */
> +		 * the linked list. */
>   		current->next = temp;
>   		current = current->next;
>   		lines++;
> -
> -
> -		free(line_buf);
> -		line_buf = NULL;
>   	}
> +	free(line_buf);
>   	fclose(in_file);
>   
>   	/* Create the bucket linked list from the earlier linked list. */
> @@ -492,6 +511,12 @@ int main(int argc, char *argv[])
>   	bcurrent = master =
>   	    (file_context_bucket_t *)
>   	    malloc(sizeof(file_context_bucket_t));
> +	if (!bcurrent) {
> +		printf
> +		    ("Error: failure allocating memory.\n");
> +		fc_free_file_context_node_list(head);
> +		return -1;
> +	}
>   	bcurrent->next = NULL;
>   	bcurrent->data = NULL;
>   
> @@ -512,25 +537,33 @@ int main(int argc, char *argv[])
>   			if (!(bcurrent->next)) {
>   				printf
>   				    ("Error: failure allocating memory.\n");
> -				exit(-1);
> +				free(head);
> +				fc_free_file_context_node_list(current);
> +				fc_merge_sort(master);
> +				fc_free_file_context_node_list(master->data);
> +				free(master);
> +				return -1;
>   			}
>   
>   			/* Make sure the new bucket thinks it's the end of the
> -			 *  list. */
> +			 * list. */
>   			bcurrent->next->next = NULL;
>   
>   			bcurrent = bcurrent->next;
>   		}
> -
>   	}
>   
>   	/* Sort the bucket list. */
>   	fc_merge_sort(master);
>   
> +	free(head);
> +
>   	/* Open the output file. */
>   	if (output_name) {
>   		if (!(out_file = fopen(output_name, "w"))) {
>   			printf("Error: failure opening output file for write.\n");
> +			fc_free_file_context_node_list(master->data);
> +			free(master);
>   			return -1;
>   		}
>   	} else {
> @@ -539,6 +572,7 @@ int main(int argc, char *argv[])
>   
>   	/* Output the sorted file_context linked list to the output file. */
>   	current = master->data;
> +
>   	while (current) {
>   		/* Output the path. */
>   		fprintf(out_file, "%s\t\t", current->path);
> @@ -551,14 +585,10 @@ int main(int argc, char *argv[])
>   		/* Output the context. */
>   		fprintf(out_file, "%s\n", current->context);
>   
> -		/* Remove the node. */
> -		temp = current;
>   		current = current->next;
> -
> -		file_context_node_destroy(temp);
> -		free(temp);
> -
>   	}
> +
> +	fc_free_file_context_node_list(master->data);
>   	free(master);
>   
>   	if (output_name) {
> _______________________________________________
> refpolicy mailing list
> refpolicy at oss.tresys.com
> http://oss.tresys.com/mailman/listinfo/refpolicy
> 


-- 
Chris PeBenito

[refpolicy] [PATCH v5] fc_sort: memory leakages

[Guido Trentalancia]

Hello Christopher. 

The latest version (v5) has been tested not only with valgrind but also with the Clang static analyzer and none of them reports any error. 

The Clang analyzer is the same one that was mentioned in the original bug report, I have used the latest version 5.0.

I hope it helps. 

Regards, 

Guido 

Il 03 ottobre 2017 03:21:38 CEST, Chris PeBenito <pebenito@ieee.org> ha scritto:
>On 09/30/2017 06:44 PM, Guido Trentalancia via refpolicy wrote:
>> Avoid memory leakages in the fc_sort executable (now passes
>> all valgrind AND Clang static analyzer tests fine).
>> 
>> Some NULL pointer checks with or without associated error
>> reporting.
>> 
>> Some white space and comment formatting fixes.
>> 
>> Optimization: avoid unnecessary operations (unnecessary
>> memory allocation/deallocation and list copying).
>> 
>> Reverts 7821eb6f37d785ab6ac3bbdc39282c799ad22393 as such
>> trick is no longer needed, given that all memory leakages
>> have now been fixed.
>> 
>> This is the fifth version of this patch. Please do not use
>> the first version as it introduces a serious bug.
>> 
>> For reference, the original issue reported by the Cland
>> static analyzer is as follows:
>> 
>> support/fc_sort.c:494:6: warning: Potential leak of memory
>> pointed to by 'head'
>>              malloc(sizeof(file_context_bucket_t));
>
>Bill, did your guys run this through their static analyzer? I'm
>inclined 
>to merge this.
>
>
>> Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
>> ---
>>   support/fc_sort.c |  108
>++++++++++++++++++++++++++++++++++--------------------
>>   1 file changed, 69 insertions(+), 39 deletions(-)
>> 
>> --- a/support/fc_sort.c	2017-09-30 02:44:18.137342226 +0200
>> +++ b/support/fc_sort.c	2017-09-30 21:16:04.899069510 +0200
>> @@ -46,6 +46,9 @@ typedef struct file_context_node {
>>   
>>   void file_context_node_destroy(file_context_node_t *x)
>>   {
>> +	if (!x)
>> +		return;
>> +
>>   	free(x->path);
>>   	free(x->file_type);
>>   	free(x->context);
>> @@ -135,8 +138,6 @@ file_context_node_t *fc_merge(file_conte
>>   	file_context_node_t *temp;
>>   	file_context_node_t *jumpto;
>>   
>> -
>> -
>>   	/* If a is a empty list, and b is not,
>>   	 *  set a as b and proceed to the end. */
>>   	if (!a && b)
>> @@ -164,7 +165,6 @@ file_context_node_t *fc_merge(file_conte
>>   			       fc_compare(a_current->next,
>>   					  b_current) != -1) {
>>   
>> -
>>   				temp = a_current->next;
>>   				a_current->next = b_current;
>>   				b_current = b_current->next;
>> @@ -177,7 +177,6 @@ file_context_node_t *fc_merge(file_conte
>>   			a_current = jumpto;
>>   		}
>>   
>> -
>>   		/* if there is anything left in b to be inserted,
>>   		   put it on the end */
>>   		if (b_current) {
>> @@ -209,11 +208,12 @@ file_context_node_t *fc_merge(file_conte
>>    */
>>   void fc_merge_sort(file_context_bucket_t *master)
>>   {
>> -
>> -
>>   	file_context_bucket_t *current;
>>   	file_context_bucket_t *temp;
>>   
>> +	if (!master)
>> +		return;
>> +
>>   	/* Loop until master is the only bucket left
>>   	 * so that this will stop when master contains
>>   	 * the sorted list. */
>> @@ -222,28 +222,20 @@ void fc_merge_sort(file_context_bucket_t
>>   
>>   		/* This loop merges buckets two-by-two. */
>>   		while (current) {
>> -
>>   			if (current->next) {
>> -
>>   				current->data =
>>   				    fc_merge(current->data,
>>   					     current->next->data);
>>   
>> -
>> -
>>   				temp = current->next;
>>   				current->next = current->next->next;
>>   
>>   				free(temp);
>> -
>>   			}
>>   
>> -
>>   			current = current->next;
>>   		}
>>   	}
>> -
>> -
>>   }
>>   
>>   
>> @@ -307,6 +299,25 @@ void fc_fill_data(file_context_node_t *f
>>   	}
>>   }
>>   
>> +
>> +
>> +/* fc_free_file_context_node_list
>> + * Free the memory allocated to the linked list and its elements.
>> + */
>> +void fc_free_file_context_node_list(struct file_context_node *node)
>> +{
>> +	struct file_context_node *next;
>> +
>> +	while (node) {
>> +		next = node->next;
>> +		file_context_node_destroy(node);
>> +		free(node);
>> +		node = next;
>> +	}
>> +}
>> +
>> +
>> +
>>   /* main
>>    * This program takes in two arguments, the input filename and the
>>    *  output filename. The input file should be syntactically
>correct.
>> @@ -328,7 +339,6 @@ int main(int argc, char *argv[])
>>   
>>   	FILE *in_file, *out_file;
>>   
>> -
>>   	/* Check for the correct number of command line arguments. */
>>   	if (argc < 2 || argc > 3) {
>>   		fprintf(stderr, "Usage: %s <infile> [<outfile>]\n",argv[0]);
>> @@ -348,24 +358,33 @@ int main(int argc, char *argv[])
>>   
>>   	/* Initialize the head of the linked list. */
>>   	head = current =
>(file_context_node_t*)malloc(sizeof(file_context_node_t));
>> +	if (!head) {
>> +		fprintf(stderr, "Error: failure allocating memory.\n");
>> +		return 1;
>> +	}
>>   	head->next = NULL;
>> +	head->path = NULL;
>> +	head->file_type = NULL;
>> +	head->context = NULL;
>>   
>>   	/* Parse the file into a file_context linked list. */
>>   	line_buf = NULL;
>>   
>>   	while ( getline(&line_buf, &buf_len, in_file) != -1 ){
>>   		line_len = strlen(line_buf);
>> +
>>   		if( line_len == 0 || line_len == 1)
>>   			continue;
>> +
>>   		/* Get rid of whitespace from the front of the line. */
>>   		for (i = 0; i < line_len; i++) {
>>   			if (!isspace(line_buf[i]))
>>   				break;
>>   		}
>>   
>> -
>>   		if (i >= line_len)
>>   			continue;
>> +
>>   		/* Check if the line isn't empty and isn't a comment */
>>   		if (line_buf[i] == '#')
>>   			continue;
>> @@ -373,7 +392,9 @@ int main(int argc, char *argv[])
>>   		/* We have a valid line - allocate a new node. */
>>   		temp = (file_context_node_t
>*)malloc(sizeof(file_context_node_t));
>>   		if (!temp) {
>> +			free(line_buf);
>>   			fprintf(stderr, "Error: failure allocating memory.\n");
>> +			fc_free_file_context_node_list(head);
>>   			return 1;
>>   		}
>>   		temp->next = NULL;
>> @@ -382,19 +403,15 @@ int main(int argc, char *argv[])
>>   		/* Parse out the regular expression from the line. */
>>   		start = i;
>>   
>> -
>>   		while (i < line_len && (!isspace(line_buf[i])))
>>   			i++;
>>   		finish = i;
>>   
>> -
>>   		regex_len = finish - start;
>>   
>>   		if (regex_len == 0) {
>>   			file_context_node_destroy(temp);
>>   			free(temp);
>> -
>> -
>>   			continue;
>>   		}
>>   
>> @@ -402,13 +419,14 @@ int main(int argc, char *argv[])
>>   		if (!temp->path) {
>>   			file_context_node_destroy(temp);
>>   			free(temp);
>> +			free(line_buf);
>>   			fprintf(stderr, "Error: failure allocating memory.\n");
>> +			fc_free_file_context_node_list(head);
>>   			return 1;
>>   		}
>>   
>>   		/* Get rid of whitespace after the regular expression. */
>>   		for (; i < line_len; i++) {
>> -
>>   			if (!isspace(line_buf[i]))
>>   				break;
>>   		}
>> @@ -420,18 +438,21 @@ int main(int argc, char *argv[])
>>   		}
>>   
>>   		/* Parse out the type from the line (if it
>> -			*  is there). */
>> +		 * is there). */
>>   		if (line_buf[i] == '-') {
>>   			temp->file_type = (char *)malloc(sizeof(char) * 3);
>>   			if (!(temp->file_type)) {
>> +				file_context_node_destroy(temp);
>> +				free(temp);
>> +				free(line_buf);
>>   				fprintf(stderr, "Error: failure allocating memory.\n");
>> +				fc_free_file_context_node_list(head);
>>   				return 1;
>>   			}
>>   
>>   			if( i + 2 >= line_len ) {
>>   				file_context_node_destroy(temp);
>>   				free(temp);
>> -
>>   				continue;
>>   			}
>>   
>> @@ -448,7 +469,6 @@ int main(int argc, char *argv[])
>>   			}
>>   
>>   			if (i == line_len) {
>> -
>>   				file_context_node_destroy(temp);
>>   				free(temp);
>>   				continue;
>> @@ -467,24 +487,23 @@ int main(int argc, char *argv[])
>>   		if (!temp->context) {
>>   			file_context_node_destroy(temp);
>>   			free(temp);
>> +			free(line_buf);
>>   			fprintf(stderr, "Error: failure allocating memory.\n");
>> +			fc_free_file_context_node_list(head);
>>   			return 1;
>>   		}
>>   
>>   		/* Set all the data about the regular
>> -			*  expression. */
>> +		 * expression. */
>>   		fc_fill_data(temp);
>>   
>>   		/* Link this line of code at the end of
>> -			*  the linked list. */
>> +		 * the linked list. */
>>   		current->next = temp;
>>   		current = current->next;
>>   		lines++;
>> -
>> -
>> -		free(line_buf);
>> -		line_buf = NULL;
>>   	}
>> +	free(line_buf);
>>   	fclose(in_file);
>>   
>>   	/* Create the bucket linked list from the earlier linked list. */
>> @@ -492,6 +511,12 @@ int main(int argc, char *argv[])
>>   	bcurrent = master =
>>   	    (file_context_bucket_t *)
>>   	    malloc(sizeof(file_context_bucket_t));
>> +	if (!bcurrent) {
>> +		printf
>> +		    ("Error: failure allocating memory.\n");
>> +		fc_free_file_context_node_list(head);
>> +		return -1;
>> +	}
>>   	bcurrent->next = NULL;
>>   	bcurrent->data = NULL;
>>   
>> @@ -512,25 +537,33 @@ int main(int argc, char *argv[])
>>   			if (!(bcurrent->next)) {
>>   				printf
>>   				    ("Error: failure allocating memory.\n");
>> -				exit(-1);
>> +				free(head);
>> +				fc_free_file_context_node_list(current);
>> +				fc_merge_sort(master);
>> +				fc_free_file_context_node_list(master->data);
>> +				free(master);
>> +				return -1;
>>   			}
>>   
>>   			/* Make sure the new bucket thinks it's the end of the
>> -			 *  list. */
>> +			 * list. */
>>   			bcurrent->next->next = NULL;
>>   
>>   			bcurrent = bcurrent->next;
>>   		}
>> -
>>   	}
>>   
>>   	/* Sort the bucket list. */
>>   	fc_merge_sort(master);
>>   
>> +	free(head);
>> +
>>   	/* Open the output file. */
>>   	if (output_name) {
>>   		if (!(out_file = fopen(output_name, "w"))) {
>>   			printf("Error: failure opening output file for write.\n");
>> +			fc_free_file_context_node_list(master->data);
>> +			free(master);
>>   			return -1;
>>   		}
>>   	} else {
>> @@ -539,6 +572,7 @@ int main(int argc, char *argv[])
>>   
>>   	/* Output the sorted file_context linked list to the output file.
>*/
>>   	current = master->data;
>> +
>>   	while (current) {
>>   		/* Output the path. */
>>   		fprintf(out_file, "%s\t\t", current->path);
>> @@ -551,14 +585,10 @@ int main(int argc, char *argv[])
>>   		/* Output the context. */
>>   		fprintf(out_file, "%s\n", current->context);
>>   
>> -		/* Remove the node. */
>> -		temp = current;
>>   		current = current->next;
>> -
>> -		file_context_node_destroy(temp);
>> -		free(temp);
>> -
>>   	}
>> +
>> +	fc_free_file_context_node_list(master->data);
>>   	free(master);
>>   
>>   	if (output_name) {
>> _______________________________________________
>> refpolicy mailing list
>> refpolicy at oss.tresys.com
>> http://oss.tresys.com/mailman/listinfo/refpolicy
>> 

[refpolicy] [PATCH v5] fc_sort: memory leakages

[William Roberts]

On Wed, Oct 4, 2017 at 2:41 AM, Guido Trentalancia via refpolicy
<refpolicy@oss.tresys.com> wrote:
> Hello Christopher.
>
> The latest version (v5) has been tested not only with valgrind but also with the Clang static analyzer and none of them reports any error.
>
> The Clang analyzer is the same one that was mentioned in the original bug report, I have used the latest version 5.0.
>
> I hope it helps.
>
> Regards,
>
> Guido
>
> Il 03 ottobre 2017 03:21:38 CEST, Chris PeBenito <pebenito@ieee.org> ha scritto:
>>On 09/30/2017 06:44 PM, Guido Trentalancia via refpolicy wrote:
>>> Avoid memory leakages in the fc_sort executable (now passes
>>> all valgrind AND Clang static analyzer tests fine).
>>>
>>> Some NULL pointer checks with or without associated error
>>> reporting.
>>>
>>> Some white space and comment formatting fixes.
>>>
>>> Optimization: avoid unnecessary operations (unnecessary
>>> memory allocation/deallocation and list copying).
>>>
>>> Reverts 7821eb6f37d785ab6ac3bbdc39282c799ad22393 as such
>>> trick is no longer needed, given that all memory leakages
>>> have now been fixed.
>>>
>>> This is the fifth version of this patch. Please do not use
>>> the first version as it introduces a serious bug.
>>>
>>> For reference, the original issue reported by the Cland
>>> static analyzer is as follows:
>>>
>>> support/fc_sort.c:494:6: warning: Potential leak of memory
>>> pointed to by 'head'
>>>              malloc(sizeof(file_context_bucket_t));
>>
>>Bill, did your guys run this through their static analyzer? I'm
>>inclined
>>to merge this.
>>
>>
>>> Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
>>> ---
>>>   support/fc_sort.c |  108
>>++++++++++++++++++++++++++++++++++--------------------
>>>   1 file changed, 69 insertions(+), 39 deletions(-)
>>>
>>> --- a/support/fc_sort.c      2017-09-30 02:44:18.137342226 +0200
>>> +++ b/support/fc_sort.c      2017-09-30 21:16:04.899069510 +0200
>>> @@ -46,6 +46,9 @@ typedef struct file_context_node {
>>>
>>>   void file_context_node_destroy(file_context_node_t *x)
>>>   {
>>> +    if (!x)
>>> +            return;
>>> +
>>>      free(x->path);
>>>      free(x->file_type);
>>>      free(x->context);
>>> @@ -135,8 +138,6 @@ file_context_node_t *fc_merge(file_conte
>>>      file_context_node_t *temp;
>>>      file_context_node_t *jumpto;
>>>
>>> -
>>> -
>>>      /* If a is a empty list, and b is not,
>>>       *  set a as b and proceed to the end. */
>>>      if (!a && b)
>>> @@ -164,7 +165,6 @@ file_context_node_t *fc_merge(file_conte
>>>                             fc_compare(a_current->next,
>>>                                        b_current) != -1) {
>>>
>>> -
>>>                              temp = a_current->next;
>>>                              a_current->next = b_current;
>>>                              b_current = b_current->next;
>>> @@ -177,7 +177,6 @@ file_context_node_t *fc_merge(file_conte
>>>                      a_current = jumpto;
>>>              }
>>>
>>> -
>>>              /* if there is anything left in b to be inserted,
>>>                 put it on the end */
>>>              if (b_current) {
>>> @@ -209,11 +208,12 @@ file_context_node_t *fc_merge(file_conte
>>>    */
>>>   void fc_merge_sort(file_context_bucket_t *master)
>>>   {
>>> -
>>> -
>>>      file_context_bucket_t *current;
>>>      file_context_bucket_t *temp;
>>>
>>> +    if (!master)
>>> +            return;
>>> +
>>>      /* Loop until master is the only bucket left
>>>       * so that this will stop when master contains
>>>       * the sorted list. */
>>> @@ -222,28 +222,20 @@ void fc_merge_sort(file_context_bucket_t
>>>
>>>              /* This loop merges buckets two-by-two. */
>>>              while (current) {
>>> -
>>>                      if (current->next) {
>>> -
>>>                              current->data =
>>>                                  fc_merge(current->data,
>>>                                           current->next->data);
>>>
>>> -
>>> -
>>>                              temp = current->next;
>>>                              current->next = current->next->next;
>>>
>>>                              free(temp);
>>> -
>>>                      }
>>>
>>> -
>>>                      current = current->next;
>>>              }
>>>      }
>>> -
>>> -
>>>   }
>>>
>>>
>>> @@ -307,6 +299,25 @@ void fc_fill_data(file_context_node_t *f
>>>      }
>>>   }
>>>
>>> +
>>> +
>>> +/* fc_free_file_context_node_list
>>> + * Free the memory allocated to the linked list and its elements.
>>> + */
>>> +void fc_free_file_context_node_list(struct file_context_node *node)
>>> +{
>>> +    struct file_context_node *next;
>>> +
>>> +    while (node) {
>>> +            next = node->next;
>>> +            file_context_node_destroy(node);
>>> +            free(node);
>>> +            node = next;
>>> +    }
>>> +}
>>> +
>>> +
>>> +
>>>   /* main
>>>    * This program takes in two arguments, the input filename and the
>>>    *  output filename. The input file should be syntactically
>>correct.
>>> @@ -328,7 +339,6 @@ int main(int argc, char *argv[])
>>>
>>>      FILE *in_file, *out_file;
>>>
>>> -
>>>      /* Check for the correct number of command line arguments. */
>>>      if (argc < 2 || argc > 3) {
>>>              fprintf(stderr, "Usage: %s <infile> [<outfile>]\n",argv[0]);
>>> @@ -348,24 +358,33 @@ int main(int argc, char *argv[])
>>>
>>>      /* Initialize the head of the linked list. */
>>>      head = current =
>>(file_context_node_t*)malloc(sizeof(file_context_node_t));
>>> +    if (!head) {
>>> +            fprintf(stderr, "Error: failure allocating memory.\n");
>>> +            return 1;
>>> +    }
>>>      head->next = NULL;
>>> +    head->path = NULL;
>>> +    head->file_type = NULL;
>>> +    head->context = NULL;
>>>
>>>      /* Parse the file into a file_context linked list. */
>>>      line_buf = NULL;
>>>
>>>      while ( getline(&line_buf, &buf_len, in_file) != -1 ){
>>>              line_len = strlen(line_buf);
>>> +
>>>              if( line_len == 0 || line_len == 1)
>>>                      continue;
>>> +
>>>              /* Get rid of whitespace from the front of the line. */
>>>              for (i = 0; i < line_len; i++) {
>>>                      if (!isspace(line_buf[i]))
>>>                              break;
>>>              }
>>>
>>> -
>>>              if (i >= line_len)
>>>                      continue;
>>> +
>>>              /* Check if the line isn't empty and isn't a comment */
>>>              if (line_buf[i] == '#')
>>>                      continue;
>>> @@ -373,7 +392,9 @@ int main(int argc, char *argv[])
>>>              /* We have a valid line - allocate a new node. */
>>>              temp = (file_context_node_t
>>*)malloc(sizeof(file_context_node_t));
>>>              if (!temp) {
>>> +                    free(line_buf);
>>>                      fprintf(stderr, "Error: failure allocating memory.\n");
>>> +                    fc_free_file_context_node_list(head);
>>>                      return 1;
>>>              }
>>>              temp->next = NULL;
>>> @@ -382,19 +403,15 @@ int main(int argc, char *argv[])
>>>              /* Parse out the regular expression from the line. */
>>>              start = i;
>>>
>>> -
>>>              while (i < line_len && (!isspace(line_buf[i])))
>>>                      i++;
>>>              finish = i;
>>>
>>> -
>>>              regex_len = finish - start;
>>>
>>>              if (regex_len == 0) {
>>>                      file_context_node_destroy(temp);
>>>                      free(temp);
>>> -
>>> -
>>>                      continue;
>>>              }
>>>
>>> @@ -402,13 +419,14 @@ int main(int argc, char *argv[])
>>>              if (!temp->path) {
>>>                      file_context_node_destroy(temp);
>>>                      free(temp);
>>> +                    free(line_buf);
>>>                      fprintf(stderr, "Error: failure allocating memory.\n");
>>> +                    fc_free_file_context_node_list(head);
>>>                      return 1;
>>>              }
>>>
>>>              /* Get rid of whitespace after the regular expression. */
>>>              for (; i < line_len; i++) {
>>> -
>>>                      if (!isspace(line_buf[i]))
>>>                              break;
>>>              }
>>> @@ -420,18 +438,21 @@ int main(int argc, char *argv[])
>>>              }
>>>
>>>              /* Parse out the type from the line (if it
>>> -                    *  is there). */
>>> +             * is there). */
>>>              if (line_buf[i] == '-') {
>>>                      temp->file_type = (char *)malloc(sizeof(char) * 3);
>>>                      if (!(temp->file_type)) {
>>> +                            file_context_node_destroy(temp);
>>> +                            free(temp);
>>> +                            free(line_buf);
>>>                              fprintf(stderr, "Error: failure allocating memory.\n");
>>> +                            fc_free_file_context_node_list(head);
>>>                              return 1;
>>>                      }
>>>
>>>                      if( i + 2 >= line_len ) {
>>>                              file_context_node_destroy(temp);
>>>                              free(temp);
>>> -
>>>                              continue;
>>>                      }
>>>
>>> @@ -448,7 +469,6 @@ int main(int argc, char *argv[])
>>>                      }
>>>
>>>                      if (i == line_len) {
>>> -
>>>                              file_context_node_destroy(temp);
>>>                              free(temp);
>>>                              continue;
>>> @@ -467,24 +487,23 @@ int main(int argc, char *argv[])
>>>              if (!temp->context) {
>>>                      file_context_node_destroy(temp);
>>>                      free(temp);
>>> +                    free(line_buf);
>>>                      fprintf(stderr, "Error: failure allocating memory.\n");
>>> +                    fc_free_file_context_node_list(head);
>>>                      return 1;
>>>              }
>>>
>>>              /* Set all the data about the regular
>>> -                    *  expression. */
>>> +             * expression. */
>>>              fc_fill_data(temp);
>>>
>>>              /* Link this line of code at the end of
>>> -                    *  the linked list. */
>>> +             * the linked list. */
>>>              current->next = temp;
>>>              current = current->next;
>>>              lines++;
>>> -
>>> -
>>> -            free(line_buf);
>>> -            line_buf = NULL;
>>>      }
>>> +    free(line_buf);
>>>      fclose(in_file);
>>>
>>>      /* Create the bucket linked list from the earlier linked list. */
>>> @@ -492,6 +511,12 @@ int main(int argc, char *argv[])
>>>      bcurrent = master =
>>>          (file_context_bucket_t *)
>>>          malloc(sizeof(file_context_bucket_t));
>>> +    if (!bcurrent) {
>>> +            printf
>>> +                ("Error: failure allocating memory.\n");
>>> +            fc_free_file_context_node_list(head);
>>> +            return -1;
>>> +    }
>>>      bcurrent->next = NULL;
>>>      bcurrent->data = NULL;
>>>
>>> @@ -512,25 +537,33 @@ int main(int argc, char *argv[])
>>>                      if (!(bcurrent->next)) {
>>>                              printf
>>>                                  ("Error: failure allocating memory.\n");
>>> -                            exit(-1);
>>> +                            free(head);
>>> +                            fc_free_file_context_node_list(current);
>>> +                            fc_merge_sort(master);
>>> +                            fc_free_file_context_node_list(master->data);
>>> +                            free(master);
>>> +                            return -1;
>>>                      }
>>>
>>>                      /* Make sure the new bucket thinks it's the end of the
>>> -                     *  list. */
>>> +                     * list. */
>>>                      bcurrent->next->next = NULL;
>>>
>>>                      bcurrent = bcurrent->next;
>>>              }
>>> -
>>>      }
>>>
>>>      /* Sort the bucket list. */
>>>      fc_merge_sort(master);
>>>
>>> +    free(head);
>>> +
>>>      /* Open the output file. */
>>>      if (output_name) {
>>>              if (!(out_file = fopen(output_name, "w"))) {
>>>                      printf("Error: failure opening output file for write.\n");
>>> +                    fc_free_file_context_node_list(master->data);
>>> +                    free(master);
>>>                      return -1;
>>>              }
>>>      } else {
>>> @@ -539,6 +572,7 @@ int main(int argc, char *argv[])
>>>
>>>      /* Output the sorted file_context linked list to the output file.
>>*/
>>>      current = master->data;
>>> +
>>>      while (current) {
>>>              /* Output the path. */
>>>              fprintf(out_file, "%s\t\t", current->path);
>>> @@ -551,14 +585,10 @@ int main(int argc, char *argv[])
>>>              /* Output the context. */
>>>              fprintf(out_file, "%s\n", current->context);
>>>
>>> -            /* Remove the node. */
>>> -            temp = current;
>>>              current = current->next;
>>> -
>>> -            file_context_node_destroy(temp);
>>> -            free(temp);
>>> -
>>>      }
>>> +
>>> +    fc_free_file_context_node_list(master->data);
>>>      free(master);
>>>
>>>      if (output_name) {
>>> _______________________________________________
>>> refpolicy mailing list
>>> refpolicy at oss.tresys.com
>>> http://oss.tresys.com/mailman/listinfo/refpolicy
>>>
>
> _______________________________________________
> refpolicy mailing list
> refpolicy at oss.tresys.com
> http://oss.tresys.com/mailman/listinfo/refpolicy


I would go ahead and merge this, and it seems to be the proper fix for
the error reported via clang
and it works on Android as expected.

<acked-by: William Roberts william.c.roberts@intel.com>
<reviewed-by: William Roberts william.c.roberts@intel.com>
<tested-by: William Roberts william.c.roberts@intel.com>

(unfortunately my @intel email address sometimes has issues with
mailing lists, but please use it)
(Take any of the -by lines you want, I really don't care about that stuff)

Just make sure either the applier or author (v6) fixes:
Applying: fc_sort: memory leakages
.git/rebase-apply/patch:269: trailing whitespace.
   ("Error: failure allocating memory.\n");
warning: 1 line adds whitespace errors.

Always check your patches with something like
  * git diff --check
  * git show HEAD --check
prior to sending.

The bug does seem fixed per my clang version of 3.8:

Patch Applied:
$ scan-build gcc -o go support/fc_sort.c
scan-build: Using '/usr/lib/llvm-3.8/bin/clang' for static analysis
scan-build: Removing directory
'/tmp/scan-build-2017-10-04-104851-19628-1' because it contains no
reports.
scan-build: No bugs found.

Without Patch:
support/fc_sort.c:494:6: warning: Potential leak of memory pointed to by 'head'
            malloc(sizeof(file_context_bucket_t));
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
scan-build: 1 bug found.
scan-build: Run 'scan-view /tmp/scan-build-2017-10-04-104902-19662-1'
to examine bug reports.

I also manually ran the fc_sort against the Android build artifacts to
verify that they are the same via below:

Produced via Android build (old fc_sort)
$ md5sum $OUT/obj/ETC/file_contexts.bin_intermediates/file_contexts.device.sorted.tmp
27d0c1b0d0a516fa9019917b31d3f196
/home/wcrobert/workspace/aosp/out/target/product/hikey/obj/ETC/file_contexts.bin_intermediates/file_contexts.device.sorted.tmp

The new file with the patched fc_sort:
~/workspace/refpolicy/fc_sort
$OUT/obj/ETC/file_contexts.bin_intermediates/file_contexts.device.tmp
file_contexts.device.sorted.tmp
$ md5sum file_contexts.device.sorted.tmp
27d0c1b0d0a516fa9019917b31d3f196  file_contexts.device.sorted.tmp

Valgrind also reports no leaks or memory issues

Once this is merged, ill go ahead and update AOSPs usage of fc_sort
unless someone from Google beats me to it.

FYI Ill be on vacation for a week, so I won't be reachable on this
matter. Thanks for the patch!

Bill

[refpolicy] [PATCH v5] fc_sort: memory leakages

[Guido Trentalancia]

On Wed, 04/10/2017 at 11.05 -0700, William Roberts wrote:
> On Wed, Oct 4, 2017 at 2:41 AM, Guido Trentalancia via refpolicy
> <refpolicy@oss.tresys.com> wrote:
> > Hello Christopher.
> > 
> > The latest version (v5) has been tested not only with valgrind but
> > also with the Clang static analyzer and none of them reports any
> > error.
> > 
> > The Clang analyzer is the same one that was mentioned in the
> > original bug report, I have used the latest version 5.0.
> > 
> > I hope it helps.
> > 
> > Regards,
> > 
> > Guido
> > 
> > Il 03 ottobre 2017 03:21:38 CEST, Chris PeBenito <pebenito@ieee.org
> > > ha scritto:
> > > On 09/30/2017 06:44 PM, Guido Trentalancia via refpolicy wrote:
> > > > Avoid memory leakages in the fc_sort executable (now passes
> > > > all valgrind AND Clang static analyzer tests fine).
> > > > 
> > > > Some NULL pointer checks with or without associated error
> > > > reporting.
> > > > 
> > > > Some white space and comment formatting fixes.
> > > > 
> > > > Optimization: avoid unnecessary operations (unnecessary
> > > > memory allocation/deallocation and list copying).
> > > > 
> > > > Reverts 7821eb6f37d785ab6ac3bbdc39282c799ad22393 as such
> > > > trick is no longer needed, given that all memory leakages
> > > > have now been fixed.
> > > > 
> > > > This is the fifth version of this patch. Please do not use
> > > > the first version as it introduces a serious bug.
> > > > 
> > > > For reference, the original issue reported by the Cland
> > > > static analyzer is as follows:
> > > > 
> > > > support/fc_sort.c:494:6: warning: Potential leak of memory
> > > > pointed to by 'head'
> > > >              malloc(sizeof(file_context_bucket_t));
> > > 
> > > Bill, did your guys run this through their static analyzer? I'm
> > > inclined
> > > to merge this.

[...]

> 
> FYI Ill be on vacation for a week, so I won't be reachable on this
> matter. Thanks for the patch!
> 
> Bill

Thanks very much for reviewing it Bill ! I have now fixed the
whitespace as suggested by "git diff --check".

The new patch version will follow shortly...

Regards,

Guido

[refpolicy] [PATCH v6] fc_sort: memory leakages

[Guido Trentalancia]

Avoid memory leakages in the fc_sort executable (now passes
all valgrind AND Clang static analyzer tests fine).

Some NULL pointer checks with or without associated error
reporting.

Some white space and comment formatting fixes.

Optimization: avoid unnecessary operations (unnecessary
memory allocation/deallocation and list copying).

Reverts 7821eb6f37d785ab6ac3bbdc39282c799ad22393 as such
trick is no longer needed, given that all memory leakages
have now been fixed.

This is the sixth version of this patch. Please do not use
the first version as it introduces a serious bug.

For reference, the original issue reported by the Cland
static analyzer is as follows:

support/fc_sort.c:494:6: warning: Potential leak of memory 
pointed to by 'head'
            malloc(sizeof(file_context_bucket_t));

Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
Acked-by: William Roberts <william.c.roberts@intel.com>
---
 support/fc_sort.c |  108 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 69 insertions(+), 39 deletions(-)

--- a/support/fc_sort.c	2017-09-30 02:44:18.137342226 +0200
+++ b/support/fc_sort.c	2017-09-30 21:16:04.899069510 +0200
@@ -46,6 +46,9 @@ typedef struct file_context_node {
 
 void file_context_node_destroy(file_context_node_t *x)
 {
+	if (!x)
+		return;
+
 	free(x->path);
 	free(x->file_type);
 	free(x->context);
@@ -135,8 +138,6 @@ file_context_node_t *fc_merge(file_conte
 	file_context_node_t *temp;
 	file_context_node_t *jumpto;
 
-
-
 	/* If a is a empty list, and b is not,
 	 *  set a as b and proceed to the end. */
 	if (!a && b)
@@ -164,7 +165,6 @@ file_context_node_t *fc_merge(file_conte
 			       fc_compare(a_current->next,
 					  b_current) != -1) {
 
-
 				temp = a_current->next;
 				a_current->next = b_current;
 				b_current = b_current->next;
@@ -177,7 +177,6 @@ file_context_node_t *fc_merge(file_conte
 			a_current = jumpto;
 		}
 
-
 		/* if there is anything left in b to be inserted,
 		   put it on the end */
 		if (b_current) {
@@ -209,11 +208,12 @@ file_context_node_t *fc_merge(file_conte
  */
 void fc_merge_sort(file_context_bucket_t *master)
 {
-
-
 	file_context_bucket_t *current;
 	file_context_bucket_t *temp;
 
+	if (!master)
+		return;
+
 	/* Loop until master is the only bucket left
 	 * so that this will stop when master contains
 	 * the sorted list. */
@@ -222,28 +222,20 @@ void fc_merge_sort(file_context_bucket_t
 
 		/* This loop merges buckets two-by-two. */
 		while (current) {
-
 			if (current->next) {
-
 				current->data =
 				    fc_merge(current->data,
 					     current->next->data);
 
-
-
 				temp = current->next;
 				current->next = current->next->next;
 
 				free(temp);
-
 			}
 
-
 			current = current->next;
 		}
 	}
-
-
 }
 
 
@@ -307,6 +299,25 @@ void fc_fill_data(file_context_node_t *f
 	}
 }
 
+
+
+/* fc_free_file_context_node_list
+ * Free the memory allocated to the linked list and its elements.
+ */
+void fc_free_file_context_node_list(struct file_context_node *node)
+{
+	struct file_context_node *next;
+
+	while (node) {
+		next = node->next;
+		file_context_node_destroy(node);
+		free(node);
+		node = next;
+	}
+}
+
+
+
 /* main
  * This program takes in two arguments, the input filename and the
  *  output filename. The input file should be syntactically correct.
@@ -328,7 +339,6 @@ int main(int argc, char *argv[])
 
 	FILE *in_file, *out_file;
 
-
 	/* Check for the correct number of command line arguments. */
 	if (argc < 2 || argc > 3) {
 		fprintf(stderr, "Usage: %s <infile> [<outfile>]\n",argv[0]);
@@ -348,24 +358,33 @@ int main(int argc, char *argv[])
 
 	/* Initialize the head of the linked list. */
 	head = current = (file_context_node_t*)malloc(sizeof(file_context_node_t));
+	if (!head) {
+		fprintf(stderr, "Error: failure allocating memory.\n");
+		return 1;
+	}
 	head->next = NULL;
+	head->path = NULL;
+	head->file_type = NULL;
+	head->context = NULL;
 
 	/* Parse the file into a file_context linked list. */
 	line_buf = NULL;
 
 	while ( getline(&line_buf, &buf_len, in_file) != -1 ){
 		line_len = strlen(line_buf);
+
 		if( line_len == 0 || line_len == 1)
 			continue;
+
 		/* Get rid of whitespace from the front of the line. */
 		for (i = 0; i < line_len; i++) {
 			if (!isspace(line_buf[i]))
 				break;
 		}
 
-
 		if (i >= line_len)
 			continue;
+
 		/* Check if the line isn't empty and isn't a comment */
 		if (line_buf[i] == '#')
 			continue;
@@ -373,7 +392,9 @@ int main(int argc, char *argv[])
 		/* We have a valid line - allocate a new node. */
 		temp = (file_context_node_t *)malloc(sizeof(file_context_node_t));
 		if (!temp) {
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 		temp->next = NULL;
@@ -382,19 +403,15 @@ int main(int argc, char *argv[])
 		/* Parse out the regular expression from the line. */
 		start = i;
 
-
 		while (i < line_len && (!isspace(line_buf[i])))
 			i++;
 		finish = i;
 
-
 		regex_len = finish - start;
 
 		if (regex_len == 0) {
 			file_context_node_destroy(temp);
 			free(temp);
-
-
 			continue;
 		}
 
@@ -402,13 +419,14 @@ int main(int argc, char *argv[])
 		if (!temp->path) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 
 		/* Get rid of whitespace after the regular expression. */
 		for (; i < line_len; i++) {
-
 			if (!isspace(line_buf[i]))
 				break;
 		}
@@ -420,18 +438,21 @@ int main(int argc, char *argv[])
 		}
 
 		/* Parse out the type from the line (if it
-			*  is there). */
+		 * is there). */
 		if (line_buf[i] == '-') {
 			temp->file_type = (char *)malloc(sizeof(char) * 3);
 			if (!(temp->file_type)) {
+				file_context_node_destroy(temp);
+				free(temp);
+				free(line_buf);
 				fprintf(stderr, "Error: failure allocating memory.\n");
+				fc_free_file_context_node_list(head);
 				return 1;
 			}
 
 			if( i + 2 >= line_len ) {
 				file_context_node_destroy(temp);
 				free(temp);
-
 				continue;
 			}
 
@@ -448,7 +469,6 @@ int main(int argc, char *argv[])
 			}
 
 			if (i == line_len) {
-
 				file_context_node_destroy(temp);
 				free(temp);
 				continue;
@@ -467,24 +487,23 @@ int main(int argc, char *argv[])
 		if (!temp->context) {
 			file_context_node_destroy(temp);
 			free(temp);
+			free(line_buf);
 			fprintf(stderr, "Error: failure allocating memory.\n");
+			fc_free_file_context_node_list(head);
 			return 1;
 		}
 
 		/* Set all the data about the regular
-			*  expression. */
+		 * expression. */
 		fc_fill_data(temp);
 
 		/* Link this line of code at the end of
-			*  the linked list. */
+		 * the linked list. */
 		current->next = temp;
 		current = current->next;
 		lines++;
-
-
-		free(line_buf);
-		line_buf = NULL;
 	}
+	free(line_buf);
 	fclose(in_file);
 
 	/* Create the bucket linked list from the earlier linked list. */
@@ -492,6 +511,12 @@ int main(int argc, char *argv[])
 	bcurrent = master =
 	    (file_context_bucket_t *)
 	    malloc(sizeof(file_context_bucket_t));
+	if (!bcurrent) {
+		printf
+		    ("Error: failure allocating memory.\n");
+		fc_free_file_context_node_list(head);
+		return -1;
+	}
 	bcurrent->next = NULL;
 	bcurrent->data = NULL;
 
@@ -512,25 +537,33 @@ int main(int argc, char *argv[])
 			if (!(bcurrent->next)) {
 				printf
 				    ("Error: failure allocating memory.\n");
-				exit(-1);
+				free(head);
+				fc_free_file_context_node_list(current);
+				fc_merge_sort(master);
+				fc_free_file_context_node_list(master->data);
+				free(master);
+				return -1;
 			}
 
 			/* Make sure the new bucket thinks it's the end of the
-			 *  list. */
+			 * list. */
 			bcurrent->next->next = NULL;
 
 			bcurrent = bcurrent->next;
 		}
-
 	}
 
 	/* Sort the bucket list. */
 	fc_merge_sort(master);
 
+	free(head);
+
 	/* Open the output file. */
 	if (output_name) {
 		if (!(out_file = fopen(output_name, "w"))) {
 			printf("Error: failure opening output file for write.\n");
+			fc_free_file_context_node_list(master->data);
+			free(master);
 			return -1;
 		}
 	} else {
@@ -539,6 +572,7 @@ int main(int argc, char *argv[])
 
 	/* Output the sorted file_context linked list to the output file. */
 	current = master->data;
+
 	while (current) {
 		/* Output the path. */
 		fprintf(out_file, "%s\t\t", current->path);
@@ -551,14 +585,10 @@ int main(int argc, char *argv[])
 		/* Output the context. */
 		fprintf(out_file, "%s\n", current->context);
 
-		/* Remove the node. */
-		temp = current;
 		current = current->next;
-
-		file_context_node_destroy(temp);
-		free(temp);
-
 	}
+
+	fc_free_file_context_node_list(master->data);
 	free(master);
 
 	if (output_name) {

[refpolicy] [PATCH v6] fc_sort: memory leakages

[Chris PeBenito]

On 10/04/2017 05:02 PM, Guido Trentalancia via refpolicy wrote:
> Avoid memory leakages in the fc_sort executable (now passes
> all valgrind AND Clang static analyzer tests fine).
> 
> Some NULL pointer checks with or without associated error
> reporting.
> 
> Some white space and comment formatting fixes.
> 
> Optimization: avoid unnecessary operations (unnecessary
> memory allocation/deallocation and list copying).
> 
> Reverts 7821eb6f37d785ab6ac3bbdc39282c799ad22393 as such
> trick is no longer needed, given that all memory leakages
> have now been fixed.
> 
> This is the sixth version of this patch. Please do not use
> the first version as it introduces a serious bug.
> 
> For reference, the original issue reported by the Cland
> static analyzer is as follows:
> 
> support/fc_sort.c:494:6: warning: Potential leak of memory
> pointed to by 'head'
>              malloc(sizeof(file_context_bucket_t));


Thanks, merged.


> Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
> Acked-by: William Roberts <william.c.roberts@intel.com>
> ---
>   support/fc_sort.c |  108 ++++++++++++++++++++++++++++++++++--------------------
>   1 file changed, 69 insertions(+), 39 deletions(-)
> 
> --- a/support/fc_sort.c	2017-09-30 02:44:18.137342226 +0200
> +++ b/support/fc_sort.c	2017-09-30 21:16:04.899069510 +0200
> @@ -46,6 +46,9 @@ typedef struct file_context_node {
>   
>   void file_context_node_destroy(file_context_node_t *x)
>   {
> +	if (!x)
> +		return;
> +
>   	free(x->path);
>   	free(x->file_type);
>   	free(x->context);
> @@ -135,8 +138,6 @@ file_context_node_t *fc_merge(file_conte
>   	file_context_node_t *temp;
>   	file_context_node_t *jumpto;
>   
> -
> -
>   	/* If a is a empty list, and b is not,
>   	 *  set a as b and proceed to the end. */
>   	if (!a && b)
> @@ -164,7 +165,6 @@ file_context_node_t *fc_merge(file_conte
>   			       fc_compare(a_current->next,
>   					  b_current) != -1) {
>   
> -
>   				temp = a_current->next;
>   				a_current->next = b_current;
>   				b_current = b_current->next;
> @@ -177,7 +177,6 @@ file_context_node_t *fc_merge(file_conte
>   			a_current = jumpto;
>   		}
>   
> -
>   		/* if there is anything left in b to be inserted,
>   		   put it on the end */
>   		if (b_current) {
> @@ -209,11 +208,12 @@ file_context_node_t *fc_merge(file_conte
>    */
>   void fc_merge_sort(file_context_bucket_t *master)
>   {
> -
> -
>   	file_context_bucket_t *current;
>   	file_context_bucket_t *temp;
>   
> +	if (!master)
> +		return;
> +
>   	/* Loop until master is the only bucket left
>   	 * so that this will stop when master contains
>   	 * the sorted list. */
> @@ -222,28 +222,20 @@ void fc_merge_sort(file_context_bucket_t
>   
>   		/* This loop merges buckets two-by-two. */
>   		while (current) {
> -
>   			if (current->next) {
> -
>   				current->data =
>   				    fc_merge(current->data,
>   					     current->next->data);
>   
> -
> -
>   				temp = current->next;
>   				current->next = current->next->next;
>   
>   				free(temp);
> -
>   			}
>   
> -
>   			current = current->next;
>   		}
>   	}
> -
> -
>   }
>   
>   
> @@ -307,6 +299,25 @@ void fc_fill_data(file_context_node_t *f
>   	}
>   }
>   
> +
> +
> +/* fc_free_file_context_node_list
> + * Free the memory allocated to the linked list and its elements.
> + */
> +void fc_free_file_context_node_list(struct file_context_node *node)
> +{
> +	struct file_context_node *next;
> +
> +	while (node) {
> +		next = node->next;
> +		file_context_node_destroy(node);
> +		free(node);
> +		node = next;
> +	}
> +}
> +
> +
> +
>   /* main
>    * This program takes in two arguments, the input filename and the
>    *  output filename. The input file should be syntactically correct.
> @@ -328,7 +339,6 @@ int main(int argc, char *argv[])
>   
>   	FILE *in_file, *out_file;
>   
> -
>   	/* Check for the correct number of command line arguments. */
>   	if (argc < 2 || argc > 3) {
>   		fprintf(stderr, "Usage: %s <infile> [<outfile>]\n",argv[0]);
> @@ -348,24 +358,33 @@ int main(int argc, char *argv[])
>   
>   	/* Initialize the head of the linked list. */
>   	head = current = (file_context_node_t*)malloc(sizeof(file_context_node_t));
> +	if (!head) {
> +		fprintf(stderr, "Error: failure allocating memory.\n");
> +		return 1;
> +	}
>   	head->next = NULL;
> +	head->path = NULL;
> +	head->file_type = NULL;
> +	head->context = NULL;
>   
>   	/* Parse the file into a file_context linked list. */
>   	line_buf = NULL;
>   
>   	while ( getline(&line_buf, &buf_len, in_file) != -1 ){
>   		line_len = strlen(line_buf);
> +
>   		if( line_len == 0 || line_len == 1)
>   			continue;
> +
>   		/* Get rid of whitespace from the front of the line. */
>   		for (i = 0; i < line_len; i++) {
>   			if (!isspace(line_buf[i]))
>   				break;
>   		}
>   
> -
>   		if (i >= line_len)
>   			continue;
> +
>   		/* Check if the line isn't empty and isn't a comment */
>   		if (line_buf[i] == '#')
>   			continue;
> @@ -373,7 +392,9 @@ int main(int argc, char *argv[])
>   		/* We have a valid line - allocate a new node. */
>   		temp = (file_context_node_t *)malloc(sizeof(file_context_node_t));
>   		if (!temp) {
> +			free(line_buf);
>   			fprintf(stderr, "Error: failure allocating memory.\n");
> +			fc_free_file_context_node_list(head);
>   			return 1;
>   		}
>   		temp->next = NULL;
> @@ -382,19 +403,15 @@ int main(int argc, char *argv[])
>   		/* Parse out the regular expression from the line. */
>   		start = i;
>   
> -
>   		while (i < line_len && (!isspace(line_buf[i])))
>   			i++;
>   		finish = i;
>   
> -
>   		regex_len = finish - start;
>   
>   		if (regex_len == 0) {
>   			file_context_node_destroy(temp);
>   			free(temp);
> -
> -
>   			continue;
>   		}
>   
> @@ -402,13 +419,14 @@ int main(int argc, char *argv[])
>   		if (!temp->path) {
>   			file_context_node_destroy(temp);
>   			free(temp);
> +			free(line_buf);
>   			fprintf(stderr, "Error: failure allocating memory.\n");
> +			fc_free_file_context_node_list(head);
>   			return 1;
>   		}
>   
>   		/* Get rid of whitespace after the regular expression. */
>   		for (; i < line_len; i++) {
> -
>   			if (!isspace(line_buf[i]))
>   				break;
>   		}
> @@ -420,18 +438,21 @@ int main(int argc, char *argv[])
>   		}
>   
>   		/* Parse out the type from the line (if it
> -			*  is there). */
> +		 * is there). */
>   		if (line_buf[i] == '-') {
>   			temp->file_type = (char *)malloc(sizeof(char) * 3);
>   			if (!(temp->file_type)) {
> +				file_context_node_destroy(temp);
> +				free(temp);
> +				free(line_buf);
>   				fprintf(stderr, "Error: failure allocating memory.\n");
> +				fc_free_file_context_node_list(head);
>   				return 1;
>   			}
>   
>   			if( i + 2 >= line_len ) {
>   				file_context_node_destroy(temp);
>   				free(temp);
> -
>   				continue;
>   			}
>   
> @@ -448,7 +469,6 @@ int main(int argc, char *argv[])
>   			}
>   
>   			if (i == line_len) {
> -
>   				file_context_node_destroy(temp);
>   				free(temp);
>   				continue;
> @@ -467,24 +487,23 @@ int main(int argc, char *argv[])
>   		if (!temp->context) {
>   			file_context_node_destroy(temp);
>   			free(temp);
> +			free(line_buf);
>   			fprintf(stderr, "Error: failure allocating memory.\n");
> +			fc_free_file_context_node_list(head);
>   			return 1;
>   		}
>   
>   		/* Set all the data about the regular
> -			*  expression. */
> +		 * expression. */
>   		fc_fill_data(temp);
>   
>   		/* Link this line of code at the end of
> -			*  the linked list. */
> +		 * the linked list. */
>   		current->next = temp;
>   		current = current->next;
>   		lines++;
> -
> -
> -		free(line_buf);
> -		line_buf = NULL;
>   	}
> +	free(line_buf);
>   	fclose(in_file);
>   
>   	/* Create the bucket linked list from the earlier linked list. */
> @@ -492,6 +511,12 @@ int main(int argc, char *argv[])
>   	bcurrent = master =
>   	    (file_context_bucket_t *)
>   	    malloc(sizeof(file_context_bucket_t));
> +	if (!bcurrent) {
> +		printf
> +		    ("Error: failure allocating memory.\n");
> +		fc_free_file_context_node_list(head);
> +		return -1;
> +	}
>   	bcurrent->next = NULL;
>   	bcurrent->data = NULL;
>   
> @@ -512,25 +537,33 @@ int main(int argc, char *argv[])
>   			if (!(bcurrent->next)) {
>   				printf
>   				    ("Error: failure allocating memory.\n");
> -				exit(-1);
> +				free(head);
> +				fc_free_file_context_node_list(current);
> +				fc_merge_sort(master);
> +				fc_free_file_context_node_list(master->data);
> +				free(master);
> +				return -1;
>   			}
>   
>   			/* Make sure the new bucket thinks it's the end of the
> -			 *  list. */
> +			 * list. */
>   			bcurrent->next->next = NULL;
>   
>   			bcurrent = bcurrent->next;
>   		}
> -
>   	}
>   
>   	/* Sort the bucket list. */
>   	fc_merge_sort(master);
>   
> +	free(head);
> +
>   	/* Open the output file. */
>   	if (output_name) {
>   		if (!(out_file = fopen(output_name, "w"))) {
>   			printf("Error: failure opening output file for write.\n");
> +			fc_free_file_context_node_list(master->data);
> +			free(master);
>   			return -1;
>   		}
>   	} else {
> @@ -539,6 +572,7 @@ int main(int argc, char *argv[])
>   
>   	/* Output the sorted file_context linked list to the output file. */
>   	current = master->data;
> +
>   	while (current) {
>   		/* Output the path. */
>   		fprintf(out_file, "%s\t\t", current->path);
> @@ -551,14 +585,10 @@ int main(int argc, char *argv[])
>   		/* Output the context. */
>   		fprintf(out_file, "%s\n", current->context);
>   
> -		/* Remove the node. */
> -		temp = current;
>   		current = current->next;
> -
> -		file_context_node_destroy(temp);
> -		free(temp);
> -
>   	}
> +
> +	fc_free_file_context_node_list(master->data);
>   	free(master);
>   
>   	if (output_name) {

-- 
Chris PeBenito