Re: [PATCH] libselinux: clean up process file

[William Roberts]

 Also, there are some memory leaks in there; run it under valgrind, e.g.
 valgrind --leak-check=full matchpathcon /etc
>>>
>>> OK I'll run that test.
>
> I cant reproduce:
bad send... Can you send your valgrind output? Are you sure its not there
prior to my patch? The only heap alloc I add is the strdup, closef should
always be called despite any flow changes in process_file() as the caller,
init() always goes to label finish.


-- 
Respectfully,

William C Roberts
___
Seandroid-list mailing list
Seandroid-list@tycho.nsa.gov
To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov.
To get help, send an email containing "help" to 
seandroid-list-requ...@tycho.nsa.gov.

Re: [PATCH] libselinux: clean up process file

[William Roberts]

On Tue, Sep 6, 2016 at 1:22 PM, Stephen Smalley  wrote:
> On 09/06/2016 04:06 PM, William Roberts wrote:
>> On Sep 6, 2016 13:01, "Stephen Smalley" > > wrote:
>>>
>>> On 09/06/2016 11:51 AM, william.c.robe...@intel.com
>>  wrote:
>>> > From: William Roberts > >
>>> >
>>> > The current process_file() code will open the file
>>> > twice on the case of a binary file, correct this.
>>> >
>>> > The general flow through process_file() was a bit
>>> > difficult to read, streamline the routine to be
>>> > more readable.
>>> >
>>> > Detailed statistics of before and after:
>>> >
>>> > Source lines of code reported by cloc on modified files:
>>> > before: 1090
>>> > after: 1102
>>> >
>>> > Object size difference:
>>> > before: 195530 bytes
>>> > after:  195563 bytes
>>>
>>> Old logic would use the .bin file as long as it is not older than the
>>> base file; new logic will only use the .bin file if it is newer.  The
>>> end result on my system was that it was using the text file instead.
>>
>> I'm not following here. I spent a lot of time with strace comparing old
>> vs new behavior here. If x and x.bin exist, it should use x.bin if it's
>> newer or the same age as x? Is that correct?
>
> Yes.  strace before shows that it opens the text file and the bin file,
> and then proceeds to map the bin file and close the text file without
> processing it.  strace after shows that it stats them both and only
> opens the text file.  The st_mtime values are the same (possibly we
> ought to be comparing the full st_mtim with nanosecond precision
> instead, but regardless, if equal, we ought to use the bin file) because
> we now generate the bin file in the sandbox and then copy them both to
> /etc/selinux at the same time (commit
> a7334eb0de98af11ec38b6263536fa01bc2a606c).  You wouldn't see that if
> your policy was last built/updated with the older selinux userspace
> prior to that commit; if you run semodule -B after installing the
> current userspace, you will get that behavior.

Ok this should be simple enough the > should go to >= for the mtime comparison.
We can change the granularity in a separate patch, if you'd like.

>
>>
>>>
>>> Also, there are some memory leaks in there; run it under valgrind, e.g.
>>> valgrind --leak-check=full matchpathcon /etc
>>
>> OK I'll run that test.

I cant reproduce:



___
Seandroid-list mailing list
Seandroid-list@tycho.nsa.gov
To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov.
To get help, send an email containing "help" to 
seandroid-list-requ...@tycho.nsa.gov.

Re: [PATCH] libselinux: clean up process file

[Stephen Smalley]

On 09/06/2016 04:06 PM, William Roberts wrote:
> On Sep 6, 2016 13:01, "Stephen Smalley"  > wrote:
>>
>> On 09/06/2016 11:51 AM, william.c.robe...@intel.com
>  wrote:
>> > From: William Roberts  >
>> >
>> > The current process_file() code will open the file
>> > twice on the case of a binary file, correct this.
>> >
>> > The general flow through process_file() was a bit
>> > difficult to read, streamline the routine to be
>> > more readable.
>> >
>> > Detailed statistics of before and after:
>> >
>> > Source lines of code reported by cloc on modified files:
>> > before: 1090
>> > after: 1102
>> >
>> > Object size difference:
>> > before: 195530 bytes
>> > after:  195563 bytes
>>
>> Old logic would use the .bin file as long as it is not older than the
>> base file; new logic will only use the .bin file if it is newer.  The
>> end result on my system was that it was using the text file instead.
> 
> I'm not following here. I spent a lot of time with strace comparing old
> vs new behavior here. If x and x.bin exist, it should use x.bin if it's
> newer or the same age as x? Is that correct?

