On 08/26/2016 04:14 PM, william.c.roberts@xxxxxxxxx wrote:
> From: William Roberts <william.c.roberts@xxxxxxxxx>
>
> The current process_file() code will open the file
> twice on the case of a binary file, correct this.
>
> Just open the file, determine the type, mmap and
> process.
>
> The general flow through process_file() was a bit
> difficult to read, streamline the routine to be
> more readable.
>
> This change results in a slight performance increase
> on textual files (presumable mmap) and no noticeable
> change on bin files. The object code is 39 bytes
> larger and there are 29 more lines from cloc.
I'm not sure what the motivation is for this patch.
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.
>
> 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 <william.c.roberts@xxxxxxxxx>
> ---
> 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, &mmap_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?
> + if (size > max)
> + return NULL ;
Here and elsewhere: extraneous whitespace before ;
> +
> + /* Append any given suffix */
> + strcat(current, ".");
> + strcat(current, suffix);
This could be done more simply/efficiently, e.g. use stpcpy() or snprintf().
> +
> + return current;
> +}
> +
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +
> +static FILE *open_file(const char *path, const char *suffix, size_t *size,
> + char**save_path)
> +{
> +
> + unsigned i;
> + int rc;
> + char stack_path[PATH_MAX + 1];
> +
> + struct file_details *found = NULL;
> + char found_path[sizeof(stack_path)];
> +
> + /*
> + * Rolling append of suffix, iw we try with path.suffix then the
Is "iw" supposed to be "ie"?
> + * 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);
> + if (rc >= (int) sizeof(stack_path)) {
> + errno = ENAMETOOLONG;
> + return NULL ;
> }
>
> - /* ok, read it in... */
> - len = mmap_stat.st_size;
> - len += (sysconf(_SC_PAGE_SIZE) - 1);
> - len &= ~(sysconf(_SC_PAGE_SIZE) - 1);
> + for (i = 0; i < ARRAY_SIZE(fdetails); i++) {
>
> - mmap_area = malloc(sizeof(*mmap_area));
> - if (!mmap_area) {
> - close(mmapfd);
> - return -1;
> + path = rolling_append(stack_path, fdetails[i].suffix,
> + sizeof(stack_path));
> + if (!path)
> + return NULL ;
> +
> + rc = stat(path, &fdetails[i].sb);
> + if (rc)
> + continue;
> +
> + /* first file thing found, just take it */
thing?
> + if (!found) {
> + strcpy(found_path, path);
> + found = &fdetails[i];
> + continue;
> + }
> +
> + /* next possible finds, keep picking the newest file */
> + if (fdetails[i].sb.st_mtime > found->sb.st_mtime) {
> + found = &fdetails[i];
> + strcpy(found_path, path);
> + }
> }
>
<snip>
> +
> +/*
> + * Similair to getline() but not quite the same.
Similar.
Do we truly need to roll our own getline()? fmemopen() + getline()
would probably work, but might be specific to glibc.
> + * It returns the length of the string in the size paramter,
> + * not the buffer size.
> + *
> + * Returns:
> + * 1 - EOF
> + * 0 - Success
> + * -1 - Error
> + */
> +static int get_next_newline(struct mmap_area *fp, char **line, size_t *size)
> +{
> + size_t offset = 0;
> + char *p = fp->next_addr;
> +
> + /* normal end of file condition */
> + if (!fp->next_len)
> + return 1;
> +
> + while (*p != '\n') {
> +
Extraneous empty line
> + if (offset > fp->next_len)
> + return -1;
What if the file doesn't end with a newline?
> +
> + p++;
> + offset++;
> }
p = memchr(fp->next_addr, '\n', fp->next_len);
if (p)
offset = p - fp->next_addr;
>
> - /* 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 = len;
> - mmap_area->next = data->mmap_areas;
> - data->mmap_areas = mmap_area;
> + /* capture the newline */
> + offset++;
> +
> + /* ensure no overflows on below arithmatic */
arithmetic
> + if (offset == (size_t) -1)
> + return -1;
>
> - /* check if this looks like an fcontext file */
> - rc = next_entry(&magic, mmap_area, sizeof(uint32_t));
> - if (rc < 0 || magic != SELINUX_MAGIC_COMPILED_FCONTEXT)
> + *line = malloc(offset + 1);
> + if (!*line)
> return -1;
>
> + strncpy(*line, fp->next_addr, offset);
> +
> + (*line)[offset] = '\0';
*line = strndup(fp->next_addr, offset);
> +
> + fp->next_addr = (char *) fp->next_addr + offset;
> + fp->next_len -= offset;
> + *size = offset;
> + return 0;
> +}
> +
> +static int read_text_file(struct selabel_handle *rec, const char *prefix)
> +{
> + int rc;
> + char *line;
> + size_t size;
> + unsigned lineno = 0;
> +
> + struct saved_data *saved_data = (struct saved_data *) rec->data;
> + struct mmap_area *area = saved_data->mmap_areas;
> +
> + while (get_next_newline(area, &line, &size) == 0) {
> + rc = process_line(rec, saved_data->path, prefix, line, ++lineno);
> + free(line);
So we're malloc+free'ing on every line?
> + if (rc)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int read_binary_file(struct selabel_handle *rec)
> +{
> +
Extraneous empty line
> + int err;
> + size_t len;
> + int *stem_map;
> + char *str_buf;
> + uint32_t entry_len;
> +
> + int rc = -1;
Do we really need separate err and rc variables, and if so, it is clear
which one to use at all times? Or why not switch the usage so you don't
have to rewrite so many lines unnecessarily?
> +
> + struct saved_data *data = (struct saved_data *) rec->data;
> + struct mmap_area *map_area = data->mmap_areas;
> +
> + /*
> + * We assume entry from is_binary() puts us past the uint32_t
> + * magic header
> + */
Is it worth this inconsistency? Might as well just rewind always and
recheck here.
> @@ -205,9 +383,10 @@ static int load_mmap(struct selabel_handle *rec, const char *path,
> free(str_buf);
> }
>
> + uint32_t stem_map_len;
Why move it? Not really a fan of scattering local variables around
unless there is truly a separate scope.
> /* allocate the stems_data array */
> - rc = next_entry(&stem_map_len, mmap_area, sizeof(uint32_t));
> - if (rc < 0 || !stem_map_len)
> + err = next_entry(&stem_map_len, map_area, sizeof(stem_map_len));
> + if (err < 0 || !stem_map_len)
> return -1;
>
> /*
> @@ -218,295 +397,223 @@ static int load_mmap(struct selabel_handle *rec, const char *path,
> if (!stem_map)
> return -1;
>
> + uint32_t i;
Ditto
> -err:
> - free(stem_map);
> + rc = 0;
>
> + out: free(stem_map);
Labels should be left-aligned and on a separate line from any statements.
> return rc;
> }
>
> static int process_file(const char *path, const char *suffix,
> - struct selabel_handle *rec,
> - const char *prefix, struct selabel_digest *digest)
> + struct selabel_handle *rec, const char *prefix,
> + struct selabel_digest *digest)
> {
> +
Extraneous empty line.
>
> - /*
> - * 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;
> - }
> + is_binary = fcontext_is_binary(area);
> + if (is_binary)
> + rc = read_binary_file(rec);
> + else
> + rc = read_text_file(rec, prefix);
>
> - rc = digest_add_specfile(digest, fp, NULL, sb.st_size, path);
> + if (rc < 0)
> + goto out;
>
> -out:
> - free(line_buf);
> - if (fp)
> - fclose(fp);
> + rc = digest_add_specfile(digest, fp, NULL, size, path);
> + out:
Left aligned labels.
> + /* closef() deals with mmapings */
> + fclose(fp);
> return rc;
> }
_______________________________________________
Selinux mailing list
Selinux@xxxxxxxxxxxxx
To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
