From: bill.c.roberts@gmail.com (William Roberts) Date: Fri, 29 Sep 2017 09:30:22 -0700 Subject: [refpolicy] [PATCH] fc_sort: memory leakages (was: Pull Request for fixing memory leak warning) In-Reply-To: <1506703966.24492.1.camel@trentalancia.com> References: <1506643940.32317.6.camel@trentalancia.com> <1506703966.24492.1.camel@trentalancia.com> Message-ID: To: refpolicy@oss.tresys.com List-Id: refpolicy.oss.tresys.com On Fri, Sep 29, 2017 at 9:52 AM, Guido Trentalancia via refpolicy 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 >> 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. 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