From: guido@trentalancia.com (Guido Trentalancia) Date: Fri, 29 Sep 2017 18:52:46 +0200 Subject: [refpolicy] [PATCH] fc_sort: memory leakages (was: Pull Request for fixing memory leak warning) In-Reply-To: References: <1506643940.32317.6.camel@trentalancia.com> Message-ID: <1506703966.24492.1.camel@trentalancia.com> To: refpolicy@oss.tresys.com List-Id: refpolicy.oss.tresys.com 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 > 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 > > --- > > 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 > > []\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