On Sep 6, 2016 13:01, "Stephen Smalley" <[email protected]> wrote: > > On 09/06/2016 11:51 AM, [email protected] wrote: > > From: William Roberts <[email protected]> > > > > 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? > > 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 <[email protected]> > > --- > > 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(&line_buf, &line_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, &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 +288,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", data->path, spec->lr.ctx_raw); > > goto err; > > } > > } > > @@ -408,105 +390,150 @@ 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; > > +}; > > + > > +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 > > + */ > > + 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], "."); > > + strcat(to, suffix); > > + > > + return current; > > +} > > + > > +static bool fcontext_is_binary(FILE *fp) > > +{ > > + uint32_t magic; > > + > > + 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, struct stat *sb) > > +{ > > + unsigned i; > > int rc; > > char stack_path[PATH_MAX + 1]; > > - bool isbinary = false; > > - 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; > > + struct file_details *found = NULL; > > + char found_path[sizeof(stack_path)]; > > + > > + /* > > + * 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); > > + 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; > > + > > + /* first file thing found, just take it */ > > + if (!found) { > > + strcpy(found_path, path); > > + found = &fdetails[i]; > > + continue; > > } > > > > - if (magic == SELINUX_MAGIC_COMPILED_FCONTEXT) { > > - /* file_contexts.bin format */ > > - fclose(fp); > > - fp = NULL; > > - isbinary = true; > > - } else { > > - rewind(fp); > > + /* 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); > > } > > - } 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(). > > - */ > > - sb.st_mtime = 0; > > } > > > > - rc = load_mmap(rec, path, &sb, isbinary, digest); > > - if (rc == 0) > > - goto out; > > + if (!found) { > > + errno = ENOENT; > > + return NULL; > > + } > > + *save_path = strdup(found_path); > > + if (!*save_path) > > + return NULL; > > > > - if (!fp) > > - return -1; /* no text or bin file */ > > + memcpy(sb, &found->sb, sizeof(*sb)); > > + return fopen(found_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) > > +{ > > + FILE *fp; > > + struct stat sb; > > + int rc; > > + struct saved_data *data = (struct saved_data *)rec->data; > > > > - rc = digest_add_specfile(digest, fp, NULL, sb.st_size, path); > > + fp = open_file(path, suffix, &data->path, &sb); > > + if (fp == NULL) > > + return -1; > > > > + rc = fcontext_is_binary(fp) ? > > + load_mmap(fp, sb.st_size, rec) : > > + process_text_file(fp, prefix, rec); > > + if (rc < 0) > > + goto out; > > + > > + rc = digest_add_specfile(digest, fp, NULL, sb.st_size, path); > > out: > > - free(line_buf); > > - if (fp) > > - fclose(fp); > > + fclose(fp); > > return rc; > > } > > > > @@ -634,6 +661,7 @@ static void closef(struct selabel_handle *rec) > > area = area->next; > > free(last_area); > > } > > + free(data->path); > > free(data); > > } > > > > diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h > > index 6d1e890..fe5dc60 100644 > > --- a/libselinux/src/label_file.h > > +++ b/libselinux/src/label_file.h > > @@ -76,6 +76,7 @@ struct saved_data { > > int num_stems; > > int alloc_stems; > > struct mmap_area *mmap_areas; > > + char *path; > > }; > > > > static inline pcre_extra *get_pcre_extra(struct spec *spec) > > > > _______________________________________________ > Seandroid-list mailing list > [email protected] > To unsubscribe, send email to [email protected]. > To get help, send an email containing "help" to [email protected].
_______________________________________________ Seandroid-list mailing list [email protected] To unsubscribe, send email to [email protected]. To get help, send an email containing "help" to [email protected].
