RE: [PATCH v2] libselinux: clean up process file

2016-09-08 Thread Roberts, William C 

> > > +static FILE *open_file(const char *path, const char *suffix,
> > > +char *save_path, size_t len, struct stat *sb) {
> > > + unsigned i;
> > > + int rc;
> > > + char stack_path[len];
> >
> > Ew, what is this?  C99 magic.  Probably just make it PATH_MAX and be
> > done with it.
> 
> You already have C99 magic with mixed code and data declarations, so I don't 
> see
> the problem.
> https://gcc.gnu.org/onlinedocs/gcc/Mixed-Declarations.html
> https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html
> 

You also seem to have more magic with inline, when passing
CFLAGS='-std=c90' to make it complains about:

cc -std=c90 -I../include -I/usr/include -D_GNU_SOURCE   -c -o booleans.o 
booleans.c
In file included from avc.c:14:0:
avc_internal.h:36:15: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ 
before ‘void’
 static inline void set_callbacks(const struct avc_memory_callback *mem_cb,

Looking at:
https://gcc.gnu.org/onlinedocs/gcc/Inline.html
http://stackoverflow.com/questions/12151168/does-ansi-c-not-know-the-inline-keyword

It appears you need something like __inline__

If this code base was met to compile C90, it already failed and is only C99 
compliant + GNUC extensions.
So I don't see the harm in using the C99 capabilities.




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

RE: [PATCH v2] libselinux: clean up process file

2016-09-08 Thread Roberts, William C 


> -Original Message-
> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> Sent: Thursday, September 8, 2016 12:41 PM
> To: Roberts, William C <william.c.robe...@intel.com>; selinux@tycho.nsa.gov;
> seandroid-l...@tycho.nsa.gov; jwca...@tycho.nsa.gov
> Subject: Re: [PATCH v2] libselinux: clean up process file
> 
> On 09/08/2016 03:30 PM, Roberts, William C wrote:
> > 
> >
> >>>> +/* Append any given suffix */
> >>>> +char *to = stpcpy([current_size], ".");
> >>>
> >>> Simpler as:
> >>>   char *to = current + current_size;
> >>>   *to++ = '.';
> >>
> >> I don't think this is simpler, but I'll do it.
> >
> > Doing that as is gets us this:
> > ==26050== Conditional jump or move depends on uninitialised value(s)
> > ==26050==at 0x4C2DD9A: strcat (in /usr/lib/valgrind/vgpreload_memcheck-
> amd64-linux.so)
> > ==26050==by 0x4E4B6D8: rolling_append (label_file.c:429)
> > ==26050==by 0x4E4B8A3: open_file (label_file.c:472)
> > ==26050==by 0x4E4BAC6: process_file (label_file.c:519)
> > ==26050==by 0x4E4BE6D: init (label_file.c:582)
> > ==26050==by 0x4E4D02B: selabel_file_init (label_file.c:965)
> > ==26050==by 0x4E481F5: selabel_open (label.c:340)
> > ==26050==by 0x4E513E0: matchpathcon_init_prefix (matchpathcon.c:322)
> > ==26050==by 0x4E51725: matchpathcon (matchpathcon.c:413)
> > ==26050==by 0x400D86: printmatchpathcon (matchpathcon.c:26)
> > ==26050==by 0x40141F: main (matchpathcon.c:196)
> >
> > Because strcat() needs to fastforward to the null byte, this would
> > need to change to an strcpy.
> 
> Yes, sorry - switch the strcat() to strcpy() too.  Simpler and more 
> efficient, and if
> you had to append further components, you could use
> stpcpy() rather than strcpy() each time until the last one.
> 
FYI: That change actually dropped off aprox 100 bytes on the obj code.


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

Re: [PATCH v2] libselinux: clean up process file

2016-09-08 Thread Stephen Smalley 
On 09/08/2016 03:30 PM, Roberts, William C wrote:
> 
> 
 +  /* Append any given suffix */
 +  char *to = stpcpy([current_size], ".");
>>>
>>> Simpler as:
>>> char *to = current + current_size;
>>> *to++ = '.';
>>
>> I don't think this is simpler, but I'll do it.
> 
> Doing that as is gets us this:
> ==26050== Conditional jump or move depends on uninitialised value(s)
> ==26050==at 0x4C2DD9A: strcat (in 
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==26050==by 0x4E4B6D8: rolling_append (label_file.c:429)
> ==26050==by 0x4E4B8A3: open_file (label_file.c:472)
> ==26050==by 0x4E4BAC6: process_file (label_file.c:519)
> ==26050==by 0x4E4BE6D: init (label_file.c:582)
> ==26050==by 0x4E4D02B: selabel_file_init (label_file.c:965)
> ==26050==by 0x4E481F5: selabel_open (label.c:340)
> ==26050==by 0x4E513E0: matchpathcon_init_prefix (matchpathcon.c:322)
> ==26050==by 0x4E51725: matchpathcon (matchpathcon.c:413)
> ==26050==by 0x400D86: printmatchpathcon (matchpathcon.c:26)
> ==26050==by 0x40141F: main (matchpathcon.c:196)
> 
> Because strcat() needs to fastforward to the null byte, this would need to
> change to an strcpy.

Yes, sorry - switch the strcat() to strcpy() too.  Simpler and more
efficient, and if you had to append further components, you could use
stpcpy() rather than strcpy() each time until the last one.


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

RE: [PATCH v2] libselinux: clean up process file

2016-09-08 Thread Roberts, William C 


> > > + /* Append any given suffix */
> > > + char *to = stpcpy([current_size], ".");
> >
> > Simpler as:
> > char *to = current + current_size;
> > *to++ = '.';
> 
> I don't think this is simpler, but I'll do it.

Doing that as is gets us this:
==26050== Conditional jump or move depends on uninitialised value(s)
==26050==at 0x4C2DD9A: strcat (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==26050==by 0x4E4B6D8: rolling_append (label_file.c:429)
==26050==by 0x4E4B8A3: open_file (label_file.c:472)
==26050==by 0x4E4BAC6: process_file (label_file.c:519)
==26050==by 0x4E4BE6D: init (label_file.c:582)
==26050==by 0x4E4D02B: selabel_file_init (label_file.c:965)
==26050==by 0x4E481F5: selabel_open (label.c:340)
==26050==by 0x4E513E0: matchpathcon_init_prefix (matchpathcon.c:322)
==26050==by 0x4E51725: matchpathcon (matchpathcon.c:413)
==26050==by 0x400D86: printmatchpathcon (matchpathcon.c:26)
==26050==by 0x40141F: main (matchpathcon.c:196)

Because strcat() needs to fastforward to the null byte, this would need to
change to an strcpy.




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

RE: [PATCH v2] libselinux: clean up process file

2016-09-08 Thread Roberts, William C 


> -Original Message-
> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> Sent: Thursday, September 8, 2016 8:15 AM
> To: Roberts, William C <william.c.robe...@intel.com>; selinux@tycho.nsa.gov;
> seandroid-l...@tycho.nsa.gov; jwca...@tycho.nsa.gov
> Subject: Re: [PATCH v2] libselinux: clean up process file
> 
> On 09/06/2016 08:07 PM, william.c.robe...@intel.com wrote:
> > From: William Roberts <william.c.robe...@intel.com>
> >
> > 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: 735
> > after: 740
> >
> > Object size difference:
> > before: 195530 bytes
> > after:  195529 bytes
> >
> > Signed-off-by: William Roberts <william.c.robe...@intel.com>
> > ---
> >  libselinux/src/label_file.c | 263
> > 
> >  1 file changed, 145 insertions(+), 118 deletions(-)
> >
> > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > index c89bb35..6839a77 100644
> > --- a/libselinux/src/label_file.c
> > +++ b/libselinux/src/label_file.c
> > @@ -97,62 +97,43 @@ 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, const char *path) {
> > +   /*
> > +* Then do detailed validation of the input and fill the spec array
> > +*/
> 
> You can drop this comment; it is no longer helpful/relevant.
> 
> > +   int rc;
> > +   size_t line_len;
> > +   unsigned lineno = 0;
> > +   char *line_buf = NULL;
> > +
> > +   while (getline(_buf, _len, fp) > 0) {
> > +   rc = process_line(rec, 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, const char *path)
> >  {
> > 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 (mmap_stat.st_mtime < sb->st_mtime) {
> > -   close(mmapfd);
> > -   return -1;
> > -   }
> > -
> > -   /* ok, read it in... */
> > -   len = mmap_stat.st_size;
> > -   len += (sysconf(_SC_PAGE_SIZE) - 1);
> > -   len &= ~(sysconf(_SC_PAGE_SIZE) - 1);
> > -
> > mmap_area = malloc(sizeof(*mmap_area));
> > if (!mmap_area) {
> > -   close(mmapfd);
> > return -1;
> > }
> >
> > -   addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, mmapfd, 0);
> > -   close(mmapfd);
> > +   addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
> > if (addr == MAP_FAILED) {
> > free(mmap_area);
> > perror("mmap");
&g

Re: [PATCH v2] libselinux: clean up process file

2016-09-08 Thread Stephen Smalley 
On 09/06/2016 08:07 PM, 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: 735
> after: 740
> 
> Object size difference:
> before: 195530 bytes
> after:  195529 bytes
> 
> Signed-off-by: William Roberts 
> ---
>  libselinux/src/label_file.c | 263 
> 
>  1 file changed, 145 insertions(+), 118 deletions(-)
> 
> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> index c89bb35..6839a77 100644
> --- a/libselinux/src/label_file.c
> +++ b/libselinux/src/label_file.c
> @@ -97,62 +97,43 @@ 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, const char *path)
> +{
> + /*
> +  * Then do detailed validation of the input and fill the spec array
> +  */

You can drop this comment; it is no longer helpful/relevant.

> + int rc;
> + size_t line_len;
> + unsigned lineno = 0;
> + char *line_buf = NULL;
> +
> + while (getline(_buf, _len, fp) > 0) {
> + rc = process_line(rec, 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, const 
> char *path)
>  {
>   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 (mmap_stat.st_mtime < sb->st_mtime) {
> - close(mmapfd);
> - return -1;
> - }
> -
> - /* ok, read it in... */
> - len = mmap_stat.st_size;
> - len += (sysconf(_SC_PAGE_SIZE) - 1);
> - len &= ~(sysconf(_SC_PAGE_SIZE) - 1);
> -
>   mmap_area = malloc(sizeof(*mmap_area));
>   if (!mmap_area) {
> - close(mmapfd);
>   return -1;
>   }
>  
> - addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, mmapfd, 0);
> - close(mmapfd);
> + addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
>   if (addr == MAP_FAILED) {
>   free(mmap_area);
>   perror("mmap");
> @@ -306,7 +287,7 @@ static int load_mmap(struct selabel_handle *rec, const 
> char *path,
>   if (strcmp(spec->lr.ctx_raw, "<>") && rec->validating) {
>   if (selabel_validate(rec, >lr) < 0) {
>   selinux_log(SELINUX_ERROR,
> - "%s: context %s is invalid\n", 
> mmap_path, spec->lr.ctx_raw);
> + "%s: context %s is invalid\n", 
> path, spec->lr.ctx_raw);
>   goto err;
>   }
>   }
> @@ -408,105 +389,151 @@ static int load_mmap(struct selabel_handle *rec, 
> const char *path,
>   data->nspec++;
>   }
>  
> - rc = digest_add_specfile(digest, NULL, addr, mmap_stat.st_size,
> - mmap_path);
> - if (rc)
> - goto err;
> -

We should explicitly set rc = 0 here.

>  err:

And maybe this should be out: since it is the only exit path and not
only for errors.

>   free(stem_map);
>  
>   return rc;
>  }
>  
> -static int process_file(const char *path, const char *suffix,
> -   struct selabel_handle *rec,
> -

[PATCH v2] libselinux: clean up process file

2016-09-06 Thread william . c . roberts 
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: 735
after: 740

Object size difference:
before: 195530 bytes
after:  195529 bytes

Signed-off-by: William Roberts 
---
 libselinux/src/label_file.c | 263 
 1 file changed, 145 insertions(+), 118 deletions(-)

diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index c89bb35..6839a77 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -97,62 +97,43 @@ 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, const char *path)
+{
+   /*
+* 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;
+
+   while (getline(_buf, _len, fp) > 0) {
+   rc = process_line(rec, 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, const 
char *path)
 {
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 (mmap_stat.st_mtime < sb->st_mtime) {
-   close(mmapfd);
-   return -1;
-   }
-
-   /* ok, read it in... */
-   len = mmap_stat.st_size;
-   len += (sysconf(_SC_PAGE_SIZE) - 1);
-   len &= ~(sysconf(_SC_PAGE_SIZE) - 1);
-
mmap_area = malloc(sizeof(*mmap_area));
if (!mmap_area) {
-   close(mmapfd);
return -1;
}
 
-   addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, mmapfd, 0);
-   close(mmapfd);
+   addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
if (addr == MAP_FAILED) {
free(mmap_area);
perror("mmap");
@@ -306,7 +287,7 @@ static int load_mmap(struct selabel_handle *rec, const char 
*path,
if (strcmp(spec->lr.ctx_raw, "<>") && rec->validating) {
if (selabel_validate(rec, >lr) < 0) {
selinux_log(SELINUX_ERROR,
-   "%s: context %s is invalid\n", 
mmap_path, spec->lr.ctx_raw);
+   "%s: context %s is invalid\n", 
path, spec->lr.ctx_raw);
goto err;
}
}
@@ -408,105 +389,151 @@ static int load_mmap(struct selabel_handle *rec, const 
char *path,
data->nspec++;
}
 
-   rc = digest_add_specfile(digest, NULL, addr, mmap_stat.st_size,
-   mmap_path);
-   if (rc)
-   goto err;
-
 err:
free(stem_map);
 
return rc;
 }
 
-static int process_file(const char *path, const char *suffix,
- struct selabel_handle *rec,
- const char *prefix, struct selabel_digest *digest)
-{
-   FILE *fp;
+struct file_details {
+   const char *suffix;
struct stat sb;
-   unsigned int lineno;
-   size_t line_len = 0;
-   char *line_buf = NULL;
-   int rc;
-   char stack_path[PATH_MAX + 1];
-   bool isbinary = false;
+};
+
+static char *rolling_append(char *current, const