Yes.  strace before shows that it opens the text file and the bin file,
and then proceeds to map the bin file and close the text file without
processing it.  strace after shows that it stats them both and only
opens the text file.  The st_mtime values are the same (possibly we
ought to be comparing the full st_mtim with nanosecond precision
instead, but regardless, if equal, we ought to use the bin file) because
we now generate the bin file in the sandbox and then copy them both to
/etc/selinux at the same time (commit
a7334eb0de98af11ec38b6263536fa01bc2a606c).  You wouldn't see that if
your policy was last built/updated with the older selinux userspace
prior to that commit; if you run semodule -B after installing the
current userspace, you will get that behavior.

> 
>>
>> Also, there are some memory leaks in there; run it under valgrind, e.g.
>> valgrind --leak-check=full matchpathcon /etc
> 
> OK I'll run that test.
> 
>>
>> >
>> > Signed-off-by: William Roberts  >
>> > ---
>> >  libselinux/src/label_file.c | 264
> 
>> >  libselinux/src/label_file.h |   1 +
>> >  2 files changed, 147 insertions(+), 118 deletions(-)
>> >
>> > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
>> > index c89bb35..26b1b87 100644
>> > --- a/libselinux/src/label_file.c
>> > +++ b/libselinux/src/label_file.c
>> > @@ -97,62 +97,44 @@ static int nodups_specs(struct saved_data *data,
> const char *path)
>> >   return rc;
>> >  }
>> >
>> > -static int load_mmap(struct selabel_handle *rec, const char *path,
>> > - struct stat *sb, bool isbinary,
>> > - struct selabel_digest *digest)
>> > +static int process_text_file(FILE *fp, const char *prefix, struct
> selabel_handle *rec)
>> > +{
>> > + /*
>> > +  * Then do detailed validation of the input and fill the spec
> array
>> > +  */
>> > + int rc;
>> > + size_t line_len;
>> > + unsigned lineno = 0;
>> > + char *line_buf = NULL;
>> > + struct saved_data *data = (struct saved_data *)rec->data;
>> > +
>> > + while (getline(_buf, _len, fp) > 0) {
>> > + rc = process_line(rec, data->path, prefix, line_buf,
> ++lineno);
>> > + if (rc)
>> > + goto out;
>> > + }
>> > + rc = 0;
>> > +out:
>> > + free(line_buf);
>> > + return rc;
>> > +}
>> > +
>> > +static int load_mmap(FILE *fp, size_t len, struct selabel_handle *rec)
>> >  {
>> >   struct saved_data *data = (struct saved_data *)rec->data;
>> > - char mmap_path[PATH_MAX + 1];
>> > - int mmapfd;
>> >   int rc;
>> > - struct stat mmap_stat;
>> >   char *addr, *str_buf;
>> > - size_t len;
>> >   int *stem_map;
>> >   struct mmap_area *mmap_area;
>> >   uint32_t i, magic, version;
>> >   uint32_t entry_len, stem_map_len, regex_array_len;
>> >
>> > - if (isbinary) {
>> > - len = strlen(path);
>> > - if (len >= sizeof(mmap_path))
>> > - return -1;
>> > - strcpy(mmap_path, path);
>> > - } else {
>> > - rc = snprintf(mmap_path, sizeof(mmap_path), "%s.bin",
> path);
>> > - if (rc >= (int)sizeof(mmap_path))
>> > - return -1;
>> > - }
>> > -
>> > - mmapfd = open(mmap_path, O_RDONLY | O_CLOEXEC);
>> > - if (mmapfd < 0)
>> > - return -1;
>> > -
>> > - rc = fstat(mmapfd, _stat);
>> > - if (rc < 0) {
>> > - close(mmapfd);
>> > - return -1;
>> > - }
>> > -
>> > - /* if mmap is old, ignore it */
>> > - if 

RE: [PATCH] libselinux: clean up process file

[Roberts, William C]

> -Original Message-
> From: Roberts, William C
> Sent: Friday, September 2, 2016 10:34 AM
> To: 'Stephen Smalley' <s...@tycho.nsa.gov>; 'seli...@tycho.nsa.gov'
> <seli...@tycho.nsa.gov>; 'jwca...@tycho.nsa.gov' <jwca...@tycho.nsa.gov>;
> 'seandroid-list@tycho.nsa.gov' <seandroid-list@tycho.nsa.gov>
> Subject: RE: [PATCH] libselinux: clean up process file
> 
> 
> > since M, I think it makes sense to have this in the following order:
> > 1. patch to only open the file once
> > 2. patch to switch to stdio fmemopen
> 
> Actually looking at this, it would be write it once for item 1, then discard 
> that as
> Most of it would change for item two. So I see no value in splitting this up.
> I think we just implement item 2 and as a sideffect it won't open the file up
> multiple times.

Looking at this in the context of fmemopen() or even some of the others like 
open_memstream()
They both seem to copy data into an internal buffer, so I did not see a way, 
for instance to just
set pointers into the mapping. The hand rolled getline like routine that worked 
on the area
struct should really not be allocating and using the pointers into the map, 
that was a poor
decision.

> 
> 

___
Seandroid-list mailing list
Seandroid-list@tycho.nsa.gov
To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov.
To get help, send an email containing "help" to 
seandroid-list-requ...@tycho.nsa.gov.

RE: [PATCH] libselinux: clean up process file

[Roberts, William C]

> since M, I think it makes sense to have this in the following order:
> 1. patch to only open the file once
> 2. patch to switch to stdio fmemopen

Actually looking at this, it would be write it once for item 1, then discard 
that as
Most of it would change for item two. So I see no value in splitting this up.
I think we just implement item 2 and as a sideffect it won't open the file up
multiple times.



___
Seandroid-list mailing list
Seandroid-list@tycho.nsa.gov
To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov.
To get help, send an email containing "help" to 
seandroid-list-requ...@tycho.nsa.gov.

RE: [PATCH] libselinux: clean up process file

[Roberts, William C]

> 
> I'm not sure what the motivation is for this patch.

It simplifies and streamlines the code. The existing code will  cause ones
eyes to bleed and I was sick of the bleeding.

> In any event, I think it would be better to do it as a series of smaller 
> changes,
> preferably starting with any actual fixes/improvements and only then moving
> into code rewriting.  Some specific comments below.

Sure I'll split this into two things.
After reading everything and now knowing that bionic supports fmemopen() since 
M,
I think it makes sense to have this in the following order:
1. patch to only open the file once
2. patch to switch to stdio fmemopen

The second thing takes care of a lot of stuff and kills of area struct which is 
nice.

> 
> >
> > Detailed statistics of before and after:
> >
> > Source lines of code reported by cloc on modified files:
> > before: 1090
> > after: 1119
> >
> > Object size difference:
> > before: 195530 bytes
> > after:  195569 bytes
> >
> > Performance:
> >
> > text fc files (avg of 3 runs with selabel_open()):
> > before: 248 ms
> > after: 243 ms
> >
> > bin fc files (avg of 3 runs with selabel_open()):
> > before: 236 ms
> > after:  236 ms
> >
> > Signed-off-by: William Roberts 
> > ---
> >  libselinux/src/label_file.c | 606
> > ++--
> >  libselinux/src/label_file.h |  17 +-
> >  2 files changed, 359 insertions(+), 264 deletions(-)
> >
> > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > index c89bb35..bd65ee7 100644
> > --- a/libselinux/src/label_file.c
> > +++ b/libselinux/src/label_file.c
> > @@ -97,92 +97,270 @@ static int nodups_specs(struct saved_data *data, const
> char *path)
> > return rc;
> >  }
> >
> > -static int load_mmap(struct selabel_handle *rec, const char *path,
> > -   struct stat *sb, bool isbinary,
> > -   struct selabel_digest *digest)
> > +static int load_mmap(FILE *fp, off_t size, struct selabel_handle
> > +*rec)
> >  {
> > -   struct saved_data *data = (struct saved_data *)rec->data;
> > -   char mmap_path[PATH_MAX + 1];
> > -   int mmapfd;
> > -   int rc;
> > -   struct stat mmap_stat;
> > -   char *addr, *str_buf;
> > -   size_t len;
> > -   int *stem_map;
> > +   char *addr;
> > struct mmap_area *mmap_area;
> > -   uint32_t i, magic, version;
> > -   uint32_t entry_len, stem_map_len, regex_array_len;
> >
> > -   if (isbinary) {
> > -   len = strlen(path);
> > -   if (len >= sizeof(mmap_path))
> > -   return -1;
> > -   strcpy(mmap_path, path);
> > -   } else {
> > -   rc = snprintf(mmap_path, sizeof(mmap_path), "%s.bin", path);
> > -   if (rc >= (int)sizeof(mmap_path))
> > -   return -1;
> > -   }
> > +   struct saved_data *data = (struct saved_data *) rec->data;
> >
> > -   mmapfd = open(mmap_path, O_RDONLY | O_CLOEXEC);
> > -   if (mmapfd < 0)
> > +   addr = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
> > +   if (addr == MAP_FAILED )
> > return -1;
> >
> > -   rc = fstat(mmapfd, _stat);
> > -   if (rc < 0) {
> > -   close(mmapfd);
> > +   mmap_area = malloc(sizeof(*mmap_area));
> > +   if (!mmap_area) {
> > +   munmap(addr, size);
> > return -1;
> > }
> >
> > -   /* if mmap is old, ignore it */
> > -   if (mmap_stat.st_mtime < sb->st_mtime) {
> > -   close(mmapfd);
> > -   return -1;
> > +   /* save where we mmap'd the file to cleanup on close() */
> > +   mmap_area->addr = mmap_area->next_addr = addr;
> > +   mmap_area->len = mmap_area->next_len = size;
> > +   mmap_area->next = data->mmap_areas;
> > +   data->mmap_areas = mmap_area;
> > +
> > +   return 0;
> > +}
> > +
> > +struct file_details {
> > +   const char *suffix;
> > +   struct stat sb;
> > +};
> > +
> > +static char *rolling_append(char *current, const char *suffix, size_t
> > +max) {
> > +   size_t size;
> > +
> > +   if (!suffix)
> > +   return current;
> > +
> > +   /*
> > +* Overflow check:
> > +* current contents of tmp (stack_path) + '.' + suffix  + '\0'
> 
> Comment refers to variables that don't exist in this scope.
> 
> > +* should never be too big.
> > +*/
> > +   size = strlen(current) + 1 + strlen(suffix) + 1;
> 
> If we are worried about overflow, then are we worried about integer overflow
> above?

Initially I thought no, because we already checked that current is PATH_MAX, 
but we
Didn't check suffix, so it would be possible. So I think we should be worried 
here. I know
You said libsepol gets hit with old compilers, but what about libselinux? Can 
we use any
Intrinsics here?

> 
> > +   if (size > max)
> > +   return NULL ;
> 
> Here and elsewhere: extraneous whitespace before ;
Weird, done.
> 
> > +
> > +   /* Append any given suffix */
> > +   strcat(current, ".");
> > +   strcat(current, suffix);
> 
> This could be done more 

RE: [PATCH] libselinux: clean up process file

[Roberts, William C]

> + rc = 0;
> 
> + out: free(stem_map);

Dang eclipse formatter, now I see why libsepol has the indents for the labels 
way out,
Ill aggregate feedback and possible send v2 next week.

>   return rc;
>  }



___
Seandroid-list mailing list
Seandroid-list@tycho.nsa.gov
To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov.
To get help, send an email containing "help" to 
seandroid-list-requ...@tycho.nsa.gov.