* RE: [PATCH v2] libselinux: clean up process file
2016-09-08 15:14 ` Stephen Smalley
@ 2016-09-08 19:06 ` Roberts, William C
2016-09-08 19:20 ` Roberts, William C
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Roberts, William C @ 2016-09-08 19:06 UTC (permalink / raw)
To: Stephen Smalley, selinux, seandroid-list, jwcart2
> -----Original Message-----
> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> Sent: Thursday, September 8, 2016 8:15 AM
> To: Roberts, William C <william.c.roberts@intel.com>; selinux@tycho.nsa.gov;
> seandroid-list@tycho.nsa.gov; jwcart2@tycho.nsa.gov
> Subject: Re: [PATCH v2] libselinux: clean up process file
>
> On 09/06/2016 08:07 PM, william.c.roberts@intel.com wrote:
> > From: William Roberts <william.c.roberts@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.roberts@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(&line_buf, &line_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, &mmap_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;
> > }
I just noticed this, I made this a single line if and didn't drop the braces, this
Will be in v3
> >
> > - 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, "<<none>>") && rec->validating) {
> > if (selabel_validate(rec, &spec->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,
> > - 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 char *suffix, size_t
> > +max) {
> > + size_t size;
> > + size_t suffix_size;
> > + size_t current_size;
> > +
> > + if (!suffix)
> > + return current;
> > +
> > + /*
> > + * Overflow check that the following
> > + * arithmatec will not overflow or
> > + */
>
> I think you can drop the comment, or if not, fix the spelling and drop the trailing
> or.
>
> > + current_size = strlen(current);
> > + suffix_size = strlen(suffix);
> > +
> > + size = current_size + suffix_size;
> > + if (size < current_size || size < suffix_size)
> > + return NULL;
> > +
> > + /* ensure space for the '.' and the '\0' characters. */
> > + if (size >= (SIZE_MAX - 2))
> > + return NULL;
> > +
> > + size += 2;
> > +
> > + if (size > max)
> > + return NULL;
> > +
> > + /* Append any given suffix */
> > + char *to = stpcpy(¤t[current_size], ".");
>
> Simpler as:
> char *to = current + current_size;
> *to++ = '.';
>
> > + strcat(to, suffix);
> > +
> > + return current;
> > +}
> > +
> > +static bool fcontext_is_binary(FILE *fp) {
> > uint32_t magic;
> >
> > - /* append the path suffix if we have one */
> > - if (suffix) {
> > - rc = snprintf(stack_path, sizeof(stack_path),
> > - "%s.%s", path, suffix);
> > - if (rc >= (int)sizeof(stack_path)) {
> > - errno = ENAMETOOLONG;
> > - return -1;
> > - }
> > - path = stack_path;
> > + size_t len = fread(&magic, sizeof(magic), 1, fp);
> > + rewind(fp);
> > +
> > + return (len && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
> > +}
> > +
> > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > +
> > +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.
>
> > + struct file_details *found = NULL;
> > +
> > + /*
> > + * Rolling append of suffix. Try to open with path.suffix then the
> > + * next as path.suffix.suffix and so forth.
> > + */
> > + struct file_details fdetails[2] = {
> > + { .suffix = suffix },
> > + { .suffix = "bin" }
> > + };
> > +
> > + rc = snprintf(stack_path, sizeof(stack_path), "%s", path);
>
> What does gcc yield as sizeof(stack_path) here since it is dynamically sized?
>
> > + if (rc >= (int) sizeof(stack_path)) {
> > + errno = ENAMETOOLONG;
> > + return NULL;
> > }
> >
> > - /* Open the specification file. */
> > - fp = fopen(path, "r");
> > - if (fp) {
> > - __fsetlocking(fp, FSETLOCKING_BYCALLER);
> > + for (i = 0; i < ARRAY_SIZE(fdetails); i++) {
> >
> > - if (fstat(fileno(fp), &sb) < 0)
> > - return -1;
> > - if (!S_ISREG(sb.st_mode)) {
> > - errno = EINVAL;
> > - return -1;
> > - }
> > + /* This handles the case if suffix is null */
> > + path = rolling_append(stack_path, fdetails[i].suffix,
> > + sizeof(stack_path));
> > + if (!path)
> > + return NULL;
> >
> > - magic = 0;
> > - if (fread(&magic, sizeof magic, 1, fp) != 1) {
> > - if (ferror(fp)) {
> > - errno = EINVAL;
> > - fclose(fp);
> > - return -1;
> > - }
> > - clearerr(fp);
> > - }
> > + rc = stat(path, &fdetails[i].sb);
> > + if (rc)
> > + continue;
> >
> > - if (magic == SELINUX_MAGIC_COMPILED_FCONTEXT) {
> > - /* file_contexts.bin format */
> > - fclose(fp);
> > - fp = NULL;
> > - isbinary = true;
> > - } else {
> > - rewind(fp);
> > + /* first file thing found, just take it */
> > + if (!found) {
> > + strcpy(save_path, path);
> > + found = &fdetails[i];
> > + continue;
> > }
> > - } else {
> > +
> > /*
> > - * Text file does not exist, so clear the timestamp
> > - * so that we will always pass the timestamp comparison
> > - * with the bin file in load_mmap().
> > + * Keep picking the newest file found. Where "newest"
> > + * includes equality. This provides a precedence on
> > + * secondary suffixes even when the timestamp is the
> > + * same. Ie choose file_contexts.bin over file_contexts
> > + * even if the time stamp is the same.
> > */
> > - sb.st_mtime = 0;
> > + if (fdetails[i].sb.st_mtime >= found->sb.st_mtime) {
> > + found = &fdetails[i];
> > + strcpy(save_path, path);
> > + }
> > }
> >
> > - rc = load_mmap(rec, path, &sb, isbinary, digest);
> > - if (rc == 0)
> > - goto out;
> > + if (!found) {
> > + errno = ENOENT;
> > + return NULL;
> > + }
> >
> > - if (!fp)
> > - return -1; /* no text or bin file */
> > + memcpy(sb, &found->sb, sizeof(*sb));
> > + return fopen(save_path, "r");
> > +}
> >
> > - /*
> > - * Then do detailed validation of the input and fill the spec array
> > - */
> > - lineno = 0;
> > - rc = 0;
> > - while (getline(&line_buf, &line_len, fp) > 0) {
> > - rc = process_line(rec, path, prefix, line_buf, ++lineno);
> > - if (rc)
> > - goto out;
> > - }
> > +static int process_file(const char *path, const char *suffix,
> > + struct selabel_handle *rec,
> > + const char *prefix, struct selabel_digest *digest) {
> > + int rc;
> > + struct stat sb;
> > + FILE *fp = NULL;
> > + char found_path[PATH_MAX + 1];
>
> I think this is wrong elsewhere too but technically these only need to be
> PATH_MAX since that includes the NUL terminator and also avoids unnecessarily
> crossing another page boundary.
>
> > +
> > + fp = open_file(path, suffix, found_path, sizeof(found_path), &sb);
> > + if (fp == NULL)
> > + return -1;
> >
> > - rc = digest_add_specfile(digest, fp, NULL, sb.st_size, path);
> > + rc = fcontext_is_binary(fp) ?
> > + load_mmap(fp, sb.st_size, rec, found_path) :
> > + process_text_file(fp, prefix, rec, found_path);
> > + if (rc < 0)
> > + goto out;
> >
> > + rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path);
> > out:
> > - free(line_buf);
> > - if (fp)
> > - fclose(fp);
> > + fclose(fp);
> > return rc;
> > }
> >
> >
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH v2] libselinux: clean up process file
2016-09-08 15:14 ` Stephen Smalley
2016-09-08 19:06 ` Roberts, William C
@ 2016-09-08 19:20 ` Roberts, William C
2016-09-08 19:30 ` Roberts, William C
2016-09-08 19:53 ` Roberts, William C
3 siblings, 0 replies; 8+ messages in thread
From: Roberts, William C @ 2016-09-08 19:20 UTC (permalink / raw)
To: Stephen Smalley, selinux, seandroid-list, jwcart2
> -----Original Message-----
> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> Sent: Thursday, September 8, 2016 8:15 AM
> To: Roberts, William C <william.c.roberts@intel.com>; selinux@tycho.nsa.gov;
> seandroid-list@tycho.nsa.gov; jwcart2@tycho.nsa.gov
> Subject: Re: [PATCH v2] libselinux: clean up process file
>
> On 09/06/2016 08:07 PM, william.c.roberts@intel.com wrote:
> > From: William Roberts <william.c.roberts@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.roberts@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(&line_buf, &line_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, &mmap_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, "<<none>>") && rec->validating) {
> > if (selabel_validate(rec, &spec->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,
> > - 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 char *suffix, size_t
> > +max) {
> > + size_t size;
> > + size_t suffix_size;
> > + size_t current_size;
> > +
> > + if (!suffix)
> > + return current;
> > +
> > + /*
> > + * Overflow check that the following
> > + * arithmatec will not overflow or
> > + */
>
> I think you can drop the comment, or if not, fix the spelling and drop the trailing
> or.
>
> > + current_size = strlen(current);
> > + suffix_size = strlen(suffix);
> > +
> > + size = current_size + suffix_size;
> > + if (size < current_size || size < suffix_size)
> > + return NULL;
> > +
> > + /* ensure space for the '.' and the '\0' characters. */
> > + if (size >= (SIZE_MAX - 2))
> > + return NULL;
> > +
> > + size += 2;
> > +
> > + if (size > max)
> > + return NULL;
> > +
> > + /* Append any given suffix */
> > + char *to = stpcpy(¤t[current_size], ".");
>
> Simpler as:
> char *to = current + current_size;
> *to++ = '.';
I don't think this is simpler, but ill do it.
>
> > + strcat(to, suffix);
> > +
> > + return current;
> > +}
> > +
> > +static bool fcontext_is_binary(FILE *fp) {
> > uint32_t magic;
> >
> > - /* append the path suffix if we have one */
> > - if (suffix) {
> > - rc = snprintf(stack_path, sizeof(stack_path),
> > - "%s.%s", path, suffix);
> > - if (rc >= (int)sizeof(stack_path)) {
> > - errno = ENAMETOOLONG;
> > - return -1;
> > - }
> > - path = stack_path;
> > + size_t len = fread(&magic, sizeof(magic), 1, fp);
> > + rewind(fp);
> > +
> > + return (len && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
> > +}
> > +
> > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > +
> > +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
>
> > + struct file_details *found = NULL;
> > +
> > + /*
> > + * Rolling append of suffix. Try to open with path.suffix then the
> > + * next as path.suffix.suffix and so forth.
> > + */
> > + struct file_details fdetails[2] = {
> > + { .suffix = suffix },
> > + { .suffix = "bin" }
> > + };
> > +
> > + rc = snprintf(stack_path, sizeof(stack_path), "%s", path);
>
> What does gcc yield as sizeof(stack_path) here since it is dynamically sized?
Sizeof works as expected on these. In this case len is optimized out since its static,
But even in the case of a pure dynamic stack allocation, size of still works correctly:
Here's a sample for completeness:
$ ./go 1 2 3
boo: 4096
boo: 4
CODE:
#include <stdio.h>
#include <linux/limits.h>
void foo(int len) {
char boo[len];
printf ("boo: %lu\n", sizeof(boo));
}
int main(int argc, char*argv[]) {
foo(PATH_MAX);
foo(argc);
return 0;
}
>
> > + if (rc >= (int) sizeof(stack_path)) {
> > + errno = ENAMETOOLONG;
> > + return NULL;
> > }
> >
> > - /* Open the specification file. */
> > - fp = fopen(path, "r");
> > - if (fp) {
> > - __fsetlocking(fp, FSETLOCKING_BYCALLER);
> > + for (i = 0; i < ARRAY_SIZE(fdetails); i++) {
> >
> > - if (fstat(fileno(fp), &sb) < 0)
> > - return -1;
> > - if (!S_ISREG(sb.st_mode)) {
> > - errno = EINVAL;
> > - return -1;
> > - }
> > + /* This handles the case if suffix is null */
> > + path = rolling_append(stack_path, fdetails[i].suffix,
> > + sizeof(stack_path));
> > + if (!path)
> > + return NULL;
> >
> > - magic = 0;
> > - if (fread(&magic, sizeof magic, 1, fp) != 1) {
> > - if (ferror(fp)) {
> > - errno = EINVAL;
> > - fclose(fp);
> > - return -1;
> > - }
> > - clearerr(fp);
> > - }
> > + rc = stat(path, &fdetails[i].sb);
> > + if (rc)
> > + continue;
> >
> > - if (magic == SELINUX_MAGIC_COMPILED_FCONTEXT) {
> > - /* file_contexts.bin format */
> > - fclose(fp);
> > - fp = NULL;
> > - isbinary = true;
> > - } else {
> > - rewind(fp);
> > + /* first file thing found, just take it */
> > + if (!found) {
> > + strcpy(save_path, path);
> > + found = &fdetails[i];
> > + continue;
> > }
> > - } else {
> > +
> > /*
> > - * Text file does not exist, so clear the timestamp
> > - * so that we will always pass the timestamp comparison
> > - * with the bin file in load_mmap().
> > + * Keep picking the newest file found. Where "newest"
> > + * includes equality. This provides a precedence on
> > + * secondary suffixes even when the timestamp is the
> > + * same. Ie choose file_contexts.bin over file_contexts
> > + * even if the time stamp is the same.
> > */
> > - sb.st_mtime = 0;
> > + if (fdetails[i].sb.st_mtime >= found->sb.st_mtime) {
> > + found = &fdetails[i];
> > + strcpy(save_path, path);
> > + }
> > }
> >
> > - rc = load_mmap(rec, path, &sb, isbinary, digest);
> > - if (rc == 0)
> > - goto out;
> > + if (!found) {
> > + errno = ENOENT;
> > + return NULL;
> > + }
> >
> > - if (!fp)
> > - return -1; /* no text or bin file */
> > + memcpy(sb, &found->sb, sizeof(*sb));
> > + return fopen(save_path, "r");
> > +}
> >
> > - /*
> > - * Then do detailed validation of the input and fill the spec array
> > - */
> > - lineno = 0;
> > - rc = 0;
> > - while (getline(&line_buf, &line_len, fp) > 0) {
> > - rc = process_line(rec, path, prefix, line_buf, ++lineno);
> > - if (rc)
> > - goto out;
> > - }
> > +static int process_file(const char *path, const char *suffix,
> > + struct selabel_handle *rec,
> > + const char *prefix, struct selabel_digest *digest) {
> > + int rc;
> > + struct stat sb;
> > + FILE *fp = NULL;
> > + char found_path[PATH_MAX + 1];
>
> I think this is wrong elsewhere too but technically these only need to be
> PATH_MAX since that includes the NUL terminator and also avoids unnecessarily
> crossing another page boundary.
Sure we can drop these, it felt dirty but they were there already, so I wasn't sure
about them.
>
> > +
> > + fp = open_file(path, suffix, found_path, sizeof(found_path), &sb);
> > + if (fp == NULL)
> > + return -1;
> >
> > - rc = digest_add_specfile(digest, fp, NULL, sb.st_size, path);
> > + rc = fcontext_is_binary(fp) ?
> > + load_mmap(fp, sb.st_size, rec, found_path) :
> > + process_text_file(fp, prefix, rec, found_path);
> > + if (rc < 0)
> > + goto out;
> >
> > + rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path);
> > out:
> > - free(line_buf);
> > - if (fp)
> > - fclose(fp);
> > + fclose(fp);
> > return rc;
> > }
> >
> >
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH v2] libselinux: clean up process file
2016-09-08 15:14 ` Stephen Smalley
2016-09-08 19:06 ` Roberts, William C
2016-09-08 19:20 ` Roberts, William C
@ 2016-09-08 19:30 ` Roberts, William C
2016-09-08 19:40 ` Stephen Smalley
2016-09-08 19:53 ` Roberts, William C
3 siblings, 1 reply; 8+ messages in thread
From: Roberts, William C @ 2016-09-08 19:30 UTC (permalink / raw)
To: Stephen Smalley, selinux, seandroid-list, jwcart2
<snip>
> > > + /* Append any given suffix */
> > > + char *to = stpcpy(¤t[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.
<snip>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] libselinux: clean up process file
2016-09-08 19:30 ` Roberts, William C
@ 2016-09-08 19:40 ` Stephen Smalley
2016-09-08 19:44 ` Roberts, William C
0 siblings, 1 reply; 8+ messages in thread
From: Stephen Smalley @ 2016-09-08 19:40 UTC (permalink / raw)
To: Roberts, William C, selinux, seandroid-list, jwcart2
On 09/08/2016 03:30 PM, Roberts, William C wrote:
> <snip>
>
>>>> + /* Append any given suffix */
>>>> + char *to = stpcpy(¤t[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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2] libselinux: clean up process file
2016-09-08 19:40 ` Stephen Smalley
@ 2016-09-08 19:44 ` Roberts, William C
0 siblings, 0 replies; 8+ messages in thread
From: Roberts, William C @ 2016-09-08 19:44 UTC (permalink / raw)
To: Stephen Smalley, selinux, seandroid-list, jwcart2
> -----Original Message-----
> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> Sent: Thursday, September 8, 2016 12:41 PM
> To: Roberts, William C <william.c.roberts@intel.com>; selinux@tycho.nsa.gov;
> seandroid-list@tycho.nsa.gov; jwcart2@tycho.nsa.gov
> Subject: Re: [PATCH v2] libselinux: clean up process file
>
> On 09/08/2016 03:30 PM, Roberts, William C wrote:
> > <snip>
> >
> >>>> + /* Append any given suffix */
> >>>> + char *to = stpcpy(¤t[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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2] libselinux: clean up process file
2016-09-08 15:14 ` Stephen Smalley
` (2 preceding siblings ...)
2016-09-08 19:30 ` Roberts, William C
@ 2016-09-08 19:53 ` Roberts, William C
3 siblings, 0 replies; 8+ messages in thread
From: Roberts, William C @ 2016-09-08 19:53 UTC (permalink / raw)
To: Stephen Smalley, selinux, seandroid-list, jwcart2
<snip>
> > > +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.
<snip>
^ permalink raw reply [flat|nested] 8+ messages in thread