From: bill.c.roberts@gmail.com (William Roberts)
To: refpolicy@oss.tresys.com
Subject: [refpolicy] [PATCH] fc_sort: memory leakages (was: Pull Request for fixing memory leak warning)
Date: Fri, 29 Sep 2017 09:30:22 -0700 [thread overview]
Message-ID: <CAFftDdpSGRtE8wc4eTy66EPN3PAm9oE=aLCB_P0NvH4kHCH+1w@mail.gmail.com> (raw)
In-Reply-To: <1506703966.24492.1.camel@trentalancia.com>
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
next prev parent reply other threads:[~2017-09-29 16:30 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-28 16:05 [refpolicy] Pull Request for fixing memory leak warning William Roberts
2017-09-29 0:12 ` [refpolicy] [PATCH] fc_sort: memory leakages (was: Pull Request for fixing memory leak warning) Guido Trentalancia
2017-09-28 22:24 ` William Roberts
2017-09-29 16:52 ` Guido Trentalancia
2017-09-29 16:30 ` William Roberts [this message]
2017-09-29 18:43 ` Guido Trentalancia
2017-09-29 17:37 ` William Roberts
2017-09-29 23:29 ` Guido Trentalancia
2017-09-29 18:35 ` [refpolicy] [PATCH v2] fc_sort: memory leakages Guido Trentalancia
2017-09-29 23:30 ` [refpolicy] [PATCH v3] " Guido Trentalancia
2017-09-30 6:03 ` [refpolicy] [PATCH v4] " Guido Trentalancia
2017-09-30 19:15 ` Guido Trentalancia
2017-09-30 22:44 ` [refpolicy] [PATCH v5] " Guido Trentalancia
2017-10-03 1:21 ` Chris PeBenito
2017-10-04 9:41 ` Guido Trentalancia
2017-10-04 18:05 ` William Roberts
2017-10-04 20:59 ` Guido Trentalancia
2017-10-04 21:02 ` [refpolicy] [PATCH v6] " Guido Trentalancia
2017-10-04 23:31 ` Chris PeBenito
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFftDdpSGRtE8wc4eTy66EPN3PAm9oE=aLCB_P0NvH4kHCH+1w@mail.gmail.com' \
--to=bill.c.roberts@gmail.com \
--cc=refpolicy@oss.tresys.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.