From mboxrd@z Thu Jan  1 00:00:00 1970
From: bill.c.roberts@gmail.com (William Roberts)
Date: Thu, 28 Sep 2017 15:24:14 -0700
Subject: [refpolicy] [PATCH] fc_sort: memory leakages (was: Pull Request
 for fixing memory leak warning)
In-Reply-To: <1506643940.32317.6.camel@trentalancia.com>
References: <CAFftDdr6uZxWz9FPBnP4RW0Y-KLcS7aFTq0hHaF_dNB6K8wi2g@mail.gmail.com>
	<1506643940.32317.6.camel@trentalancia.com>
Message-ID: <CAFftDdqy_BxjsPZjqqPFQZxhVA8sHG1Fq1adfCgjKAkuBsNk-w@mail.gmail.com>
To: refpolicy@oss.tresys.com
List-Id: refpolicy.oss.tresys.com

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