[RFC] mmap file_contexts and property_contexts:

Commit Message

Roberts, William C Sept. 19, 2016, 9:45 p.m. UTC

From: William Roberts <[e-mail]>

THIS IS WIP...

Rather than using stdio and making copies, just mmap the files
and use the pointers in place. The affect of this change, is that
text file load time is now faster than binary load time by 4.7%
when testing with a file_contexts file from the Android tree. Note
that the Android doesn't use monstrous regexs.

Times are the average of 3 runs.

BEFORE:
Text file allocs: 114803
Text file load time: 0.266101
Bin file allocs: 93073
Bin file load time: 0.248757667

AFTER:
Text file allocs: 103933
Text file load time: 0.236192667
Bin file allocs: 87645
Bin file load time: .247607333

THINGS TO DO:
1. What's arm performance like?
2. What interfaces to backends are busted by this (if any)?
3. Test Android Properties
4. Im pretty sure this breaks sefcontext_compile, fix.
5. Test with PCRE2 enabled.
6. Spell check this message!
7. Run checkpatch

Signed-off-by: William Roberts <[e-mail]>
---
 libselinux/src/label.c                  |   2 -
 libselinux/src/label_android_property.c |  22 ++---
 libselinux/src/label_file.c             | 140 +++++++++++++++++++-------------
 libselinux/src/label_file.h             |  66 +++++++++------
 libselinux/src/label_internal.h         |   3 +-
 libselinux/src/label_support.c          |  79 ++++++++----------
 6 files changed, 172 insertions(+), 140 deletions(-)

Comments

Roberts, William C Sept. 19, 2016, 9:51 p.m. UTC | #1

FYI I only tested this with checkfc...

> -----Original Message-----
> From: Roberts, William C
> Sent: Monday, September 19, 2016 2:45 PM
> To: [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> Cc: Roberts, William C <[e-mail]>
> Subject: [RFC] mmap file_contexts and property_contexts:
> 
> From: William Roberts <[e-mail]>
> 
> THIS IS WIP...
> 
> Rather than using stdio and making copies, just mmap the files and use the
> pointers in place. The affect of this change, is that text file load time is now faster
> than binary load time by 4.7% when testing with a file_contexts file from the
> Android tree. Note that the Android doesn't use monstrous regexs.
> 
> Times are the average of 3 runs.
> 
> BEFORE:
> Text file allocs: 114803
> Text file load time: 0.266101
> Bin file allocs: 93073
> Bin file load time: 0.248757667
> 
> AFTER:
> Text file allocs: 103933
> Text file load time: 0.236192667
> Bin file allocs: 87645
> Bin file load time: .247607333
> 
> THINGS TO DO:
> 1. What's arm performance like?
> 2. What interfaces to backends are busted by this (if any)?
> 3. Test Android Properties
> 4. Im pretty sure this breaks sefcontext_compile, fix.
> 5. Test with PCRE2 enabled.
> 6. Spell check this message!
> 7. Run checkpatch
> 
> Signed-off-by: William Roberts <[e-mail]>
> ---
>  libselinux/src/label.c                  |   2 -
>  libselinux/src/label_android_property.c |  22 ++---
>  libselinux/src/label_file.c             | 140 +++++++++++++++++++-------------
>  libselinux/src/label_file.h             |  66 +++++++++------
>  libselinux/src/label_internal.h         |   3 +-
>  libselinux/src/label_support.c          |  79 ++++++++----------
>  6 files changed, 172 insertions(+), 140 deletions(-)
> 
> diff --git a/libselinux/src/label.c b/libselinux/src/label.c index 963bfcb..d767b49
> 100644
> --- a/libselinux/src/label.c
> +++ b/libselinux/src/label.c
> @@ -15,8 +15,6 @@
>  #include "callbacks.h"
>  #include "label_internal.h"
> 
> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> -
>  typedef int (*selabel_initfunc)(struct selabel_handle *rec,
>  				const struct selinux_opt *opts,
>  				unsigned nopts);
> diff --git a/libselinux/src/label_android_property.c
> b/libselinux/src/label_android_property.c
> index 290b438..2aac394 100644
> --- a/libselinux/src/label_android_property.c
> +++ b/libselinux/src/label_android_property.c
> @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec,
>  			int pass, unsigned lineno)
>  {
>  	int items;
> -	char *prop = NULL, *context = NULL;
> +	union {
> +		struct {
> +			char *prop;
> +			char *context;
> +		};
> +		char *array[2];
> +	} found = { .array = { 0 } };
>  	struct saved_data *data = (struct saved_data *)rec->data;
>  	spec_t *spec_arr = data->spec_arr;
>  	unsigned int nspec = data->nspec;
>  	const char *errbuf = NULL;
> 
> -	items = read_spec_entries(line_buf, &errbuf, 2, &prop, &context);
> +	items = read_spec_entries(line_buf, &errbuf, ARRAY_SIZE(found.array),
> +found.array);
>  	if (items < 0) {
>  		items = errno;
>  		selinux_log(SELINUX_ERROR,
> @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle *rec,
>  		selinux_log(SELINUX_ERROR,
>  			    "%s:  line %u is missing fields\n", path,
>  			    lineno);
> -		free(prop);
>  		errno = EINVAL;
>  		return -1;
>  	}
> 
> -	if (pass == 0) {
> -		free(prop);
> -		free(context);
> -	} else if (pass == 1) {
> +	if (pass == 1) {
>  		/* On the second pass, process and store the specification in
> spec. */
> -		spec_arr[nspec].property_key = prop;
> -		spec_arr[nspec].lr.ctx_raw = context;
> +		spec_arr[nspec].property_key = found.prop;
> +		spec_arr[nspec].lr.ctx_raw = found.context;
> 
>  		if (rec->validating) {
>  			if (selabel_validate(rec, &spec_arr[nspec].lr) < 0) { @@ -
> 234,7 +236,7 @@ static void closef(struct selabel_handle *rec)
>  	for (i = 0; i < data->nspec; i++) {
>  		spec = &data->spec_arr[i];
>  		free(spec->property_key);
> -		free(spec->lr.ctx_raw);
> +		//free(spec->lr.ctx_raw);
>  		free(spec->lr.ctx_trans);
>  	}
> 
> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c index
> 7156825..4dc440e 100644
> --- a/libselinux/src/label_file.c
> +++ b/libselinux/src/label_file.c
> @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data, const
> char *path)
>  	return rc;
>  }
> 
> -static int process_text_file(FILE *fp, const char *prefix,
> -			     struct selabel_handle *rec, const char *path)
> +static inline struct saved_data *rec_to_data(struct selabel_handle
> +*rec) {
> +	return (struct saved_data *)rec->data; }
> +
> +static char *mmap_area_get_line(struct mmap_area *area) {
> +	size_t len = area->next_len;
> +	if (!len)
> +		return NULL;
> +
> +	char *start = area->next_addr;
> +	char *end = memchr(start, '\n', len);
> +
> +	/* the file may not end with a newline */
> +	if (!end)
> +		end = (char *)area->next_addr + len - 1;
> +
> +	*end = '\0';
> +	/* len includes null byte */
> +	len = end - start;
> +
> +	area->next_len -= len + 1;
> +	area->next_addr = ++end;
> +	return start;
> +}
> +
> +static int process_text_file(const char *path, const char *prefix,
> +		struct selabel_handle *rec)
>  {
>  	int rc;
> -	size_t line_len;
>  	unsigned int lineno = 0;
> -	char *line_buf = NULL;
> +	char *line_buf;
> +	struct mmap_area *areas = rec_to_data(rec)->mmap_areas;
> +
> +	/* mmap_area_get_line() and process_line() require mutable string
> pointers */
> +	rc = mprotect(areas->addr, areas->len, PROT_READ | PROT_WRITE);
> +	if (rc < 0)
> +		goto out;
> 
> -	while (getline(&line_buf, &line_len, fp) > 0) {
> +	while ( (line_buf = mmap_area_get_line(areas)) ) {
>  		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)
> +static int mmap_file(FILE *fp, size_t len, struct saved_data *data)
>  {
> -	struct saved_data *data = (struct saved_data *)rec->data;
> -	int rc;
> -	char *addr, *str_buf;
> -	int *stem_map;
> -	struct mmap_area *mmap_area;
> -	uint32_t i, magic, version;
> -	uint32_t entry_len, stem_map_len, regex_array_len;
> -	const char *reg_version;
> -
> -	mmap_area = malloc(sizeof(*mmap_area));
> +	struct mmap_area *mmap_area = malloc(sizeof(*mmap_area));
>  	if (!mmap_area) {
>  		return -1;
>  	}
> 
> -	addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
> +	void *addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
>  	if (addr == MAP_FAILED) {
>  		free(mmap_area);
>  		perror("mmap");
> @@ -145,17 +166,26 @@ static int load_mmap(FILE *fp, size_t len, struct
> selabel_handle *rec,
>  	mmap_area->next = data->mmap_areas;
>  	data->mmap_areas = mmap_area;
> 
> -	/* 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)
> -		return -1;
> +	return 0;
> +}
> +
> +static int process_binary_file(const char *path, struct selabel_handle
> +*rec) {
> +	struct saved_data *data = (struct saved_data *)rec->data;
> +	int rc;
> +	char *str_buf;
> +	int *stem_map;
> +	struct mmap_area *mmap_area = data->mmap_areas;
> +	uint32_t i, version;
> +	uint32_t entry_len, stem_map_len, regex_array_len;
> +	size_t len;
> 
>  	/* check if this version is higher than we understand */
>  	rc = next_entry(&version, mmap_area, sizeof(uint32_t));
>  	if (rc < 0 || version > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
>  		return -1;
> 
> -	reg_version = regex_version();
> +	const char *reg_version = regex_version();
>  	if (!reg_version)
>  		return -1;
> 
> @@ -235,12 +265,11 @@ static int load_mmap(FILE *fp, size_t len, struct
> selabel_handle *rec,
>  		/* store the mapping between old and new */
>  		newid = find_stem(data, buf, stem_len);
>  		if (newid < 0) {
> -			newid = store_stem(data, buf, stem_len);
> +			newid = store_stem(data, buf, stem_len, true);
>  			if (newid < 0) {
>  				rc = newid;
>  				goto out;
>  			}
> -			data->stem_arr[newid].from_mmap = 1;
>  		}
>  		stem_map[i] = newid;
>  	}
> @@ -262,7 +291,6 @@ static int load_mmap(FILE *fp, size_t len, struct
> selabel_handle *rec,
>  			goto out;
> 
>  		spec = &data->spec_arr[data->nspec];
> -		spec->from_mmap = 1;
> 
>  		/* Process context */
>  		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t)); @@ -
> 271,22 +299,10 @@ static int load_mmap(FILE *fp, size_t len, struct
> selabel_handle *rec,
>  			goto out;
>  		}
> 
> -		str_buf = malloc(entry_len);
> -		if (!str_buf) {
> -			rc = -1;
> -			goto out;
> -		}
> -		rc = next_entry(str_buf, mmap_area, entry_len);
> +		rc = next_pstr(&spec->lr.ctx_raw, mmap_area, entry_len);
>  		if (rc < 0)
>  			goto out;
> 
> -		if (str_buf[entry_len - 1] != '\0') {
> -			free(str_buf);
> -			rc = -1;
> -			goto out;
> -		}
> -		spec->lr.ctx_raw = str_buf;
> -
>  		if (strcmp(spec->lr.ctx_raw, "<<none>>") && rec->validating) {
>  			if (selabel_validate(rec, &spec->lr) < 0) {
>  				selinux_log(SELINUX_ERROR,
> @@ -401,17 +417,29 @@ static char *rolling_append(char *current, const char
> *suffix, size_t max)
>  	return current;
>  }
> 
> -static bool fcontext_is_binary(FILE *fp)
> +static inline int mmap_area_rewind(struct mmap_area *area, size_t
> +bytes) {
> +	if (area->next_len + bytes > area->len)
> +		return -1;
> +
> +	area->next_addr = (char *) area->next_addr - bytes;
> +	area->next_len += bytes;
> +	return 0;
> +}
> +
> +static bool fcontext_is_binary(struct mmap_area *area)
>  {
>  	uint32_t magic;
> +	bool is_binary = false;
> 
> -	size_t len = fread(&magic, sizeof(magic), 1, fp);
> -	rewind(fp);
> +	int rc = next_entry(&magic, area, sizeof(magic));
> 
> -	return (len && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
> -}
> +	is_binary = (!rc && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
> +	if (!is_binary)
> +		mmap_area_rewind(area, sizeof(magic));
> 
> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +	return is_binary;
> +}
> 
>  static FILE *open_file(const char *path, const char *suffix,
>  	       char *save_path, size_t len, struct stat *sb, bool open_oldest) @@ -
> 504,12 +532,17 @@ static int process_file(const char *path, const char *suffix,
>  		if (fp == NULL)
>  			return -1;
> 
> -		rc = fcontext_is_binary(fp) ?
> -				load_mmap(fp, sb.st_size, rec, found_path) :
> -				process_text_file(fp, prefix, rec, found_path);
> +		rc = mmap_file(fp, sb.st_size, rec_to_data(rec));
> +		if (rc < 0) {
> +			fclose(fp);
> +			return -1;
> +		}
> +
> +		rc = fcontext_is_binary(rec_to_data(rec)->mmap_areas) ?
> +				process_binary_file(found_path, rec) :
> +				process_text_file(found_path, prefix, rec);
>  		if (!rc)
> -			rc = digest_add_specfile(digest, fp, NULL, sb.st_size,
> -				found_path);
> +			rc = digest_add_specfile(digest, fp, NULL, sb.st_size,
> found_path);
> 
>  		fclose(fp);
> 
> @@ -613,12 +646,7 @@ static void closef(struct selabel_handle *rec)
>  	for (i = 0; i < data->nspec; i++) {
>  		spec = &data->spec_arr[i];
>  		free(spec->lr.ctx_trans);
> -		free(spec->lr.ctx_raw);
>  		regex_data_free(spec->regex);
> -		if (spec->from_mmap)
> -			continue;
> -		free(spec->regex_str);
> -		free(spec->type_str);
>  	}
> 
>  	for (i = 0; i < (unsigned int)data->num_stems; i++) { diff --git
> a/libselinux/src/label_file.h b/libselinux/src/label_file.h index 88f4294..2704906
> 100644
> --- a/libselinux/src/label_file.h
> +++ b/libselinux/src/label_file.h
> @@ -37,7 +37,6 @@ struct spec {
>  	int matches;		/* number of matching pathnames */
>  	int stem_id;		/* indicates which stem-compression item */
>  	char hasMetaChars;	/* regular expression has meta-chars */
> -	char from_mmap;		/* this spec is from an mmap of the data
> */
>  	size_t prefix_len;      /* length of fixed path prefix */
>  };
> 
> @@ -255,7 +254,7 @@ static inline int find_stem(struct saved_data *data, const
> char *buf,  }
> 
>  /* returns the index of the new stored object */ -static inline int
> store_stem(struct saved_data *data, char *buf, int stem_len)
> +static inline int store_stem(struct saved_data *data, char *buf, int
> +stem_len, bool from_mmap)
>  {
>  	int num = data->num_stems;
> 
> @@ -271,7 +270,7 @@ static inline int store_stem(struct saved_data *data, char
> *buf, int stem_len)
>  	}
>  	data->stem_arr[num].len = stem_len;
>  	data->stem_arr[num].buf = buf;
> -	data->stem_arr[num].from_mmap = 0;
> +	data->stem_arr[num].from_mmap = from_mmap;
>  	data->num_stems++;
> 
>  	return num;
> @@ -298,7 +297,7 @@ static inline int find_stem_from_spec(struct saved_data
> *data, const char *buf)
>  	if (!stem)
>  		return -1;
> 
> -	return store_stem(data, stem, stem_len);
> +	return store_stem(data, stem, stem_len, false);
>  }
> 
>  /* This will always check for buffer over-runs and either read the next entry @@
> -317,6 +316,22 @@ static inline int next_entry(void *buf, struct mmap_area *fp,
> size_t bytes)
>  	return 0;
>  }
> 
> +static inline int next_pstr(char **str, struct mmap_area *fp, size_t
> +len) {
> +	if (len > fp->next_len)
> +		return -1;
> +
> +	char *tmp = (char *)fp->next_addr;
> +	if (tmp[len-1] != '\0')
> +		return -1;
> +
> +	*str = tmp;
> +
> +	fp->next_addr = (char *)fp->next_addr + len;
> +	fp->next_len -= len;
> +	return 0;
> +}
> +
>  static inline int compile_regex(struct saved_data *data, struct spec *spec,
>  					    const char **errbuf)
>  {
> @@ -375,13 +390,21 @@ static inline int process_line(struct selabel_handle *rec,
>  			char *line_buf, unsigned lineno)
>  {
>  	int items, len, rc;
> -	char *regex = NULL, *type = NULL, *context = NULL;
> +	union {
> +		struct {
> +			char *regex;
> +			char *type;
> +			char *context;
> +		};
> +		char *array[3];
> +	} found = { .array = { 0 } };
> +
>  	struct saved_data *data = (struct saved_data *)rec->data;
>  	struct spec *spec_arr;
>  	unsigned int nspec = data->nspec;
>  	const char *errbuf = NULL;
> 
> -	items = read_spec_entries(line_buf, &errbuf, 3, &regex, &type,
> &context);
> +	items = read_spec_entries(line_buf, &errbuf, ARRAY_SIZE(found.array),
> +found.array);
>  	if (items < 0) {
>  		rc = errno;
>  		selinux_log(SELINUX_ERROR,
> @@ -398,22 +421,17 @@ static inline int process_line(struct selabel_handle *rec,
>  		COMPAT_LOG(SELINUX_ERROR,
>  			    "%s:  line %u is missing fields\n", path,
>  			    lineno);
> -		if (items == 1)
> -			free(regex);
>  		errno = EINVAL;
>  		return -1;
>  	} else if (items == 2) {
>  		/* The type field is optional. */
> -		context = type;
> -		type = 0;
> +		found.context = found.type;
> +		found.type = NULL;
>  	}
> 
> -	len = get_stem_from_spec(regex);
> -	if (len && prefix && strncmp(prefix, regex, len)) {
> +	len = get_stem_from_spec(found.regex);
> +	if (len && prefix && strncmp(prefix, found.regex, len)) {
>  		/* Stem of regex does not match requested prefix, discard. */
> -		free(regex);
> -		free(type);
> -		free(context);
>  		return 0;
>  	}
> 
> @@ -424,13 +442,13 @@ static inline int process_line(struct selabel_handle *rec,
>  	spec_arr = data->spec_arr;
> 
>  	/* process and store the specification in spec. */
> -	spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
> -	spec_arr[nspec].regex_str = regex;
> +	spec_arr[nspec].stem_id = find_stem_from_spec(data, found.regex);
> +	spec_arr[nspec].regex_str = found.regex;
> 
> -	spec_arr[nspec].type_str = type;
> +	spec_arr[nspec].type_str = found.type;
>  	spec_arr[nspec].mode = 0;
> 
> -	spec_arr[nspec].lr.ctx_raw = context;
> +	spec_arr[nspec].lr.ctx_raw = found.context;
> 
>  	/*
>  	 * bump data->nspecs to cause closef() to cover it in its free @@ -442,18
> +460,18 @@ static inline int process_line(struct selabel_handle *rec,
>  			&& compile_regex(data, &spec_arr[nspec], &errbuf)) {
>  		COMPAT_LOG(SELINUX_ERROR,
>  			   "%s:  line %u has invalid regex %s:  %s\n",
> -			   path, lineno, regex, errbuf);
> +			   path, lineno, found.regex, errbuf);
>  		errno = EINVAL;
>  		return -1;
>  	}
> 
> -	if (type) {
> -		mode_t mode = string_to_mode(type);
> +	if (found.type) {
> +		mode_t mode = string_to_mode(found.type);
> 
>  		if (mode == (mode_t)-1) {
>  			COMPAT_LOG(SELINUX_ERROR,
>  				   "%s:  line %u has invalid file type %s\n",
> -				   path, lineno, type);
> +				   path, lineno, found.type);
>  			errno = EINVAL;
>  			return -1;
>  		}
> @@ -464,7 +482,7 @@ static inline int process_line(struct selabel_handle *rec,
>  	 * any meta characters in the RE */
>  	spec_hasMetaChars(&spec_arr[nspec]);
> 
> -	if (strcmp(context, "<<none>>") && rec->validating)
> +	if (strcmp(found.context, "<<none>>") && rec->validating)
>  		compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
> 
>  	return 0;
> diff --git a/libselinux/src/label_internal.h b/libselinux/src/label_internal.h index
> aa48fff..e40d68b 100644
> --- a/libselinux/src/label_internal.h
> +++ b/libselinux/src/label_internal.h
> @@ -136,10 +136,11 @@ compat_validate(struct selabel_handle *rec,
>  		struct selabel_lookup_rec *contexts,
>  		const char *path, unsigned lineno) hidden;
> 
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>  /*
>   * The read_spec_entries function may be used to
>   * replace sscanf to read entries from spec files.
>   */
> -extern int read_spec_entries(char *line_buf, const char **errbuf, int num_args,
> ...);
> +extern int hidden read_spec_entries(char *line_buf, const char
> +**errbuf, unsigned int num, char *found[]);
> 
>  #endif				/* _SELABEL_INTERNAL_H_ */
> diff --git a/libselinux/src/label_support.c b/libselinux/src/label_support.c index
> 26f9ef1..3f5728d 100644
> --- a/libselinux/src/label_support.c
> +++ b/libselinux/src/label_support.c
> @@ -17,39 +17,40 @@
>   * Read an entry from a spec file (e.g. file_contexts)
>   * entry - Buffer to allocate for the entry.
>   * ptr - current location of the line to be processed.
> - * returns  - 0 on success and *entry is set to be a null
> - *            terminated value. On Error it returns -1 and
> - *            errno will be set.
> + * returns  - a pointer to the begining of the string.
> + *            that is the
>   *
>   */
> -static inline int read_spec_entry(char **entry, char **ptr, int *len, const char
> **errbuf)
> +static inline char *read_spec_entry(char **current, const char
> +**errbuf)
>  {
> -	*entry = NULL;
> -	char *tmp_buf = NULL;
> -
> -	while (isspace(**ptr) && **ptr != '\0')
> -		(*ptr)++;
> +	if(!**current) {
> +		(*current)++;
> +		return NULL;
> +	}
> 
> -	tmp_buf = *ptr;
> -	*len = 0;
> +	while (isspace(**current) && **current != '\0')
> +		(*current)++;
> 
> -	while (!isspace(**ptr) && **ptr != '\0') {
> -		if (!isascii(**ptr)) {
> +	char *start = *current;
> +	while (!isspace(**current) && **current != '\0') {
> +		if (!isascii(**current)) {
>  			errno = EINVAL;
>  			*errbuf = "Non-ASCII characters found";
> -			return -1;
> +			return NULL;
>  		}
> -		(*ptr)++;
> -		(*len)++;
> +		(*current)++;
>  	}
> 
> -	if (*len) {
> -		*entry = strndup(tmp_buf, *len);
> -		if (!*entry)
> -			return -1;
> -	}
> +	char *end = *current;
> 
> -	return 0;
> +	if (start == end)
> +		return NULL;
> +
> +	if (*end != '\0') {
> +		*end = '\0';
> +		(*current)++;
> +	}
> +	return start;
>  }
> 
>  /*
> @@ -63,15 +64,13 @@ static inline int read_spec_entry(char **entry, char **ptr,
> int *len, const char
>   * This function calls read_spec_entry() to do the actual string processing.
>   * As such, can return anything from that function as well.
>   */
> -int hidden read_spec_entries(char *line_buf, const char **errbuf, int num_args,
> ...)
> +int hidden read_spec_entries(char *line_buf, const char **errbuf,
> +unsigned int num, char *found[])
>  {
> -	char **spec_entry, *buf_p;
> -	int len, rc, items, entry_len = 0;
> -	va_list ap;
> -
> +	char*buf_p;
> +	unsigned int items = 0;
>  	*errbuf = NULL;
> 
> -	len = strlen(line_buf);
> +	size_t len = strlen(line_buf);
>  	if (line_buf[len - 1] == '\n')
>  		line_buf[len - 1] = '\0';
>  	else
> @@ -89,26 +88,12 @@ int hidden read_spec_entries(char *line_buf, const char
> **errbuf, int num_args,
>  		return 0;
> 
>  	/* Process the spec file entries */
> -	va_start(ap, num_args);
> -
> -	items = 0;
> -	while (items < num_args) {
> -		spec_entry = va_arg(ap, char **);
> -
> -		if (len - 1 == buf_p - line_buf) {
> -			va_end(ap);
> -			return items;
> -		}
> -
> -		rc = read_spec_entry(spec_entry, &buf_p, &entry_len, errbuf);
> -		if (rc < 0) {
> -			va_end(ap);
> -			return rc;
> -		}
> -		if (entry_len)
> -			items++;
> +	while (items < num) {
> +		found[items] = read_spec_entry(&buf_p, errbuf);
> +		if (!found[items])
> +			break;
> +		items++;
>  	}
> -	va_end(ap);
>  	return items;
>  }
> 
> --
> 1.9.1

Jason Zaman Sept. 20, 2016, 4:14 a.m. UTC | #2

On 20 Sep 2016 5:47 am, <[e-mail]> wrote:
>
> From: William Roberts <[e-mail]>
>
> THIS IS WIP...
>
> Rather than using stdio and making copies, just mmap the files
> and use the pointers in place. The affect of this change, is that
> text file load time is now faster than binary load time by 4.7%
> when testing with a file_contexts file from the Android tree. Note
> that the Android doesn't use monstrous regexs.
>
> Times are the average of 3 runs.
>
> BEFORE:
> Text file allocs: 114803
> Text file load time: 0.266101
> Bin file allocs: 93073
> Bin file load time: 0.248757667
>
> AFTER:
> Text file allocs: 103933
> Text file load time: 0.236192667
> Bin file allocs: 87645
> Bin file load time: .247607333

Do you have the scripts that generated these stats so I can play with it
too? These stats are from android right? Do you also have a comparison for
refpolicy too?

I haven't looked that closely yet but just realised, will this need new
perms because of the mmap? If it does, can you send a patch to refpolicy?

-- Jason

> THINGS TO DO:
> 1. What's arm performance like?
> 2. What interfaces to backends are busted by this (if any)?
> 3. Test Android Properties
> 4. Im pretty sure this breaks sefcontext_compile, fix.
> 5. Test with PCRE2 enabled.
> 6. Spell check this message!
> 7. Run checkpatch
>
> Signed-off-by: William Roberts <[e-mail]>
> ---
>  libselinux/src/label.c                  |   2 -
>  libselinux/src/label_android_property.c |  22 ++---
>  libselinux/src/label_file.c             | 140
+++++++++++++++++++-------------
>  libselinux/src/label_file.h             |  66 +++++++++------
>  libselinux/src/label_internal.h         |   3 +-
>  libselinux/src/label_support.c          |  79 ++++++++----------
>  6 files changed, 172 insertions(+), 140 deletions(-)
>
> diff --git a/libselinux/src/label.c b/libselinux/src/label.c
> index 963bfcb..d767b49 100644
> --- a/libselinux/src/label.c
> +++ b/libselinux/src/label.c
> @@ -15,8 +15,6 @@
>  #include "callbacks.h"
>  #include "label_internal.h"
>
> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> -
>  typedef int (*selabel_initfunc)(struct selabel_handle *rec,
>                                 const struct selinux_opt *opts,
>                                 unsigned nopts);
> diff --git a/libselinux/src/label_android_property.c
b/libselinux/src/label_android_property.c
> index 290b438..2aac394 100644
> --- a/libselinux/src/label_android_property.c
> +++ b/libselinux/src/label_android_property.c
> @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec,
>                         int pass, unsigned lineno)
>  {
>         int items;
> -       char *prop = NULL, *context = NULL;
> +       union {
> +               struct {
> +                       char *prop;
> +                       char *context;
> +               };
> +               char *array[2];
> +       } found = { .array = { 0 } };
>         struct saved_data *data = (struct saved_data *)rec->data;
>         spec_t *spec_arr = data->spec_arr;
>         unsigned int nspec = data->nspec;
>         const char *errbuf = NULL;
>
> -       items = read_spec_entries(line_buf, &errbuf, 2, &prop, &context);
> +       items = read_spec_entries(line_buf, &errbuf,
ARRAY_SIZE(found.array), found.array);
>         if (items < 0) {
>                 items = errno;
>                 selinux_log(SELINUX_ERROR,
> @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle *rec,
>                 selinux_log(SELINUX_ERROR,
>                             "%s:  line %u is missing fields\n", path,
>                             lineno);
> -               free(prop);
>                 errno = EINVAL;
>                 return -1;
>         }
>
> -       if (pass == 0) {
> -               free(prop);
> -               free(context);
> -       } else if (pass == 1) {
> +       if (pass == 1) {
>                 /* On the second pass, process and store the
specification in spec. */
> -               spec_arr[nspec].property_key = prop;
> -               spec_arr[nspec].lr.ctx_raw = context;
> +               spec_arr[nspec].property_key = found.prop;
> +               spec_arr[nspec].lr.ctx_raw = found.context;
>
>                 if (rec->validating) {
>                         if (selabel_validate(rec, &spec_arr[nspec].lr) <
0) {
> @@ -234,7 +236,7 @@ static void closef(struct selabel_handle *rec)
>         for (i = 0; i < data->nspec; i++) {
>                 spec = &data->spec_arr[i];
>                 free(spec->property_key);
> -               free(spec->lr.ctx_raw);
> +               //free(spec->lr.ctx_raw);
>                 free(spec->lr.ctx_trans);
>         }
>
> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> index 7156825..4dc440e 100644
> --- a/libselinux/src/label_file.c
> +++ b/libselinux/src/label_file.c
> @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data,
const char *path)
>         return rc;
>  }
>
> -static int process_text_file(FILE *fp, const char *prefix,
> -                            struct selabel_handle *rec, const char *path)
> +static inline struct saved_data *rec_to_data(struct selabel_handle *rec)
> +{
> +       return (struct saved_data *)rec->data;
> +}
> +
> +static char *mmap_area_get_line(struct mmap_area *area)
> +{
> +       size_t len = area->next_len;
> +       if (!len)
> +               return NULL;
> +
> +       char *start = area->next_addr;
> +       char *end = memchr(start, '\n', len);
> +
> +       /* the file may not end with a newline */
> +       if (!end)
> +               end = (char *)area->next_addr + len - 1;
> +
> +       *end = '\0';
> +       /* len includes null byte */
> +       len = end - start;
> +
> +       area->next_len -= len + 1;
> +       area->next_addr = ++end;
> +       return start;
> +}
> +
> +static int process_text_file(const char *path, const char *prefix,
> +               struct selabel_handle *rec)
>  {
>         int rc;
> -       size_t line_len;
>         unsigned int lineno = 0;
> -       char *line_buf = NULL;
> +       char *line_buf;
> +       struct mmap_area *areas = rec_to_data(rec)->mmap_areas;
> +
> +       /* mmap_area_get_line() and process_line() require mutable string
pointers */
> +       rc = mprotect(areas->addr, areas->len, PROT_READ | PROT_WRITE);
> +       if (rc < 0)
> +               goto out;
>
> -       while (getline(&line_buf, &line_len, fp) > 0) {
> +       while ( (line_buf = mmap_area_get_line(areas)) ) {
>                 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)
> +static int mmap_file(FILE *fp, size_t len, struct saved_data *data)
>  {
> -       struct saved_data *data = (struct saved_data *)rec->data;
> -       int rc;
> -       char *addr, *str_buf;
> -       int *stem_map;
> -       struct mmap_area *mmap_area;
> -       uint32_t i, magic, version;
> -       uint32_t entry_len, stem_map_len, regex_array_len;
> -       const char *reg_version;
> -
> -       mmap_area = malloc(sizeof(*mmap_area));
> +       struct mmap_area *mmap_area = malloc(sizeof(*mmap_area));
>         if (!mmap_area) {
>                 return -1;
>         }
>
> -       addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
> +       void *addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp),
0);
>         if (addr == MAP_FAILED) {
>                 free(mmap_area);
>                 perror("mmap");
> @@ -145,17 +166,26 @@ static int load_mmap(FILE *fp, size_t len, struct
selabel_handle *rec,
>         mmap_area->next = data->mmap_areas;
>         data->mmap_areas = mmap_area;
>
> -       /* 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)
> -               return -1;
> +       return 0;
> +}
> +
> +static int process_binary_file(const char *path, struct selabel_handle
*rec)
> +{
> +       struct saved_data *data = (struct saved_data *)rec->data;
> +       int rc;
> +       char *str_buf;
> +       int *stem_map;
> +       struct mmap_area *mmap_area = data->mmap_areas;
> +       uint32_t i, version;
> +       uint32_t entry_len, stem_map_len, regex_array_len;
> +       size_t len;
>
>         /* check if this version is higher than we understand */
>         rc = next_entry(&version, mmap_area, sizeof(uint32_t));
>         if (rc < 0 || version > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
>                 return -1;
>
> -       reg_version = regex_version();
> +       const char *reg_version = regex_version();
>         if (!reg_version)
>                 return -1;
>
> @@ -235,12 +265,11 @@ static int load_mmap(FILE *fp, size_t len, struct
selabel_handle *rec,
>                 /* store the mapping between old and new */
>                 newid = find_stem(data, buf, stem_len);
>                 if (newid < 0) {
> -                       newid = store_stem(data, buf, stem_len);
> +                       newid = store_stem(data, buf, stem_len, true);
>                         if (newid < 0) {
>                                 rc = newid;
>                                 goto out;
>                         }
> -                       data->stem_arr[newid].from_mmap = 1;
>                 }
>                 stem_map[i] = newid;
>         }
> @@ -262,7 +291,6 @@ static int load_mmap(FILE *fp, size_t len, struct
selabel_handle *rec,
>                         goto out;
>
>                 spec = &data->spec_arr[data->nspec];
> -               spec->from_mmap = 1;
>
>                 /* Process context */
>                 rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
> @@ -271,22 +299,10 @@ static int load_mmap(FILE *fp, size_t len, struct
selabel_handle *rec,
>                         goto out;
>                 }
>
> -               str_buf = malloc(entry_len);
> -               if (!str_buf) {
> -                       rc = -1;
> -                       goto out;
> -               }
> -               rc = next_entry(str_buf, mmap_area, entry_len);
> +               rc = next_pstr(&spec->lr.ctx_raw, mmap_area, entry_len);
>                 if (rc < 0)
>                         goto out;
>
> -               if (str_buf[entry_len - 1] != '\0') {
> -                       free(str_buf);
> -                       rc = -1;
> -                       goto out;
> -               }
> -               spec->lr.ctx_raw = str_buf;
> -
>                 if (strcmp(spec->lr.ctx_raw, "<<none>>") &&
rec->validating) {
>                         if (selabel_validate(rec, &spec->lr) < 0) {
>                                 selinux_log(SELINUX_ERROR,
> @@ -401,17 +417,29 @@ static char *rolling_append(char *current, const
char *suffix, size_t max)
>         return current;
>  }
>
> -static bool fcontext_is_binary(FILE *fp)
> +static inline int mmap_area_rewind(struct mmap_area *area, size_t bytes)
> +{
> +       if (area->next_len + bytes > area->len)
> +               return -1;
> +
> +       area->next_addr = (char *) area->next_addr - bytes;
> +       area->next_len += bytes;
> +       return 0;
> +}
> +
> +static bool fcontext_is_binary(struct mmap_area *area)
>  {
>         uint32_t magic;
> +       bool is_binary = false;
>
> -       size_t len = fread(&magic, sizeof(magic), 1, fp);
> -       rewind(fp);
> +       int rc = next_entry(&magic, area, sizeof(magic));
>
> -       return (len && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
> -}
> +       is_binary = (!rc && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
> +       if (!is_binary)
> +               mmap_area_rewind(area, sizeof(magic));
>
> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +       return is_binary;
> +}
>
>  static FILE *open_file(const char *path, const char *suffix,
>                char *save_path, size_t len, struct stat *sb, bool
open_oldest)
> @@ -504,12 +532,17 @@ static int process_file(const char *path, const
char *suffix,
>                 if (fp == NULL)
>                         return -1;
>
> -               rc = fcontext_is_binary(fp) ?
> -                               load_mmap(fp, sb.st_size, rec,
found_path) :
> -                               process_text_file(fp, prefix, rec,
found_path);
> +               rc = mmap_file(fp, sb.st_size, rec_to_data(rec));
> +               if (rc < 0) {
> +                       fclose(fp);
> +                       return -1;
> +               }
> +
> +               rc = fcontext_is_binary(rec_to_data(rec)->mmap_areas) ?
> +                               process_binary_file(found_path, rec) :
> +                               process_text_file(found_path, prefix,
rec);
>                 if (!rc)
> -                       rc = digest_add_specfile(digest, fp, NULL,
sb.st_size,
> -                               found_path);
> +                       rc = digest_add_specfile(digest, fp, NULL,
sb.st_size, found_path);
>
>                 fclose(fp);
>
> @@ -613,12 +646,7 @@ static void closef(struct selabel_handle *rec)
>         for (i = 0; i < data->nspec; i++) {
>                 spec = &data->spec_arr[i];
>                 free(spec->lr.ctx_trans);
> -               free(spec->lr.ctx_raw);
>                 regex_data_free(spec->regex);
> -               if (spec->from_mmap)
> -                       continue;
> -               free(spec->regex_str);
> -               free(spec->type_str);
>         }
>
>         for (i = 0; i < (unsigned int)data->num_stems; i++) {
> diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
> index 88f4294..2704906 100644
> --- a/libselinux/src/label_file.h
> +++ b/libselinux/src/label_file.h
> @@ -37,7 +37,6 @@ struct spec {
>         int matches;            /* number of matching pathnames */
>         int stem_id;            /* indicates which stem-compression item
*/
>         char hasMetaChars;      /* regular expression has meta-chars */
> -       char from_mmap;         /* this spec is from an mmap of the data
*/
>         size_t prefix_len;      /* length of fixed path prefix */
>  };
>
> @@ -255,7 +254,7 @@ static inline int find_stem(struct saved_data *data,
const char *buf,
>  }
>
>  /* returns the index of the new stored object */
> -static inline int store_stem(struct saved_data *data, char *buf, int
stem_len)
> +static inline int store_stem(struct saved_data *data, char *buf, int
stem_len, bool from_mmap)
>  {
>         int num = data->num_stems;
>
> @@ -271,7 +270,7 @@ static inline int store_stem(struct saved_data *data,
char *buf, int stem_len)
>         }
>         data->stem_arr[num].len = stem_len;
>         data->stem_arr[num].buf = buf;
> -       data->stem_arr[num].from_mmap = 0;
> +       data->stem_arr[num].from_mmap = from_mmap;
>         data->num_stems++;
>
>         return num;
> @@ -298,7 +297,7 @@ static inline int find_stem_from_spec(struct
saved_data *data, const char *buf)
>         if (!stem)
>                 return -1;
>
> -       return store_stem(data, stem, stem_len);
> +       return store_stem(data, stem, stem_len, false);
>  }
>
>  /* This will always check for buffer over-runs and either read the next
entry
> @@ -317,6 +316,22 @@ static inline int next_entry(void *buf, struct
mmap_area *fp, size_t bytes)
>         return 0;
>  }
>
> +static inline int next_pstr(char **str, struct mmap_area *fp, size_t len)
> +{
> +       if (len > fp->next_len)
> +               return -1;
> +
> +       char *tmp = (char *)fp->next_addr;
> +       if (tmp[len-1] != '\0')
> +               return -1;
> +
> +       *str = tmp;
> +
> +       fp->next_addr = (char *)fp->next_addr + len;
> +       fp->next_len -= len;
> +       return 0;
> +}
> +
>  static inline int compile_regex(struct saved_data *data, struct spec
*spec,
>                                             const char **errbuf)
>  {
> @@ -375,13 +390,21 @@ static inline int process_line(struct
selabel_handle *rec,
>                         char *line_buf, unsigned lineno)
>  {
>         int items, len, rc;
> -       char *regex = NULL, *type = NULL, *context = NULL;
> +       union {
> +               struct {
> +                       char *regex;
> +                       char *type;
> +                       char *context;
> +               };
> +               char *array[3];
> +       } found = { .array = { 0 } };
> +
>         struct saved_data *data = (struct saved_data *)rec->data;
>         struct spec *spec_arr;
>         unsigned int nspec = data->nspec;
>         const char *errbuf = NULL;
>
> -       items = read_spec_entries(line_buf, &errbuf, 3, &regex, &type,
&context);
> +       items = read_spec_entries(line_buf, &errbuf,
ARRAY_SIZE(found.array), found.array);
>         if (items < 0) {
>                 rc = errno;
>                 selinux_log(SELINUX_ERROR,
> @@ -398,22 +421,17 @@ static inline int process_line(struct
selabel_handle *rec,
>                 COMPAT_LOG(SELINUX_ERROR,
>                             "%s:  line %u is missing fields\n", path,
>                             lineno);
> -               if (items == 1)
> -                       free(regex);
>                 errno = EINVAL;
>                 return -1;
>         } else if (items == 2) {
>                 /* The type field is optional. */
> -               context = type;
> -               type = 0;
> +               found.context = found.type;
> +               found.type = NULL;
>         }
>
> -       len = get_stem_from_spec(regex);
> -       if (len && prefix && strncmp(prefix, regex, len)) {
> +       len = get_stem_from_spec(found.regex);
> +       if (len && prefix && strncmp(prefix, found.regex, len)) {
>                 /* Stem of regex does not match requested prefix,
discard. */
> -               free(regex);
> -               free(type);
> -               free(context);
>                 return 0;
>         }
>
> @@ -424,13 +442,13 @@ static inline int process_line(struct
selabel_handle *rec,
>         spec_arr = data->spec_arr;
>
>         /* process and store the specification in spec. */
> -       spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
> -       spec_arr[nspec].regex_str = regex;
> +       spec_arr[nspec].stem_id = find_stem_from_spec(data, found.regex);
> +       spec_arr[nspec].regex_str = found.regex;
>
> -       spec_arr[nspec].type_str = type;
> +       spec_arr[nspec].type_str = found.type;
>         spec_arr[nspec].mode = 0;
>
> -       spec_arr[nspec].lr.ctx_raw = context;
> +       spec_arr[nspec].lr.ctx_raw = found.context;
>
>         /*
>          * bump data->nspecs to cause closef() to cover it in its free
> @@ -442,18 +460,18 @@ static inline int process_line(struct
selabel_handle *rec,
>                         && compile_regex(data, &spec_arr[nspec],
&errbuf)) {
>                 COMPAT_LOG(SELINUX_ERROR,
>                            "%s:  line %u has invalid regex %s:  %s\n",
> -                          path, lineno, regex, errbuf);
> +                          path, lineno, found.regex, errbuf);
>                 errno = EINVAL;
>                 return -1;
>         }
>
> -       if (type) {
> -               mode_t mode = string_to_mode(type);
> +       if (found.type) {
> +               mode_t mode = string_to_mode(found.type);
>
>                 if (mode == (mode_t)-1) {
>                         COMPAT_LOG(SELINUX_ERROR,
>                                    "%s:  line %u has invalid file type
%s\n",
> -                                  path, lineno, type);
> +                                  path, lineno, found.type);
>                         errno = EINVAL;
>                         return -1;
>                 }
> @@ -464,7 +482,7 @@ static inline int process_line(struct selabel_handle
*rec,
>          * any meta characters in the RE */
>         spec_hasMetaChars(&spec_arr[nspec]);
>
> -       if (strcmp(context, "<<none>>") && rec->validating)
> +       if (strcmp(found.context, "<<none>>") && rec->validating)
>                 compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
>
>         return 0;
> diff --git a/libselinux/src/label_internal.h
b/libselinux/src/label_internal.h
> index aa48fff..e40d68b 100644
> --- a/libselinux/src/label_internal.h
> +++ b/libselinux/src/label_internal.h
> @@ -136,10 +136,11 @@ compat_validate(struct selabel_handle *rec,
>                 struct selabel_lookup_rec *contexts,
>                 const char *path, unsigned lineno) hidden;
>
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>  /*
>   * The read_spec_entries function may be used to
>   * replace sscanf to read entries from spec files.
>   */
> -extern int read_spec_entries(char *line_buf, const char **errbuf, int
num_args, ...);
> +extern int hidden read_spec_entries(char *line_buf, const char **errbuf,
unsigned int num, char *found[]);
>
>  #endif                         /* _SELABEL_INTERNAL_H_ */
> diff --git a/libselinux/src/label_support.c
b/libselinux/src/label_support.c
> index 26f9ef1..3f5728d 100644
> --- a/libselinux/src/label_support.c
> +++ b/libselinux/src/label_support.c
> @@ -17,39 +17,40 @@
>   * Read an entry from a spec file (e.g. file_contexts)
>   * entry - Buffer to allocate for the entry.
>   * ptr - current location of the line to be processed.
> - * returns  - 0 on success and *entry is set to be a null
> - *            terminated value. On Error it returns -1 and
> - *            errno will be set.
> + * returns  - a pointer to the begining of the string.
> + *            that is the
>   *
>   */
> -static inline int read_spec_entry(char **entry, char **ptr, int *len,
const char **errbuf)
> +static inline char *read_spec_entry(char **current, const char **errbuf)
>  {
> -       *entry = NULL;
> -       char *tmp_buf = NULL;
> -
> -       while (isspace(**ptr) && **ptr != '\0')
> -               (*ptr)++;
> +       if(!**current) {
> +               (*current)++;
> +               return NULL;
> +       }
>
> -       tmp_buf = *ptr;
> -       *len = 0;
> +       while (isspace(**current) && **current != '\0')
> +               (*current)++;
>
> -       while (!isspace(**ptr) && **ptr != '\0') {
> -               if (!isascii(**ptr)) {
> +       char *start = *current;
> +       while (!isspace(**current) && **current != '\0') {
> +               if (!isascii(**current)) {
>                         errno = EINVAL;
>                         *errbuf = "Non-ASCII characters found";
> -                       return -1;
> +                       return NULL;
>                 }
> -               (*ptr)++;
> -               (*len)++;
> +               (*current)++;
>         }
>
> -       if (*len) {
> -               *entry = strndup(tmp_buf, *len);
> -               if (!*entry)
> -                       return -1;
> -       }
> +       char *end = *current;
>
> -       return 0;
> +       if (start == end)
> +               return NULL;
> +
> +       if (*end != '\0') {
> +               *end = '\0';
> +               (*current)++;
> +       }
> +       return start;
>  }
>
>  /*
> @@ -63,15 +64,13 @@ static inline int read_spec_entry(char **entry, char
**ptr, int *len, const char
>   * This function calls read_spec_entry() to do the actual string
processing.
>   * As such, can return anything from that function as well.
>   */
> -int hidden read_spec_entries(char *line_buf, const char **errbuf, int
num_args, ...)
> +int hidden read_spec_entries(char *line_buf, const char **errbuf,
unsigned int num, char *found[])
>  {
> -       char **spec_entry, *buf_p;
> -       int len, rc, items, entry_len = 0;
> -       va_list ap;
> -
> +       char*buf_p;
> +       unsigned int items = 0;
>         *errbuf = NULL;
>
> -       len = strlen(line_buf);
> +       size_t len = strlen(line_buf);
>         if (line_buf[len - 1] == '\n')
>                 line_buf[len - 1] = '\0';
>         else
> @@ -89,26 +88,12 @@ int hidden read_spec_entries(char *line_buf, const
char **errbuf, int num_args,
>                 return 0;
>
>         /* Process the spec file entries */
> -       va_start(ap, num_args);
> -
> -       items = 0;
> -       while (items < num_args) {
> -               spec_entry = va_arg(ap, char **);
> -
> -               if (len - 1 == buf_p - line_buf) {
> -                       va_end(ap);
> -                       return items;
> -               }
> -
> -               rc = read_spec_entry(spec_entry, &buf_p, &entry_len,
errbuf);
> -               if (rc < 0) {
> -                       va_end(ap);
> -                       return rc;
> -               }
> -               if (entry_len)
> -                       items++;
> +       while (items < num) {
> +               found[items] = read_spec_entry(&buf_p, errbuf);
> +               if (!found[items])
> +                       break;
> +               items++;
>         }
> -       va_end(ap);
>         return items;
>  }
>
> --
> 1.9.1
>
> _______________________________________________
> Selinux mailing list
> [e-mail]
> To unsubscribe, send email to [e-mail].
> To get help, send an email containing "help" to
[e-mail].

William Roberts Sept. 20, 2016, 4:50 a.m. UTC | #3

On Sep 19, 2016 21:16, "Jason Zaman" <[e-mail]> wrote:
>
> On 20 Sep 2016 5:47 am, <[e-mail]> wrote:
> >
> > From: William Roberts <[e-mail]>
> >
> > THIS IS WIP...
> >
> > Rather than using stdio and making copies, just mmap the files
> > and use the pointers in place. The affect of this change, is that
> > text file load time is now faster than binary load time by 4.7%
> > when testing with a file_contexts file from the Android tree. Note
> > that the Android doesn't use monstrous regexs.
> >
> > Times are the average of 3 runs.
> >
> > BEFORE:
> > Text file allocs: 114803
> > Text file load time: 0.266101
> > Bin file allocs: 93073
> > Bin file load time: 0.248757667
> >
> > AFTER:
> > Text file allocs: 103933
> > Text file load time: 0.236192667
> > Bin file allocs: 87645
> > Bin file load time: .247607333
>
> Do you have the scripts that generated these stats so I can play with it
too? These stats are from android right? Do you also have a comparison for
refpolicy too?

For generating these I used checkfc.c from the Android tree. I used
valgrind to measure allocations and clock to measure the time in
selabel_open().

>
> I haven't looked that closely yet but just realised, will this need new
perms because of the mmap? If it does, can you send a patch to refpolicy?

I'm confused, mmap is not a permission, even if it was the binary path
already was doing an mmap, so the permission would have been there. We're
just making it so it always mmaps.

>
> -- Jason
>
> > THINGS TO DO:
> > 1. What's arm performance like?
> > 2. What interfaces to backends are busted by this (if any)?
> > 3. Test Android Properties
> > 4. Im pretty sure this breaks sefcontext_compile, fix.
> > 5. Test with PCRE2 enabled.
> > 6. Spell check this message!
> > 7. Run checkpatch
> >
> > Signed-off-by: William Roberts <[e-mail]>
> > ---
> >  libselinux/src/label.c                  |   2 -
> >  libselinux/src/label_android_property.c |  22 ++---
> >  libselinux/src/label_file.c             | 140
+++++++++++++++++++-------------
> >  libselinux/src/label_file.h             |  66 +++++++++------
> >  libselinux/src/label_internal.h         |   3 +-
> >  libselinux/src/label_support.c          |  79 ++++++++----------
> >  6 files changed, 172 insertions(+), 140 deletions(-)
> >
> > diff --git a/libselinux/src/label.c b/libselinux/src/label.c
> > index 963bfcb..d767b49 100644
> > --- a/libselinux/src/label.c
> > +++ b/libselinux/src/label.c
> > @@ -15,8 +15,6 @@
> >  #include "callbacks.h"
> >  #include "label_internal.h"
> >
> > -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > -
> >  typedef int (*selabel_initfunc)(struct selabel_handle *rec,
> >                                 const struct selinux_opt *opts,
> >                                 unsigned nopts);
> > diff --git a/libselinux/src/label_android_property.c
b/libselinux/src/label_android_property.c
> > index 290b438..2aac394 100644
> > --- a/libselinux/src/label_android_property.c
> > +++ b/libselinux/src/label_android_property.c
> > @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec,
> >                         int pass, unsigned lineno)
> >  {
> >         int items;
> > -       char *prop = NULL, *context = NULL;
> > +       union {
> > +               struct {
> > +                       char *prop;
> > +                       char *context;
> > +               };
> > +               char *array[2];
> > +       } found = { .array = { 0 } };
> >         struct saved_data *data = (struct saved_data *)rec->data;
> >         spec_t *spec_arr = data->spec_arr;
> >         unsigned int nspec = data->nspec;
> >         const char *errbuf = NULL;
> >
> > -       items = read_spec_entries(line_buf, &errbuf, 2, &prop,
&context);
> > +       items = read_spec_entries(line_buf, &errbuf,
ARRAY_SIZE(found.array), found.array);
> >         if (items < 0) {
> >                 items = errno;
> >                 selinux_log(SELINUX_ERROR,
> > @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle
*rec,
> >                 selinux_log(SELINUX_ERROR,
> >                             "%s:  line %u is missing fields\n", path,
> >                             lineno);
> > -               free(prop);
> >                 errno = EINVAL;
> >                 return -1;
> >         }
> >
> > -       if (pass == 0) {
> > -               free(prop);
> > -               free(context);
> > -       } else if (pass == 1) {
> > +       if (pass == 1) {
> >                 /* On the second pass, process and store the
specification in spec. */
> > -               spec_arr[nspec].property_key = prop;
> > -               spec_arr[nspec].lr.ctx_raw = context;
> > +               spec_arr[nspec].property_key = found.prop;
> > +               spec_arr[nspec].lr.ctx_raw = found.context;
> >
> >                 if (rec->validating) {
> >                         if (selabel_validate(rec, &spec_arr[nspec].lr)
< 0) {
> > @@ -234,7 +236,7 @@ static void closef(struct selabel_handle *rec)
> >         for (i = 0; i < data->nspec; i++) {
> >                 spec = &data->spec_arr[i];
> >                 free(spec->property_key);
> > -               free(spec->lr.ctx_raw);
> > +               //free(spec->lr.ctx_raw);
> >                 free(spec->lr.ctx_trans);
> >         }
> >
> > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > index 7156825..4dc440e 100644
> > --- a/libselinux/src/label_file.c
> > +++ b/libselinux/src/label_file.c
> > @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data,
const char *path)
> >         return rc;
> >  }
> >
> > -static int process_text_file(FILE *fp, const char *prefix,
> > -                            struct selabel_handle *rec, const char
*path)
> > +static inline struct saved_data *rec_to_data(struct selabel_handle
*rec)
> > +{
> > +       return (struct saved_data *)rec->data;
> > +}
> > +
> > +static char *mmap_area_get_line(struct mmap_area *area)
> > +{
> > +       size_t len = area->next_len;
> > +       if (!len)
> > +               return NULL;
> > +
> > +       char *start = area->next_addr;
> > +       char *end = memchr(start, '\n', len);
> > +
> > +       /* the file may not end with a newline */
> > +       if (!end)
> > +               end = (char *)area->next_addr + len - 1;
> > +
> > +       *end = '\0';
> > +       /* len includes null byte */
> > +       len = end - start;
> > +
> > +       area->next_len -= len + 1;
> > +       area->next_addr = ++end;
> > +       return start;
> > +}
> > +
> > +static int process_text_file(const char *path, const char *prefix,
> > +               struct selabel_handle *rec)
> >  {
> >         int rc;
> > -       size_t line_len;
> >         unsigned int lineno = 0;
> > -       char *line_buf = NULL;
> > +       char *line_buf;
> > +       struct mmap_area *areas = rec_to_data(rec)->mmap_areas;
> > +
> > +       /* mmap_area_get_line() and process_line() require mutable
string pointers */
> > +       rc = mprotect(areas->addr, areas->len, PROT_READ | PROT_WRITE);
> > +       if (rc < 0)
> > +               goto out;
> >
> > -       while (getline(&line_buf, &line_len, fp) > 0) {
> > +       while ( (line_buf = mmap_area_get_line(areas)) ) {
> >                 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)
> > +static int mmap_file(FILE *fp, size_t len, struct saved_data *data)
> >  {
> > -       struct saved_data *data = (struct saved_data *)rec->data;
> > -       int rc;
> > -       char *addr, *str_buf;
> > -       int *stem_map;
> > -       struct mmap_area *mmap_area;
> > -       uint32_t i, magic, version;
> > -       uint32_t entry_len, stem_map_len, regex_array_len;
> > -       const char *reg_version;
> > -
> > -       mmap_area = malloc(sizeof(*mmap_area));
> > +       struct mmap_area *mmap_area = malloc(sizeof(*mmap_area));
> >         if (!mmap_area) {
> >                 return -1;
> >         }
> >
> > -       addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
> > +       void *addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE,
fileno(fp), 0);
> >         if (addr == MAP_FAILED) {
> >                 free(mmap_area);
> >                 perror("mmap");
> > @@ -145,17 +166,26 @@ static int load_mmap(FILE *fp, size_t len, struct
selabel_handle *rec,
> >         mmap_area->next = data->mmap_areas;
> >         data->mmap_areas = mmap_area;
> >
> > -       /* 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)
> > -               return -1;
> > +       return 0;
> > +}
> > +
> > +static int process_binary_file(const char *path, struct selabel_handle
*rec)
> > +{
> > +       struct saved_data *data = (struct saved_data *)rec->data;
> > +       int rc;
> > +       char *str_buf;
> > +       int *stem_map;
> > +       struct mmap_area *mmap_area = data->mmap_areas;
> > +       uint32_t i, version;
> > +       uint32_t entry_len, stem_map_len, regex_array_len;
> > +       size_t len;
> >
> >         /* check if this version is higher than we understand */
> >         rc = next_entry(&version, mmap_area, sizeof(uint32_t));
> >         if (rc < 0 || version > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
> >                 return -1;
> >
> > -       reg_version = regex_version();
> > +       const char *reg_version = regex_version();
> >         if (!reg_version)
> >                 return -1;
> >
> > @@ -235,12 +265,11 @@ static int load_mmap(FILE *fp, size_t len, struct
selabel_handle *rec,
> >                 /* store the mapping between old and new */
> >                 newid = find_stem(data, buf, stem_len);
> >                 if (newid < 0) {
> > -                       newid = store_stem(data, buf, stem_len);
> > +                       newid = store_stem(data, buf, stem_len, true);
> >                         if (newid < 0) {
> >                                 rc = newid;
> >                                 goto out;
> >                         }
> > -                       data->stem_arr[newid].from_mmap = 1;
> >                 }
> >                 stem_map[i] = newid;
> >         }
> > @@ -262,7 +291,6 @@ static int load_mmap(FILE *fp, size_t len, struct
selabel_handle *rec,
> >                         goto out;
> >
> >                 spec = &data->spec_arr[data->nspec];
> > -               spec->from_mmap = 1;
> >
> >                 /* Process context */
> >                 rc = next_entry(&entry_len, mmap_area,
sizeof(uint32_t));
> > @@ -271,22 +299,10 @@ static int load_mmap(FILE *fp, size_t len, struct
selabel_handle *rec,
> >                         goto out;
> >                 }
> >
> > -               str_buf = malloc(entry_len);
> > -               if (!str_buf) {
> > -                       rc = -1;
> > -                       goto out;
> > -               }
> > -               rc = next_entry(str_buf, mmap_area, entry_len);
> > +               rc = next_pstr(&spec->lr.ctx_raw, mmap_area, entry_len);
> >                 if (rc < 0)
> >                         goto out;
> >
> > -               if (str_buf[entry_len - 1] != '\0') {
> > -                       free(str_buf);
> > -                       rc = -1;
> > -                       goto out;
> > -               }
> > -               spec->lr.ctx_raw = str_buf;
> > -
> >                 if (strcmp(spec->lr.ctx_raw, "<<none>>") &&
rec->validating) {
> >                         if (selabel_validate(rec, &spec->lr) < 0) {
> >                                 selinux_log(SELINUX_ERROR,
> > @@ -401,17 +417,29 @@ static char *rolling_append(char *current, const
char *suffix, size_t max)
> >         return current;
> >  }
> >
> > -static bool fcontext_is_binary(FILE *fp)
> > +static inline int mmap_area_rewind(struct mmap_area *area, size_t
bytes)
> > +{
> > +       if (area->next_len + bytes > area->len)
> > +               return -1;
> > +
> > +       area->next_addr = (char *) area->next_addr - bytes;
> > +       area->next_len += bytes;
> > +       return 0;
> > +}
> > +
> > +static bool fcontext_is_binary(struct mmap_area *area)
> >  {
> >         uint32_t magic;
> > +       bool is_binary = false;
> >
> > -       size_t len = fread(&magic, sizeof(magic), 1, fp);
> > -       rewind(fp);
> > +       int rc = next_entry(&magic, area, sizeof(magic));
> >
> > -       return (len && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
> > -}
> > +       is_binary = (!rc && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
> > +       if (!is_binary)
> > +               mmap_area_rewind(area, sizeof(magic));
> >
> > -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > +       return is_binary;
> > +}
> >
> >  static FILE *open_file(const char *path, const char *suffix,
> >                char *save_path, size_t len, struct stat *sb, bool
open_oldest)
> > @@ -504,12 +532,17 @@ static int process_file(const char *path, const
char *suffix,
> >                 if (fp == NULL)
> >                         return -1;
> >
> > -               rc = fcontext_is_binary(fp) ?
> > -                               load_mmap(fp, sb.st_size, rec,
found_path) :
> > -                               process_text_file(fp, prefix, rec,
found_path);
> > +               rc = mmap_file(fp, sb.st_size, rec_to_data(rec));
> > +               if (rc < 0) {
> > +                       fclose(fp);
> > +                       return -1;
> > +               }
> > +
> > +               rc = fcontext_is_binary(rec_to_data(rec)->mmap_areas) ?
> > +                               process_binary_file(found_path, rec) :
> > +                               process_text_file(found_path, prefix,
rec);
> >                 if (!rc)
> > -                       rc = digest_add_specfile(digest, fp, NULL,
sb.st_size,
> > -                               found_path);
> > +                       rc = digest_add_specfile(digest, fp, NULL,
sb.st_size, found_path);
> >
> >                 fclose(fp);
> >
> > @@ -613,12 +646,7 @@ static void closef(struct selabel_handle *rec)
> >         for (i = 0; i < data->nspec; i++) {
> >                 spec = &data->spec_arr[i];
> >                 free(spec->lr.ctx_trans);
> > -               free(spec->lr.ctx_raw);
> >                 regex_data_free(spec->regex);
> > -               if (spec->from_mmap)
> > -                       continue;
> > -               free(spec->regex_str);
> > -               free(spec->type_str);
> >         }
> >
> >         for (i = 0; i < (unsigned int)data->num_stems; i++) {
> > diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
> > index 88f4294..2704906 100644
> > --- a/libselinux/src/label_file.h
> > +++ b/libselinux/src/label_file.h
> > @@ -37,7 +37,6 @@ struct spec {
> >         int matches;            /* number of matching pathnames */
> >         int stem_id;            /* indicates which stem-compression
item */
> >         char hasMetaChars;      /* regular expression has meta-chars */
> > -       char from_mmap;         /* this spec is from an mmap of the
data */
> >         size_t prefix_len;      /* length of fixed path prefix */
> >  };
> >
> > @@ -255,7 +254,7 @@ static inline int find_stem(struct saved_data
*data, const char *buf,
> >  }
> >
> >  /* returns the index of the new stored object */
> > -static inline int store_stem(struct saved_data *data, char *buf, int
stem_len)
> > +static inline int store_stem(struct saved_data *data, char *buf, int
stem_len, bool from_mmap)
> >  {
> >         int num = data->num_stems;
> >
> > @@ -271,7 +270,7 @@ static inline int store_stem(struct saved_data
*data, char *buf, int stem_len)
> >         }
> >         data->stem_arr[num].len = stem_len;
> >         data->stem_arr[num].buf = buf;
> > -       data->stem_arr[num].from_mmap = 0;
> > +       data->stem_arr[num].from_mmap = from_mmap;
> >         data->num_stems++;
> >
> >         return num;
> > @@ -298,7 +297,7 @@ static inline int find_stem_from_spec(struct
saved_data *data, const char *buf)
> >         if (!stem)
> >                 return -1;
> >
> > -       return store_stem(data, stem, stem_len);
> > +       return store_stem(data, stem, stem_len, false);
> >  }
> >
> >  /* This will always check for buffer over-runs and either read the
next entry
> > @@ -317,6 +316,22 @@ static inline int next_entry(void *buf, struct
mmap_area *fp, size_t bytes)
> >         return 0;
> >  }
> >
> > +static inline int next_pstr(char **str, struct mmap_area *fp, size_t
len)
> > +{
> > +       if (len > fp->next_len)
> > +               return -1;
> > +
> > +       char *tmp = (char *)fp->next_addr;
> > +       if (tmp[len-1] != '\0')
> > +               return -1;
> > +
> > +       *str = tmp;
> > +
> > +       fp->next_addr = (char *)fp->next_addr + len;
> > +       fp->next_len -= len;
> > +       return 0;
> > +}
> > +
> >  static inline int compile_regex(struct saved_data *data, struct spec
*spec,
> >                                             const char **errbuf)
> >  {
> > @@ -375,13 +390,21 @@ static inline int process_line(struct
selabel_handle *rec,
> >                         char *line_buf, unsigned lineno)
> >  {
> >         int items, len, rc;
> > -       char *regex = NULL, *type = NULL, *context = NULL;
> > +       union {
> > +               struct {
> > +                       char *regex;
> > +                       char *type;
> > +                       char *context;
> > +               };
> > +               char *array[3];
> > +       } found = { .array = { 0 } };
> > +
> >         struct saved_data *data = (struct saved_data *)rec->data;
> >         struct spec *spec_arr;
> >         unsigned int nspec = data->nspec;
> >         const char *errbuf = NULL;
> >
> > -       items = read_spec_entries(line_buf, &errbuf, 3, &regex, &type,
&context);
> > +       items = read_spec_entries(line_buf, &errbuf,
ARRAY_SIZE(found.array), found.array);
> >         if (items < 0) {
> >                 rc = errno;
> >                 selinux_log(SELINUX_ERROR,
> > @@ -398,22 +421,17 @@ static inline int process_line(struct
selabel_handle *rec,
> >                 COMPAT_LOG(SELINUX_ERROR,
> >                             "%s:  line %u is missing fields\n", path,
> >                             lineno);
> > -               if (items == 1)
> > -                       free(regex);
> >                 errno = EINVAL;
> >                 return -1;
> >         } else if (items == 2) {
> >                 /* The type field is optional. */
> > -               context = type;
> > -               type = 0;
> > +               found.context = found.type;
> > +               found.type = NULL;
> >         }
> >
> > -       len = get_stem_from_spec(regex);
> > -       if (len && prefix && strncmp(prefix, regex, len)) {
> > +       len = get_stem_from_spec(found.regex);
> > +       if (len && prefix && strncmp(prefix, found.regex, len)) {
> >                 /* Stem of regex does not match requested prefix,
discard. */
> > -               free(regex);
> > -               free(type);
> > -               free(context);
> >                 return 0;
> >         }
> >
> > @@ -424,13 +442,13 @@ static inline int process_line(struct
selabel_handle *rec,
> >         spec_arr = data->spec_arr;
> >
> >         /* process and store the specification in spec. */
> > -       spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
> > -       spec_arr[nspec].regex_str = regex;
> > +       spec_arr[nspec].stem_id = find_stem_from_spec(data,
found.regex);
> > +       spec_arr[nspec].regex_str = found.regex;
> >
> > -       spec_arr[nspec].type_str = type;
> > +       spec_arr[nspec].type_str = found.type;
> >         spec_arr[nspec].mode = 0;
> >
> > -       spec_arr[nspec].lr.ctx_raw = context;
> > +       spec_arr[nspec].lr.ctx_raw = found.context;
> >
> >         /*
> >          * bump data->nspecs to cause closef() to cover it in its free
> > @@ -442,18 +460,18 @@ static inline int process_line(struct
selabel_handle *rec,
> >                         && compile_regex(data, &spec_arr[nspec],
&errbuf)) {
> >                 COMPAT_LOG(SELINUX_ERROR,
> >                            "%s:  line %u has invalid regex %s:  %s\n",
> > -                          path, lineno, regex, errbuf);
> > +                          path, lineno, found.regex, errbuf);
> >                 errno = EINVAL;
> >                 return -1;
> >         }
> >
> > -       if (type) {
> > -               mode_t mode = string_to_mode(type);
> > +       if (found.type) {
> > +               mode_t mode = string_to_mode(found.type);
> >
> >                 if (mode == (mode_t)-1) {
> >                         COMPAT_LOG(SELINUX_ERROR,
> >                                    "%s:  line %u has invalid file type
%s\n",
> > -                                  path, lineno, type);
> > +                                  path, lineno, found.type);
> >                         errno = EINVAL;
> >                         return -1;
> >                 }
> > @@ -464,7 +482,7 @@ static inline int process_line(struct
selabel_handle *rec,
> >          * any meta characters in the RE */
> >         spec_hasMetaChars(&spec_arr[nspec]);
> >
> > -       if (strcmp(context, "<<none>>") && rec->validating)
> > +       if (strcmp(found.context, "<<none>>") && rec->validating)
> >                 compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
> >
> >         return 0;
> > diff --git a/libselinux/src/label_internal.h
b/libselinux/src/label_internal.h
> > index aa48fff..e40d68b 100644
> > --- a/libselinux/src/label_internal.h
> > +++ b/libselinux/src/label_internal.h
> > @@ -136,10 +136,11 @@ compat_validate(struct selabel_handle *rec,
> >                 struct selabel_lookup_rec *contexts,
> >                 const char *path, unsigned lineno) hidden;
> >
> > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> >  /*
> >   * The read_spec_entries function may be used to
> >   * replace sscanf to read entries from spec files.
> >   */
> > -extern int read_spec_entries(char *line_buf, const char **errbuf, int
num_args, ...);
> > +extern int hidden read_spec_entries(char *line_buf, const char
**errbuf, unsigned int num, char *found[]);
> >
> >  #endif                         /* _SELABEL_INTERNAL_H_ */
> > diff --git a/libselinux/src/label_support.c
b/libselinux/src/label_support.c
> > index 26f9ef1..3f5728d 100644
> > --- a/libselinux/src/label_support.c
> > +++ b/libselinux/src/label_support.c
> > @@ -17,39 +17,40 @@
> >   * Read an entry from a spec file (e.g. file_contexts)
> >   * entry - Buffer to allocate for the entry.
> >   * ptr - current location of the line to be processed.
> > - * returns  - 0 on success and *entry is set to be a null
> > - *            terminated value. On Error it returns -1 and
> > - *            errno will be set.
> > + * returns  - a pointer to the begining of the string.
> > + *            that is the
> >   *
> >   */
> > -static inline int read_spec_entry(char **entry, char **ptr, int *len,
const char **errbuf)
> > +static inline char *read_spec_entry(char **current, const char
**errbuf)
> >  {
> > -       *entry = NULL;
> > -       char *tmp_buf = NULL;
> > -
> > -       while (isspace(**ptr) && **ptr != '\0')
> > -               (*ptr)++;
> > +       if(!**current) {
> > +               (*current)++;
> > +               return NULL;
> > +       }
> >
> > -       tmp_buf = *ptr;
> > -       *len = 0;
> > +       while (isspace(**current) && **current != '\0')
> > +               (*current)++;
> >
> > -       while (!isspace(**ptr) && **ptr != '\0') {
> > -               if (!isascii(**ptr)) {
> > +       char *start = *current;
> > +       while (!isspace(**current) && **current != '\0') {
> > +               if (!isascii(**current)) {
> >                         errno = EINVAL;
> >                         *errbuf = "Non-ASCII characters found";
> > -                       return -1;
> > +                       return NULL;
> >                 }
> > -               (*ptr)++;
> > -               (*len)++;
> > +               (*current)++;
> >         }
> >
> > -       if (*len) {
> > -               *entry = strndup(tmp_buf, *len);
> > -               if (!*entry)
> > -                       return -1;
> > -       }
> > +       char *end = *current;
> >
> > -       return 0;
> > +       if (start == end)
> > +               return NULL;
> > +
> > +       if (*end != '\0') {
> > +               *end = '\0';
> > +               (*current)++;
> > +       }
> > +       return start;
> >  }
> >
> >  /*
> > @@ -63,15 +64,13 @@ static inline int read_spec_entry(char **entry,
char **ptr, int *len, const char
> >   * This function calls read_spec_entry() to do the actual string
processing.
> >   * As such, can return anything from that function as well.
> >   */
> > -int hidden read_spec_entries(char *line_buf, const char **errbuf, int
num_args, ...)
> > +int hidden read_spec_entries(char *line_buf, const char **errbuf,
unsigned int num, char *found[])
> >  {
> > -       char **spec_entry, *buf_p;
> > -       int len, rc, items, entry_len = 0;
> > -       va_list ap;
> > -
> > +       char*buf_p;
> > +       unsigned int items = 0;
> >         *errbuf = NULL;
> >
> > -       len = strlen(line_buf);
> > +       size_t len = strlen(line_buf);
> >         if (line_buf[len - 1] == '\n')
> >                 line_buf[len - 1] = '\0';
> >         else
> > @@ -89,26 +88,12 @@ int hidden read_spec_entries(char *line_buf, const
char **errbuf, int num_args,
> >                 return 0;
> >
> >         /* Process the spec file entries */
> > -       va_start(ap, num_args);
> > -
> > -       items = 0;
> > -       while (items < num_args) {
> > -               spec_entry = va_arg(ap, char **);
> > -
> > -               if (len - 1 == buf_p - line_buf) {
> > -                       va_end(ap);
> > -                       return items;
> > -               }
> > -
> > -               rc = read_spec_entry(spec_entry, &buf_p, &entry_len,
errbuf);
> > -               if (rc < 0) {
> > -                       va_end(ap);
> > -                       return rc;
> > -               }
> > -               if (entry_len)
> > -                       items++;
> > +       while (items < num) {
> > +               found[items] = read_spec_entry(&buf_p, errbuf);
> > +               if (!found[items])
> > +                       break;
> > +               items++;
> >         }
> > -       va_end(ap);
> >         return items;
> >  }
> >
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Selinux mailing list
> > [e-mail]
> > To unsubscribe, send email to [e-mail].
> > To get help, send an email containing "help" to
[e-mail].
>
>
> _______________________________________________
> Selinux mailing list
> [e-mail]
> To unsubscribe, send email to [e-mail].
> To get help, send an email containing "help" to
[e-mail].

Jason Zaman Sept. 20, 2016, 5:25 a.m. UTC | #4

On 20 Sep 2016 12:50 pm, "William Roberts" <[e-mail]> wrote:
>
> On Sep 19, 2016 21:16, "Jason Zaman" <[e-mail]> wrote:
> >
> > On 20 Sep 2016 5:47 am, <[e-mail]> wrote:
> > >
> > > From: William Roberts <[e-mail]>
> > >
> > > THIS IS WIP...
> > >
> > > Rather than using stdio and making copies, just mmap the files
> > > and use the pointers in place. The affect of this change, is that
> > > text file load time is now faster than binary load time by 4.7%
> > > when testing with a file_contexts file from the Android tree. Note
> > > that the Android doesn't use monstrous regexs.
> > >
> > > Times are the average of 3 runs.
> > >
> > > BEFORE:
> > > Text file allocs: 114803
> > > Text file load time: 0.266101
> > > Bin file allocs: 93073
> > > Bin file load time: 0.248757667
> > >
> > > AFTER:
> > > Text file allocs: 103933
> > > Text file load time: 0.236192667
> > > Bin file allocs: 87645
> > > Bin file load time: .247607333
> >
> > Do you have the scripts that generated these stats so I can play with
it too? These stats are from android right? Do you also have a comparison
for refpolicy too?
>
> For generating these I used checkfc.c from the Android tree. I used
valgrind to measure allocations and clock to measure the time in
selabel_open().

Okay cool I'll fetch that and give it a whirl when I get time.

> >
> > I haven't looked that closely yet but just realised, will this need new
perms because of the mmap? If it does, can you send a patch to refpolicy?
>
> I'm confused, mmap is not a permission, even if it was the binary path
already was doing an mmap, so the permission would have been there. We're
just making it so it always mmaps.

Yeah but mmap needs execute perms sometimes (always?). I am out so just
wanted to send an email before I forgot. If it was mmaping already then
there is nothing to worry about :).

-- Jason
> >
> > > THINGS TO DO:
> > > 1. What's arm performance like?
> > > 2. What interfaces to backends are busted by this (if any)?
> > > 3. Test Android Properties
> > > 4. Im pretty sure this breaks sefcontext_compile, fix.
> > > 5. Test with PCRE2 enabled.
> > > 6. Spell check this message!
> > > 7. Run checkpatch
> > >
> > > Signed-off-by: William Roberts <[e-mail]>
> > > ---
> > >  libselinux/src/label.c                  |   2 -
> > >  libselinux/src/label_android_property.c |  22 ++---
> > >  libselinux/src/label_file.c             | 140
+++++++++++++++++++-------------
> > >  libselinux/src/label_file.h             |  66 +++++++++------
> > >  libselinux/src/label_internal.h         |   3 +-
> > >  libselinux/src/label_support.c          |  79 ++++++++----------
> > >  6 files changed, 172 insertions(+), 140 deletions(-)
> > >
> > > diff --git a/libselinux/src/label.c b/libselinux/src/label.c
> > > index 963bfcb..d767b49 100644
> > > --- a/libselinux/src/label.c
> > > +++ b/libselinux/src/label.c
> > > @@ -15,8 +15,6 @@
> > >  #include "callbacks.h"
> > >  #include "label_internal.h"
> > >
> > > -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > > -
> > >  typedef int (*selabel_initfunc)(struct selabel_handle *rec,
> > >                                 const struct selinux_opt *opts,
> > >                                 unsigned nopts);
> > > diff --git a/libselinux/src/label_android_property.c
b/libselinux/src/label_android_property.c
> > > index 290b438..2aac394 100644
> > > --- a/libselinux/src/label_android_property.c
> > > +++ b/libselinux/src/label_android_property.c
> > > @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle
*rec,
> > >                         int pass, unsigned lineno)
> > >  {
> > >         int items;
> > > -       char *prop = NULL, *context = NULL;
> > > +       union {
> > > +               struct {
> > > +                       char *prop;
> > > +                       char *context;
> > > +               };
> > > +               char *array[2];
> > > +       } found = { .array = { 0 } };
> > >         struct saved_data *data = (struct saved_data *)rec->data;
> > >         spec_t *spec_arr = data->spec_arr;
> > >         unsigned int nspec = data->nspec;
> > >         const char *errbuf = NULL;
> > >
> > > -       items = read_spec_entries(line_buf, &errbuf, 2, &prop,
&context);
> > > +       items = read_spec_entries(line_buf, &errbuf,
ARRAY_SIZE(found.array), found.array);
> > >         if (items < 0) {
> > >                 items = errno;
> > >                 selinux_log(SELINUX_ERROR,
> > > @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle
*rec,
> > >                 selinux_log(SELINUX_ERROR,
> > >                             "%s:  line %u is missing fields\n", path,
> > >                             lineno);
> > > -               free(prop);
> > >                 errno = EINVAL;
> > >                 return -1;
> > >         }
> > >
> > > -       if (pass == 0) {
> > > -               free(prop);
> > > -               free(context);
> > > -       } else if (pass == 1) {
> > > +       if (pass == 1) {
> > >                 /* On the second pass, process and store the
specification in spec. */
> > > -               spec_arr[nspec].property_key = prop;
> > > -               spec_arr[nspec].lr.ctx_raw = context;
> > > +               spec_arr[nspec].property_key = found.prop;
> > > +               spec_arr[nspec].lr.ctx_raw = found.context;
> > >
> > >                 if (rec->validating) {
> > >                         if (selabel_validate(rec,
&spec_arr[nspec].lr) < 0) {
> > > @@ -234,7 +236,7 @@ static void closef(struct selabel_handle *rec)
> > >         for (i = 0; i < data->nspec; i++) {
> > >                 spec = &data->spec_arr[i];
> > >                 free(spec->property_key);
> > > -               free(spec->lr.ctx_raw);
> > > +               //free(spec->lr.ctx_raw);
> > >                 free(spec->lr.ctx_trans);
> > >         }
> > >
> > > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > > index 7156825..4dc440e 100644
> > > --- a/libselinux/src/label_file.c
> > > +++ b/libselinux/src/label_file.c
> > > @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data,
const char *path)
> > >         return rc;
> > >  }
> > >
> > > -static int process_text_file(FILE *fp, const char *prefix,
> > > -                            struct selabel_handle *rec, const char
*path)
> > > +static inline struct saved_data *rec_to_data(struct selabel_handle
*rec)
> > > +{
> > > +       return (struct saved_data *)rec->data;
> > > +}
> > > +
> > > +static char *mmap_area_get_line(struct mmap_area *area)
> > > +{
> > > +       size_t len = area->next_len;
> > > +       if (!len)
> > > +               return NULL;
> > > +
> > > +       char *start = area->next_addr;
> > > +       char *end = memchr(start, '\n', len);
> > > +
> > > +       /* the file may not end with a newline */
> > > +       if (!end)
> > > +               end = (char *)area->next_addr + len - 1;
> > > +
> > > +       *end = '\0';
> > > +       /* len includes null byte */
> > > +       len = end - start;
> > > +
> > > +       area->next_len -= len + 1;
> > > +       area->next_addr = ++end;
> > > +       return start;
> > > +}
> > > +
> > > +static int process_text_file(const char *path, const char *prefix,
> > > +               struct selabel_handle *rec)
> > >  {
> > >         int rc;
> > > -       size_t line_len;
> > >         unsigned int lineno = 0;
> > > -       char *line_buf = NULL;
> > > +       char *line_buf;
> > > +       struct mmap_area *areas = rec_to_data(rec)->mmap_areas;
> > > +
> > > +       /* mmap_area_get_line() and process_line() require mutable
string pointers */
> > > +       rc = mprotect(areas->addr, areas->len, PROT_READ |
PROT_WRITE);
> > > +       if (rc < 0)
> > > +               goto out;
> > >
> > > -       while (getline(&line_buf, &line_len, fp) > 0) {
> > > +       while ( (line_buf = mmap_area_get_line(areas)) ) {
> > >                 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)
> > > +static int mmap_file(FILE *fp, size_t len, struct saved_data *data)
> > >  {
> > > -       struct saved_data *data = (struct saved_data *)rec->data;
> > > -       int rc;
> > > -       char *addr, *str_buf;
> > > -       int *stem_map;
> > > -       struct mmap_area *mmap_area;
> > > -       uint32_t i, magic, version;
> > > -       uint32_t entry_len, stem_map_len, regex_array_len;
> > > -       const char *reg_version;
> > > -
> > > -       mmap_area = malloc(sizeof(*mmap_area));
> > > +       struct mmap_area *mmap_area = malloc(sizeof(*mmap_area));
> > >         if (!mmap_area) {
> > >                 return -1;
> > >         }
> > >
> > > -       addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
> > > +       void *addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE,
fileno(fp), 0);
> > >         if (addr == MAP_FAILED) {
> > >                 free(mmap_area);
> > >                 perror("mmap");
> > > @@ -145,17 +166,26 @@ static int load_mmap(FILE *fp, size_t len,
struct selabel_handle *rec,
> > >         mmap_area->next = data->mmap_areas;
> > >         data->mmap_areas = mmap_area;
> > >
> > > -       /* 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)
> > > -               return -1;
> > > +       return 0;
> > > +}
> > > +
> > > +static int process_binary_file(const char *path, struct
selabel_handle *rec)
> > > +{
> > > +       struct saved_data *data = (struct saved_data *)rec->data;
> > > +       int rc;
> > > +       char *str_buf;
> > > +       int *stem_map;
> > > +       struct mmap_area *mmap_area = data->mmap_areas;
> > > +       uint32_t i, version;
> > > +       uint32_t entry_len, stem_map_len, regex_array_len;
> > > +       size_t len;
> > >
> > >         /* check if this version is higher than we understand */
> > >         rc = next_entry(&version, mmap_area, sizeof(uint32_t));
> > >         if (rc < 0 || version > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
> > >                 return -1;
> > >
> > > -       reg_version = regex_version();
> > > +       const char *reg_version = regex_version();
> > >         if (!reg_version)
> > >                 return -1;
> > >
> > > @@ -235,12 +265,11 @@ static int load_mmap(FILE *fp, size_t len,
struct selabel_handle *rec,
> > >                 /* store the mapping between old and new */
> > >                 newid = find_stem(data, buf, stem_len);
> > >                 if (newid < 0) {
> > > -                       newid = store_stem(data, buf, stem_len);
> > > +                       newid = store_stem(data, buf, stem_len, true);
> > >                         if (newid < 0) {
> > >                                 rc = newid;
> > >                                 goto out;
> > >                         }
> > > -                       data->stem_arr[newid].from_mmap = 1;
> > >                 }
> > >                 stem_map[i] = newid;
> > >         }
> > > @@ -262,7 +291,6 @@ static int load_mmap(FILE *fp, size_t len, struct
selabel_handle *rec,
> > >                         goto out;
> > >
> > >                 spec = &data->spec_arr[data->nspec];
> > > -               spec->from_mmap = 1;
> > >
> > >                 /* Process context */
> > >                 rc = next_entry(&entry_len, mmap_area,
sizeof(uint32_t));
> > > @@ -271,22 +299,10 @@ static int load_mmap(FILE *fp, size_t len,
struct selabel_handle *rec,
> > >                         goto out;
> > >                 }
> > >
> > > -               str_buf = malloc(entry_len);
> > > -               if (!str_buf) {
> > > -                       rc = -1;
> > > -                       goto out;
> > > -               }
> > > -               rc = next_entry(str_buf, mmap_area, entry_len);
> > > +               rc = next_pstr(&spec->lr.ctx_raw, mmap_area,
entry_len);
> > >                 if (rc < 0)
> > >                         goto out;
> > >
> > > -               if (str_buf[entry_len - 1] != '\0') {
> > > -                       free(str_buf);
> > > -                       rc = -1;
> > > -                       goto out;
> > > -               }
> > > -               spec->lr.ctx_raw = str_buf;
> > > -
> > >                 if (strcmp(spec->lr.ctx_raw, "<<none>>") &&
rec->validating) {
> > >                         if (selabel_validate(rec, &spec->lr) < 0) {
> > >                                 selinux_log(SELINUX_ERROR,
> > > @@ -401,17 +417,29 @@ static char *rolling_append(char *current,
const char *suffix, size_t max)
> > >         return current;
> > >  }
> > >
> > > -static bool fcontext_is_binary(FILE *fp)
> > > +static inline int mmap_area_rewind(struct mmap_area *area, size_t
bytes)
> > > +{
> > > +       if (area->next_len + bytes > area->len)
> > > +               return -1;
> > > +
> > > +       area->next_addr = (char *) area->next_addr - bytes;
> > > +       area->next_len += bytes;
> > > +       return 0;
> > > +}
> > > +
> > > +static bool fcontext_is_binary(struct mmap_area *area)
> > >  {
> > >         uint32_t magic;
> > > +       bool is_binary = false;
> > >
> > > -       size_t len = fread(&magic, sizeof(magic), 1, fp);
> > > -       rewind(fp);
> > > +       int rc = next_entry(&magic, area, sizeof(magic));
> > >
> > > -       return (len && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
> > > -}
> > > +       is_binary = (!rc && (magic ==
SELINUX_MAGIC_COMPILED_FCONTEXT));
> > > +       if (!is_binary)
> > > +               mmap_area_rewind(area, sizeof(magic));
> > >
> > > -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > > +       return is_binary;
> > > +}
> > >
> > >  static FILE *open_file(const char *path, const char *suffix,
> > >                char *save_path, size_t len, struct stat *sb, bool
open_oldest)
> > > @@ -504,12 +532,17 @@ static int process_file(const char *path, const
char *suffix,
> > >                 if (fp == NULL)
> > >                         return -1;
> > >
> > > -               rc = fcontext_is_binary(fp) ?
> > > -                               load_mmap(fp, sb.st_size, rec,
found_path) :
> > > -                               process_text_file(fp, prefix, rec,
found_path);
> > > +               rc = mmap_file(fp, sb.st_size, rec_to_data(rec));
> > > +               if (rc < 0) {
> > > +                       fclose(fp);
> > > +                       return -1;
> > > +               }
> > > +
> > > +               rc = fcontext_is_binary(rec_to_data(rec)->mmap_areas)
?
> > > +                               process_binary_file(found_path, rec) :
> > > +                               process_text_file(found_path, prefix,
rec);
> > >                 if (!rc)
> > > -                       rc = digest_add_specfile(digest, fp, NULL,
sb.st_size,
> > > -                               found_path);
> > > +                       rc = digest_add_specfile(digest, fp, NULL,
sb.st_size, found_path);
> > >
> > >                 fclose(fp);
> > >
> > > @@ -613,12 +646,7 @@ static void closef(struct selabel_handle *rec)
> > >         for (i = 0; i < data->nspec; i++) {
> > >                 spec = &data->spec_arr[i];
> > >                 free(spec->lr.ctx_trans);
> > > -               free(spec->lr.ctx_raw);
> > >                 regex_data_free(spec->regex);
> > > -               if (spec->from_mmap)
> > > -                       continue;
> > > -               free(spec->regex_str);
> > > -               free(spec->type_str);
> > >         }
> > >
> > >         for (i = 0; i < (unsigned int)data->num_stems; i++) {
> > > diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
> > > index 88f4294..2704906 100644
> > > --- a/libselinux/src/label_file.h
> > > +++ b/libselinux/src/label_file.h
> > > @@ -37,7 +37,6 @@ struct spec {
> > >         int matches;            /* number of matching pathnames */
> > >         int stem_id;            /* indicates which stem-compression
item */
> > >         char hasMetaChars;      /* regular expression has meta-chars
*/
> > > -       char from_mmap;         /* this spec is from an mmap of the
data */
> > >         size_t prefix_len;      /* length of fixed path prefix */
> > >  };
> > >
> > > @@ -255,7 +254,7 @@ static inline int find_stem(struct saved_data
*data, const char *buf,
> > >  }
> > >
> > >  /* returns the index of the new stored object */
> > > -static inline int store_stem(struct saved_data *data, char *buf, int
stem_len)
> > > +static inline int store_stem(struct saved_data *data, char *buf, int
stem_len, bool from_mmap)
> > >  {
> > >         int num = data->num_stems;
> > >
> > > @@ -271,7 +270,7 @@ static inline int store_stem(struct saved_data
*data, char *buf, int stem_len)
> > >         }
> > >         data->stem_arr[num].len = stem_len;
> > >         data->stem_arr[num].buf = buf;
> > > -       data->stem_arr[num].from_mmap = 0;
> > > +       data->stem_arr[num].from_mmap = from_mmap;
> > >         data->num_stems++;
> > >
> > >         return num;
> > > @@ -298,7 +297,7 @@ static inline int find_stem_from_spec(struct
saved_data *data, const char *buf)
> > >         if (!stem)
> > >                 return -1;
> > >
> > > -       return store_stem(data, stem, stem_len);
> > > +       return store_stem(data, stem, stem_len, false);
> > >  }
> > >
> > >  /* This will always check for buffer over-runs and either read the
next entry
> > > @@ -317,6 +316,22 @@ static inline int next_entry(void *buf, struct
mmap_area *fp, size_t bytes)
> > >         return 0;
> > >  }
> > >
> > > +static inline int next_pstr(char **str, struct mmap_area *fp, size_t
len)
> > > +{
> > > +       if (len > fp->next_len)
> > > +               return -1;
> > > +
> > > +       char *tmp = (char *)fp->next_addr;
> > > +       if (tmp[len-1] != '\0')
> > > +               return -1;
> > > +
> > > +       *str = tmp;
> > > +
> > > +       fp->next_addr = (char *)fp->next_addr + len;
> > > +       fp->next_len -= len;
> > > +       return 0;
> > > +}
> > > +
> > >  static inline int compile_regex(struct saved_data *data, struct spec
*spec,
> > >                                             const char **errbuf)
> > >  {
> > > @@ -375,13 +390,21 @@ static inline int process_line(struct
selabel_handle *rec,
> > >                         char *line_buf, unsigned lineno)
> > >  {
> > >         int items, len, rc;
> > > -       char *regex = NULL, *type = NULL, *context = NULL;
> > > +       union {
> > > +               struct {
> > > +                       char *regex;
> > > +                       char *type;
> > > +                       char *context;
> > > +               };
> > > +               char *array[3];
> > > +       } found = { .array = { 0 } };
> > > +
> > >         struct saved_data *data = (struct saved_data *)rec->data;
> > >         struct spec *spec_arr;
> > >         unsigned int nspec = data->nspec;
> > >         const char *errbuf = NULL;
> > >
> > > -       items = read_spec_entries(line_buf, &errbuf, 3, &regex,
&type, &context);
> > > +       items = read_spec_entries(line_buf, &errbuf,
ARRAY_SIZE(found.array), found.array);
> > >         if (items < 0) {
> > >                 rc = errno;
> > >                 selinux_log(SELINUX_ERROR,
> > > @@ -398,22 +421,17 @@ static inline int process_line(struct
selabel_handle *rec,
> > >                 COMPAT_LOG(SELINUX_ERROR,
> > >                             "%s:  line %u is missing fields\n", path,
> > >                             lineno);
> > > -               if (items == 1)
> > > -                       free(regex);
> > >                 errno = EINVAL;
> > >                 return -1;
> > >         } else if (items == 2) {
> > >                 /* The type field is optional. */
> > > -               context = type;
> > > -               type = 0;
> > > +               found.context = found.type;
> > > +               found.type = NULL;
> > >         }
> > >
> > > -       len = get_stem_from_spec(regex);
> > > -       if (len && prefix && strncmp(prefix, regex, len)) {
> > > +       len = get_stem_from_spec(found.regex);
> > > +       if (len && prefix && strncmp(prefix, found.regex, len)) {
> > >                 /* Stem of regex does not match requested prefix,
discard. */
> > > -               free(regex);
> > > -               free(type);
> > > -               free(context);
> > >                 return 0;
> > >         }
> > >
> > > @@ -424,13 +442,13 @@ static inline int process_line(struct
selabel_handle *rec,
> > >         spec_arr = data->spec_arr;
> > >
> > >         /* process and store the specification in spec. */
> > > -       spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
> > > -       spec_arr[nspec].regex_str = regex;
> > > +       spec_arr[nspec].stem_id = find_stem_from_spec(data,
found.regex);
> > > +       spec_arr[nspec].regex_str = found.regex;
> > >
> > > -       spec_arr[nspec].type_str = type;
> > > +       spec_arr[nspec].type_str = found.type;
> > >         spec_arr[nspec].mode = 0;
> > >
> > > -       spec_arr[nspec].lr.ctx_raw = context;
> > > +       spec_arr[nspec].lr.ctx_raw = found.context;
> > >
> > >         /*
> > >          * bump data->nspecs to cause closef() to cover it in its free
> > > @@ -442,18 +460,18 @@ static inline int process_line(struct
selabel_handle *rec,
> > >                         && compile_regex(data, &spec_arr[nspec],
&errbuf)) {
> > >                 COMPAT_LOG(SELINUX_ERROR,
> > >                            "%s:  line %u has invalid regex %s:  %s\n",
> > > -                          path, lineno, regex, errbuf);
> > > +                          path, lineno, found.regex, errbuf);
> > >                 errno = EINVAL;
> > >                 return -1;
> > >         }
> > >
> > > -       if (type) {
> > > -               mode_t mode = string_to_mode(type);
> > > +       if (found.type) {
> > > +               mode_t mode = string_to_mode(found.type);
> > >
> > >                 if (mode == (mode_t)-1) {
> > >                         COMPAT_LOG(SELINUX_ERROR,
> > >                                    "%s:  line %u has invalid file
type %s\n",
> > > -                                  path, lineno, type);
> > > +                                  path, lineno, found.type);
> > >                         errno = EINVAL;
> > >                         return -1;
> > >                 }
> > > @@ -464,7 +482,7 @@ static inline int process_line(struct
selabel_handle *rec,
> > >          * any meta characters in the RE */
> > >         spec_hasMetaChars(&spec_arr[nspec]);
> > >
> > > -       if (strcmp(context, "<<none>>") && rec->validating)
> > > +       if (strcmp(found.context, "<<none>>") && rec->validating)
> > >                 compat_validate(rec, &spec_arr[nspec].lr, path,
lineno);
> > >
> > >         return 0;
> > > diff --git a/libselinux/src/label_internal.h
b/libselinux/src/label_internal.h
> > > index aa48fff..e40d68b 100644
> > > --- a/libselinux/src/label_internal.h
> > > +++ b/libselinux/src/label_internal.h
> > > @@ -136,10 +136,11 @@ compat_validate(struct selabel_handle *rec,
> > >                 struct selabel_lookup_rec *contexts,
> > >                 const char *path, unsigned lineno) hidden;
> > >
> > > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > >  /*
> > >   * The read_spec_entries function may be used to
> > >   * replace sscanf to read entries from spec files.
> > >   */
> > > -extern int read_spec_entries(char *line_buf, const char **errbuf,
int num_args, ...);
> > > +extern int hidden read_spec_entries(char *line_buf, const char
**errbuf, unsigned int num, char *found[]);
> > >
> > >  #endif                         /* _SELABEL_INTERNAL_H_ */
> > > diff --git a/libselinux/src/label_support.c
b/libselinux/src/label_support.c
> > > index 26f9ef1..3f5728d 100644
> > > --- a/libselinux/src/label_support.c
> > > +++ b/libselinux/src/label_support.c
> > > @@ -17,39 +17,40 @@
> > >   * Read an entry from a spec file (e.g. file_contexts)
> > >   * entry - Buffer to allocate for the entry.
> > >   * ptr - current location of the line to be processed.
> > > - * returns  - 0 on success and *entry is set to be a null
> > > - *            terminated value. On Error it returns -1 and
> > > - *            errno will be set.
> > > + * returns  - a pointer to the begining of the string.
> > > + *            that is the
> > >   *
> > >   */
> > > -static inline int read_spec_entry(char **entry, char **ptr, int
*len, const char **errbuf)
> > > +static inline char *read_spec_entry(char **current, const char
**errbuf)
> > >  {
> > > -       *entry = NULL;
> > > -       char *tmp_buf = NULL;
> > > -
> > > -       while (isspace(**ptr) && **ptr != '\0')
> > > -               (*ptr)++;
> > > +       if(!**current) {
> > > +               (*current)++;
> > > +               return NULL;
> > > +       }
> > >
> > > -       tmp_buf = *ptr;
> > > -       *len = 0;
> > > +       while (isspace(**current) && **current != '\0')
> > > +               (*current)++;
> > >
> > > -       while (!isspace(**ptr) && **ptr != '\0') {
> > > -               if (!isascii(**ptr)) {
> > > +       char *start = *current;
> > > +       while (!isspace(**current) && **current != '\0') {
> > > +               if (!isascii(**current)) {
> > >                         errno = EINVAL;
> > >                         *errbuf = "Non-ASCII characters found";
> > > -                       return -1;
> > > +                       return NULL;
> > >                 }
> > > -               (*ptr)++;
> > > -               (*len)++;
> > > +               (*current)++;
> > >         }
> > >
> > > -       if (*len) {
> > > -               *entry = strndup(tmp_buf, *len);
> > > -               if (!*entry)
> > > -                       return -1;
> > > -       }
> > > +       char *end = *current;
> > >
> > > -       return 0;
> > > +       if (start == end)
> > > +               return NULL;
> > > +
> > > +       if (*end != '\0') {
> > > +               *end = '\0';
> > > +               (*current)++;
> > > +       }
> > > +       return start;
> > >  }
> > >
> > >  /*
> > > @@ -63,15 +64,13 @@ static inline int read_spec_entry(char **entry,
char **ptr, int *len, const char
> > >   * This function calls read_spec_entry() to do the actual string
processing.
> > >   * As such, can return anything from that function as well.
> > >   */
> > > -int hidden read_spec_entries(char *line_buf, const char **errbuf,
int num_args, ...)
> > > +int hidden read_spec_entries(char *line_buf, const char **errbuf,
unsigned int num, char *found[])
> > >  {
> > > -       char **spec_entry, *buf_p;
> > > -       int len, rc, items, entry_len = 0;
> > > -       va_list ap;
> > > -
> > > +       char*buf_p;
> > > +       unsigned int items = 0;
> > >         *errbuf = NULL;
> > >
> > > -       len = strlen(line_buf);
> > > +       size_t len = strlen(line_buf);
> > >         if (line_buf[len - 1] == '\n')
> > >                 line_buf[len - 1] = '\0';
> > >         else
> > > @@ -89,26 +88,12 @@ int hidden read_spec_entries(char *line_buf,
const char **errbuf, int num_args,
> > >                 return 0;
> > >
> > >         /* Process the spec file entries */
> > > -       va_start(ap, num_args);
> > > -
> > > -       items = 0;
> > > -       while (items < num_args) {
> > > -               spec_entry = va_arg(ap, char **);
> > > -
> > > -               if (len - 1 == buf_p - line_buf) {
> > > -                       va_end(ap);
> > > -                       return items;
> > > -               }
> > > -
> > > -               rc = read_spec_entry(spec_entry, &buf_p, &entry_len,
errbuf);
> > > -               if (rc < 0) {
> > > -                       va_end(ap);
> > > -                       return rc;
> > > -               }
> > > -               if (entry_len)
> > > -                       items++;
> > > +       while (items < num) {
> > > +               found[items] = read_spec_entry(&buf_p, errbuf);
> > > +               if (!found[items])
> > > +                       break;
> > > +               items++;
> > >         }
> > > -       va_end(ap);
> > >         return items;
> > >  }
> > >
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Selinux mailing list
> > > [e-mail]
> > > To unsubscribe, send email to [e-mail].
> > > To get help, send an email containing "help" to
[e-mail].
> >
> >
> > _______________________________________________
> > Selinux mailing list
> > [e-mail]
> > To unsubscribe, send email to [e-mail].
> > To get help, send an email containing "help" to
[e-mail].

William Roberts Sept. 20, 2016, 6:27 a.m. UTC | #5

On Sep 19, 2016 22:25, "Jason Zaman" <[e-mail]> wrote:
>
> On 20 Sep 2016 12:50 pm, "William Roberts" <[e-mail]>
wrote:
> >
> > On Sep 19, 2016 21:16, "Jason Zaman" <[e-mail]> wrote:
> > >
> > > On 20 Sep 2016 5:47 am, <[e-mail]> wrote:
> > > >
> > > > From: William Roberts <[e-mail]>
> > > >
> > > > THIS IS WIP...
> > > >
> > > > Rather than using stdio and making copies, just mmap the files
> > > > and use the pointers in place. The affect of this change, is that
> > > > text file load time is now faster than binary load time by 4.7%
> > > > when testing with a file_contexts file from the Android tree. Note
> > > > that the Android doesn't use monstrous regexs.
> > > >
> > > > Times are the average of 3 runs.
> > > >
> > > > BEFORE:
> > > > Text file allocs: 114803
> > > > Text file load time: 0.266101
> > > > Bin file allocs: 93073
> > > > Bin file load time: 0.248757667
> > > >
> > > > AFTER:
> > > > Text file allocs: 103933
> > > > Text file load time: 0.236192667
> > > > Bin file allocs: 87645
> > > > Bin file load time: .247607333
> > >
> > > Do you have the scripts that generated these stats so I can play with
it too? These stats are from android right? Do you also have a comparison
for refpolicy too?
> >
> > For generating these I used checkfc.c from the Android tree. I used
valgrind to measure allocations and clock to measure the time in
selabel_open().
>
> Okay cool I'll fetch that and give it a whirl when I get time.
>
> > >
> > > I haven't looked that closely yet but just realised, will this need
new perms because of the mmap? If it does, can you send a patch to
refpolicy?
> >
> > I'm confused, mmap is not a permission, even if it was the binary path
already was doing an mmap, so the permission would have been there. We're
just making it so it always mmaps.
>
> Yeah but mmap needs execute perms sometimes (always?). I am out so just
wanted to send an email before I forgot. If it was mmaping already then
there is nothing to worry about :).

Mmap would only need execute if you attempted to set the prot bits to
execute it use mprotect to change the mapping. Then things like execmod
might come I to play if the mapping was ever writable.

>
> -- Jason
> > >
> > > > THINGS TO DO:
> > > > 1. What's arm performance like?
> > > > 2. What interfaces to backends are busted by this (if any)?
> > > > 3. Test Android Properties
> > > > 4. Im pretty sure this breaks sefcontext_compile, fix.
> > > > 5. Test with PCRE2 enabled.
> > > > 6. Spell check this message!
> > > > 7. Run checkpatch
> > > >
> > > > Signed-off-by: William Roberts <[e-mail]>
> > > > ---
> > > >  libselinux/src/label.c                  |   2 -
> > > >  libselinux/src/label_android_property.c |  22 ++---
> > > >  libselinux/src/label_file.c             | 140
+++++++++++++++++++-------------
> > > >  libselinux/src/label_file.h             |  66 +++++++++------
> > > >  libselinux/src/label_internal.h         |   3 +-
> > > >  libselinux/src/label_support.c          |  79 ++++++++----------
> > > >  6 files changed, 172 insertions(+), 140 deletions(-)
> > > >
> > > > diff --git a/libselinux/src/label.c b/libselinux/src/label.c
> > > > index 963bfcb..d767b49 100644
> > > > --- a/libselinux/src/label.c
> > > > +++ b/libselinux/src/label.c
> > > > @@ -15,8 +15,6 @@
> > > >  #include "callbacks.h"
> > > >  #include "label_internal.h"
> > > >
> > > > -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > > > -
> > > >  typedef int (*selabel_initfunc)(struct selabel_handle *rec,
> > > >                                 const struct selinux_opt *opts,
> > > >                                 unsigned nopts);
> > > > diff --git a/libselinux/src/label_android_property.c
b/libselinux/src/label_android_property.c
> > > > index 290b438..2aac394 100644
> > > > --- a/libselinux/src/label_android_property.c
> > > > +++ b/libselinux/src/label_android_property.c
> > > > @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle
*rec,
> > > >                         int pass, unsigned lineno)
> > > >  {
> > > >         int items;
> > > > -       char *prop = NULL, *context = NULL;
> > > > +       union {
> > > > +               struct {
> > > > +                       char *prop;
> > > > +                       char *context;
> > > > +               };
> > > > +               char *array[2];
> > > > +       } found = { .array = { 0 } };
> > > >         struct saved_data *data = (struct saved_data *)rec->data;
> > > >         spec_t *spec_arr = data->spec_arr;
> > > >         unsigned int nspec = data->nspec;
> > > >         const char *errbuf = NULL;
> > > >
> > > > -       items = read_spec_entries(line_buf, &errbuf, 2, &prop,
&context);
> > > > +       items = read_spec_entries(line_buf, &errbuf,
ARRAY_SIZE(found.array), found.array);
> > > >         if (items < 0) {
> > > >                 items = errno;
> > > >                 selinux_log(SELINUX_ERROR,
> > > > @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle
*rec,
> > > >                 selinux_log(SELINUX_ERROR,
> > > >                             "%s:  line %u is missing fields\n",
path,
> > > >                             lineno);
> > > > -               free(prop);
> > > >                 errno = EINVAL;
> > > >                 return -1;
> > > >         }
> > > >
> > > > -       if (pass == 0) {
> > > > -               free(prop);
> > > > -               free(context);
> > > > -       } else if (pass == 1) {
> > > > +       if (pass == 1) {
> > > >                 /* On the second pass, process and store the
specification in spec. */
> > > > -               spec_arr[nspec].property_key = prop;
> > > > -               spec_arr[nspec].lr.ctx_raw = context;
> > > > +               spec_arr[nspec].property_key = found.prop;
> > > > +               spec_arr[nspec].lr.ctx_raw = found.context;
> > > >
> > > >                 if (rec->validating) {
> > > >                         if (selabel_validate(rec,
&spec_arr[nspec].lr) < 0) {
> > > > @@ -234,7 +236,7 @@ static void closef(struct selabel_handle *rec)
> > > >         for (i = 0; i < data->nspec; i++) {
> > > >                 spec = &data->spec_arr[i];
> > > >                 free(spec->property_key);
> > > > -               free(spec->lr.ctx_raw);
> > > > +               //free(spec->lr.ctx_raw);
> > > >                 free(spec->lr.ctx_trans);
> > > >         }
> > > >
> > > > diff --git a/libselinux/src/label_file.c
b/libselinux/src/label_file.c
> > > > index 7156825..4dc440e 100644
> > > > --- a/libselinux/src/label_file.c
> > > > +++ b/libselinux/src/label_file.c
> > > > @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data
*data, const char *path)
> > > >         return rc;
> > > >  }
> > > >
> > > > -static int process_text_file(FILE *fp, const char *prefix,
> > > > -                            struct selabel_handle *rec, const char
*path)
> > > > +static inline struct saved_data *rec_to_data(struct selabel_handle
*rec)
> > > > +{
> > > > +       return (struct saved_data *)rec->data;
> > > > +}
> > > > +
> > > > +static char *mmap_area_get_line(struct mmap_area *area)
> > > > +{
> > > > +       size_t len = area->next_len;
> > > > +       if (!len)
> > > > +               return NULL;
> > > > +
> > > > +       char *start = area->next_addr;
> > > > +       char *end = memchr(start, '\n', len);
> > > > +
> > > > +       /* the file may not end with a newline */
> > > > +       if (!end)
> > > > +               end = (char *)area->next_addr + len - 1;
> > > > +
> > > > +       *end = '\0';
> > > > +       /* len includes null byte */
> > > > +       len = end - start;
> > > > +
> > > > +       area->next_len -= len + 1;
> > > > +       area->next_addr = ++end;
> > > > +       return start;
> > > > +}
> > > > +
> > > > +static int process_text_file(const char *path, const char *prefix,
> > > > +               struct selabel_handle *rec)
> > > >  {
> > > >         int rc;
> > > > -       size_t line_len;
> > > >         unsigned int lineno = 0;
> > > > -       char *line_buf = NULL;
> > > > +       char *line_buf;
> > > > +       struct mmap_area *areas = rec_to_data(rec)->mmap_areas;
> > > > +
> > > > +       /* mmap_area_get_line() and process_line() require mutable
string pointers */
> > > > +       rc = mprotect(areas->addr, areas->len, PROT_READ |
PROT_WRITE);
> > > > +       if (rc < 0)
> > > > +               goto out;
> > > >
> > > > -       while (getline(&line_buf, &line_len, fp) > 0) {
> > > > +       while ( (line_buf = mmap_area_get_line(areas)) ) {
> > > >                 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)
> > > > +static int mmap_file(FILE *fp, size_t len, struct saved_data *data)
> > > >  {
> > > > -       struct saved_data *data = (struct saved_data *)rec->data;
> > > > -       int rc;
> > > > -       char *addr, *str_buf;
> > > > -       int *stem_map;
> > > > -       struct mmap_area *mmap_area;
> > > > -       uint32_t i, magic, version;
> > > > -       uint32_t entry_len, stem_map_len, regex_array_len;
> > > > -       const char *reg_version;
> > > > -
> > > > -       mmap_area = malloc(sizeof(*mmap_area));
> > > > +       struct mmap_area *mmap_area = malloc(sizeof(*mmap_area));
> > > >         if (!mmap_area) {
> > > >                 return -1;
> > > >         }
> > > >
> > > > -       addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp),
0);
> > > > +       void *addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE,
fileno(fp), 0);
> > > >         if (addr == MAP_FAILED) {
> > > >                 free(mmap_area);
> > > >                 perror("mmap");
> > > > @@ -145,17 +166,26 @@ static int load_mmap(FILE *fp, size_t len,
struct selabel_handle *rec,
> > > >         mmap_area->next = data->mmap_areas;
> > > >         data->mmap_areas = mmap_area;
> > > >
> > > > -       /* 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)
> > > > -               return -1;
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int process_binary_file(const char *path, struct
selabel_handle *rec)
> > > > +{
> > > > +       struct saved_data *data = (struct saved_data *)rec->data;
> > > > +       int rc;
> > > > +       char *str_buf;
> > > > +       int *stem_map;
> > > > +       struct mmap_area *mmap_area = data->mmap_areas;
> > > > +       uint32_t i, version;
> > > > +       uint32_t entry_len, stem_map_len, regex_array_len;
> > > > +       size_t len;
> > > >
> > > >         /* check if this version is higher than we understand */
> > > >         rc = next_entry(&version, mmap_area, sizeof(uint32_t));
> > > >         if (rc < 0 || version > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
> > > >                 return -1;
> > > >
> > > > -       reg_version = regex_version();
> > > > +       const char *reg_version = regex_version();
> > > >         if (!reg_version)
> > > >                 return -1;
> > > >
> > > > @@ -235,12 +265,11 @@ static int load_mmap(FILE *fp, size_t len,
struct selabel_handle *rec,
> > > >                 /* store the mapping between old and new */
> > > >                 newid = find_stem(data, buf, stem_len);
> > > >                 if (newid < 0) {
> > > > -                       newid = store_stem(data, buf, stem_len);
> > > > +                       newid = store_stem(data, buf, stem_len,
true);
> > > >                         if (newid < 0) {
> > > >                                 rc = newid;
> > > >                                 goto out;
> > > >                         }
> > > > -                       data->stem_arr[newid].from_mmap = 1;
> > > >                 }
> > > >                 stem_map[i] = newid;
> > > >         }
> > > > @@ -262,7 +291,6 @@ static int load_mmap(FILE *fp, size_t len,
struct selabel_handle *rec,
> > > >                         goto out;
> > > >
> > > >                 spec = &data->spec_arr[data->nspec];
> > > > -               spec->from_mmap = 1;
> > > >
> > > >                 /* Process context */
> > > >                 rc = next_entry(&entry_len, mmap_area,
sizeof(uint32_t));
> > > > @@ -271,22 +299,10 @@ static int load_mmap(FILE *fp, size_t len,
struct selabel_handle *rec,
> > > >                         goto out;
> > > >                 }
> > > >
> > > > -               str_buf = malloc(entry_len);
> > > > -               if (!str_buf) {
> > > > -                       rc = -1;
> > > > -                       goto out;
> > > > -               }
> > > > -               rc = next_entry(str_buf, mmap_area, entry_len);
> > > > +               rc = next_pstr(&spec->lr.ctx_raw, mmap_area,
entry_len);
> > > >                 if (rc < 0)
> > > >                         goto out;
> > > >
> > > > -               if (str_buf[entry_len - 1] != '\0') {
> > > > -                       free(str_buf);
> > > > -                       rc = -1;
> > > > -                       goto out;
> > > > -               }
> > > > -               spec->lr.ctx_raw = str_buf;
> > > > -
> > > >                 if (strcmp(spec->lr.ctx_raw, "<<none>>") &&
rec->validating) {
> > > >                         if (selabel_validate(rec, &spec->lr) < 0) {
> > > >                                 selinux_log(SELINUX_ERROR,
> > > > @@ -401,17 +417,29 @@ static char *rolling_append(char *current,
const char *suffix, size_t max)
> > > >         return current;
> > > >  }
> > > >
> > > > -static bool fcontext_is_binary(FILE *fp)
> > > > +static inline int mmap_area_rewind(struct mmap_area *area, size_t
bytes)
> > > > +{
> > > > +       if (area->next_len + bytes > area->len)
> > > > +               return -1;
> > > > +
> > > > +       area->next_addr = (char *) area->next_addr - bytes;
> > > > +       area->next_len += bytes;
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static bool fcontext_is_binary(struct mmap_area *area)
> > > >  {
> > > >         uint32_t magic;
> > > > +       bool is_binary = false;
> > > >
> > > > -       size_t len = fread(&magic, sizeof(magic), 1, fp);
> > > > -       rewind(fp);
> > > > +       int rc = next_entry(&magic, area, sizeof(magic));
> > > >
> > > > -       return (len && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
> > > > -}
> > > > +       is_binary = (!rc && (magic ==
SELINUX_MAGIC_COMPILED_FCONTEXT));
> > > > +       if (!is_binary)
> > > > +               mmap_area_rewind(area, sizeof(magic));
> > > >
> > > > -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > > > +       return is_binary;
> > > > +}
> > > >
> > > >  static FILE *open_file(const char *path, const char *suffix,
> > > >                char *save_path, size_t len, struct stat *sb, bool
open_oldest)
> > > > @@ -504,12 +532,17 @@ static int process_file(const char *path,
const char *suffix,
> > > >                 if (fp == NULL)
> > > >                         return -1;
> > > >
> > > > -               rc = fcontext_is_binary(fp) ?
> > > > -                               load_mmap(fp, sb.st_size, rec,
found_path) :
> > > > -                               process_text_file(fp, prefix, rec,
found_path);
> > > > +               rc = mmap_file(fp, sb.st_size, rec_to_data(rec));
> > > > +               if (rc < 0) {
> > > > +                       fclose(fp);
> > > > +                       return -1;
> > > > +               }
> > > > +
> > > > +               rc =
fcontext_is_binary(rec_to_data(rec)->mmap_areas) ?
> > > > +                               process_binary_file(found_path,
rec) :
> > > > +                               process_text_file(found_path,
prefix, rec);
> > > >                 if (!rc)
> > > > -                       rc = digest_add_specfile(digest, fp, NULL,
sb.st_size,
> > > > -                               found_path);
> > > > +                       rc = digest_add_specfile(digest, fp, NULL,
sb.st_size, found_path);
> > > >
> > > >                 fclose(fp);
> > > >
> > > > @@ -613,12 +646,7 @@ static void closef(struct selabel_handle *rec)
> > > >         for (i = 0; i < data->nspec; i++) {
> > > >                 spec = &data->spec_arr[i];
> > > >                 free(spec->lr.ctx_trans);
> > > > -               free(spec->lr.ctx_raw);
> > > >                 regex_data_free(spec->regex);
> > > > -               if (spec->from_mmap)
> > > > -                       continue;
> > > > -               free(spec->regex_str);
> > > > -               free(spec->type_str);
> > > >         }
> > > >
> > > >         for (i = 0; i < (unsigned int)data->num_stems; i++) {
> > > > diff --git a/libselinux/src/label_file.h
b/libselinux/src/label_file.h
> > > > index 88f4294..2704906 100644
> > > > --- a/libselinux/src/label_file.h
> > > > +++ b/libselinux/src/label_file.h
> > > > @@ -37,7 +37,6 @@ struct spec {
> > > >         int matches;            /* number of matching pathnames */
> > > >         int stem_id;            /* indicates which stem-compression
item */
> > > >         char hasMetaChars;      /* regular expression has
meta-chars */
> > > > -       char from_mmap;         /* this spec is from an mmap of the
data */
> > > >         size_t prefix_len;      /* length of fixed path prefix */
> > > >  };
> > > >
> > > > @@ -255,7 +254,7 @@ static inline int find_stem(struct saved_data
*data, const char *buf,
> > > >  }
> > > >
> > > >  /* returns the index of the new stored object */
> > > > -static inline int store_stem(struct saved_data *data, char *buf,
int stem_len)
> > > > +static inline int store_stem(struct saved_data *data, char *buf,
int stem_len, bool from_mmap)
> > > >  {
> > > >         int num = data->num_stems;
> > > >
> > > > @@ -271,7 +270,7 @@ static inline int store_stem(struct saved_data
*data, char *buf, int stem_len)
> > > >         }
> > > >         data->stem_arr[num].len = stem_len;
> > > >         data->stem_arr[num].buf = buf;
> > > > -       data->stem_arr[num].from_mmap = 0;
> > > > +       data->stem_arr[num].from_mmap = from_mmap;
> > > >         data->num_stems++;
> > > >
> > > >         return num;
> > > > @@ -298,7 +297,7 @@ static inline int find_stem_from_spec(struct
saved_data *data, const char *buf)
> > > >         if (!stem)
> > > >                 return -1;
> > > >
> > > > -       return store_stem(data, stem, stem_len);
> > > > +       return store_stem(data, stem, stem_len, false);
> > > >  }
> > > >
> > > >  /* This will always check for buffer over-runs and either read the
next entry
> > > > @@ -317,6 +316,22 @@ static inline int next_entry(void *buf, struct
mmap_area *fp, size_t bytes)
> > > >         return 0;
> > > >  }
> > > >
> > > > +static inline int next_pstr(char **str, struct mmap_area *fp,
size_t len)
> > > > +{
> > > > +       if (len > fp->next_len)
> > > > +               return -1;
> > > > +
> > > > +       char *tmp = (char *)fp->next_addr;
> > > > +       if (tmp[len-1] != '\0')
> > > > +               return -1;
> > > > +
> > > > +       *str = tmp;
> > > > +
> > > > +       fp->next_addr = (char *)fp->next_addr + len;
> > > > +       fp->next_len -= len;
> > > > +       return 0;
> > > > +}
> > > > +
> > > >  static inline int compile_regex(struct saved_data *data, struct
spec *spec,
> > > >                                             const char **errbuf)
> > > >  {
> > > > @@ -375,13 +390,21 @@ static inline int process_line(struct
selabel_handle *rec,
> > > >                         char *line_buf, unsigned lineno)
> > > >  {
> > > >         int items, len, rc;
> > > > -       char *regex = NULL, *type = NULL, *context = NULL;
> > > > +       union {
> > > > +               struct {
> > > > +                       char *regex;
> > > > +                       char *type;
> > > > +                       char *context;
> > > > +               };
> > > > +               char *array[3];
> > > > +       } found = { .array = { 0 } };
> > > > +
> > > >         struct saved_data *data = (struct saved_data *)rec->data;
> > > >         struct spec *spec_arr;
> > > >         unsigned int nspec = data->nspec;
> > > >         const char *errbuf = NULL;
> > > >
> > > > -       items = read_spec_entries(line_buf, &errbuf, 3, &regex,
&type, &context);
> > > > +       items = read_spec_entries(line_buf, &errbuf,
ARRAY_SIZE(found.array), found.array);
> > > >         if (items < 0) {
> > > >                 rc = errno;
> > > >                 selinux_log(SELINUX_ERROR,
> > > > @@ -398,22 +421,17 @@ static inline int process_line(struct
selabel_handle *rec,
> > > >                 COMPAT_LOG(SELINUX_ERROR,
> > > >                             "%s:  line %u is missing fields\n",
path,
> > > >                             lineno);
> > > > -               if (items == 1)
> > > > -                       free(regex);
> > > >                 errno = EINVAL;
> > > >                 return -1;
> > > >         } else if (items == 2) {
> > > >                 /* The type field is optional. */
> > > > -               context = type;
> > > > -               type = 0;
> > > > +               found.context = found.type;
> > > > +               found.type = NULL;
> > > >         }
> > > >
> > > > -       len = get_stem_from_spec(regex);
> > > > -       if (len && prefix && strncmp(prefix, regex, len)) {
> > > > +       len = get_stem_from_spec(found.regex);
> > > > +       if (len && prefix && strncmp(prefix, found.regex, len)) {
> > > >                 /* Stem of regex does not match requested prefix,
discard. */
> > > > -               free(regex);
> > > > -               free(type);
> > > > -               free(context);
> > > >                 return 0;
> > > >         }
> > > >
> > > > @@ -424,13 +442,13 @@ static inline int process_line(struct
selabel_handle *rec,
> > > >         spec_arr = data->spec_arr;
> > > >
> > > >         /* process and store the specification in spec. */
> > > > -       spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
> > > > -       spec_arr[nspec].regex_str = regex;
> > > > +       spec_arr[nspec].stem_id = find_stem_from_spec(data,
found.regex);
> > > > +       spec_arr[nspec].regex_str = found.regex;
> > > >
> > > > -       spec_arr[nspec].type_str = type;
> > > > +       spec_arr[nspec].type_str = found.type;
> > > >         spec_arr[nspec].mode = 0;
> > > >
> > > > -       spec_arr[nspec].lr.ctx_raw = context;
> > > > +       spec_arr[nspec].lr.ctx_raw = found.context;
> > > >
> > > >         /*
> > > >          * bump data->nspecs to cause closef() to cover it in its
free
> > > > @@ -442,18 +460,18 @@ static inline int process_line(struct
selabel_handle *rec,
> > > >                         && compile_regex(data, &spec_arr[nspec],
&errbuf)) {
> > > >                 COMPAT_LOG(SELINUX_ERROR,
> > > >                            "%s:  line %u has invalid regex %s:
%s\n",
> > > > -                          path, lineno, regex, errbuf);
> > > > +                          path, lineno, found.regex, errbuf);
> > > >                 errno = EINVAL;
> > > >                 return -1;
> > > >         }
> > > >
> > > > -       if (type) {
> > > > -               mode_t mode = string_to_mode(type);
> > > > +       if (found.type) {
> > > > +               mode_t mode = string_to_mode(found.type);
> > > >
> > > >                 if (mode == (mode_t)-1) {
> > > >                         COMPAT_LOG(SELINUX_ERROR,
> > > >                                    "%s:  line %u has invalid file
type %s\n",
> > > > -                                  path, lineno, type);
> > > > +                                  path, lineno, found.type);
> > > >                         errno = EINVAL;
> > > >                         return -1;
> > > >                 }
> > > > @@ -464,7 +482,7 @@ static inline int process_line(struct
selabel_handle *rec,
> > > >          * any meta characters in the RE */
> > > >         spec_hasMetaChars(&spec_arr[nspec]);
> > > >
> > > > -       if (strcmp(context, "<<none>>") && rec->validating)
> > > > +       if (strcmp(found.context, "<<none>>") && rec->validating)
> > > >                 compat_validate(rec, &spec_arr[nspec].lr, path,
lineno);
> > > >
> > > >         return 0;
> > > > diff --git a/libselinux/src/label_internal.h
b/libselinux/src/label_internal.h
> > > > index aa48fff..e40d68b 100644
> > > > --- a/libselinux/src/label_internal.h
> > > > +++ b/libselinux/src/label_internal.h
> > > > @@ -136,10 +136,11 @@ compat_validate(struct selabel_handle *rec,
> > > >                 struct selabel_lookup_rec *contexts,
> > > >                 const char *path, unsigned lineno) hidden;
> > > >
> > > > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > > >  /*
> > > >   * The read_spec_entries function may be used to
> > > >   * replace sscanf to read entries from spec files.
> > > >   */
> > > > -extern int read_spec_entries(char *line_buf, const char **errbuf,
int num_args, ...);
> > > > +extern int hidden read_spec_entries(char *line_buf, const char
**errbuf, unsigned int num, char *found[]);
> > > >
> > > >  #endif                         /* _SELABEL_INTERNAL_H_ */
> > > > diff --git a/libselinux/src/label_support.c
b/libselinux/src/label_support.c
> > > > index 26f9ef1..3f5728d 100644
> > > > --- a/libselinux/src/label_support.c
> > > > +++ b/libselinux/src/label_support.c
> > > > @@ -17,39 +17,40 @@
> > > >   * Read an entry from a spec file (e.g. file_contexts)
> > > >   * entry - Buffer to allocate for the entry.
> > > >   * ptr - current location of the line to be processed.
> > > > - * returns  - 0 on success and *entry is set to be a null
> > > > - *            terminated value. On Error it returns -1 and
> > > > - *            errno will be set.
> > > > + * returns  - a pointer to the begining of the string.
> > > > + *            that is the
> > > >   *
> > > >   */
> > > > -static inline int read_spec_entry(char **entry, char **ptr, int
*len, const char **errbuf)
> > > > +static inline char *read_spec_entry(char **current, const char
**errbuf)
> > > >  {
> > > > -       *entry = NULL;
> > > > -       char *tmp_buf = NULL;
> > > > -
> > > > -       while (isspace(**ptr) && **ptr != '\0')
> > > > -               (*ptr)++;
> > > > +       if(!**current) {
> > > > +               (*current)++;
> > > > +               return NULL;
> > > > +       }
> > > >
> > > > -       tmp_buf = *ptr;
> > > > -       *len = 0;
> > > > +       while (isspace(**current) && **current != '\0')
> > > > +               (*current)++;
> > > >
> > > > -       while (!isspace(**ptr) && **ptr != '\0') {
> > > > -               if (!isascii(**ptr)) {
> > > > +       char *start = *current;
> > > > +       while (!isspace(**current) && **current != '\0') {
> > > > +               if (!isascii(**current)) {
> > > >                         errno = EINVAL;
> > > >                         *errbuf = "Non-ASCII characters found";
> > > > -                       return -1;
> > > > +                       return NULL;
> > > >                 }
> > > > -               (*ptr)++;
> > > > -               (*len)++;
> > > > +               (*current)++;
> > > >         }
> > > >
> > > > -       if (*len) {
> > > > -               *entry = strndup(tmp_buf, *len);
> > > > -               if (!*entry)
> > > > -                       return -1;
> > > > -       }
> > > > +       char *end = *current;
> > > >
> > > > -       return 0;
> > > > +       if (start == end)
> > > > +               return NULL;
> > > > +
> > > > +       if (*end != '\0') {
> > > > +               *end = '\0';
> > > > +               (*current)++;
> > > > +       }
> > > > +       return start;
> > > >  }
> > > >
> > > >  /*
> > > > @@ -63,15 +64,13 @@ static inline int read_spec_entry(char **entry,
char **ptr, int *len, const char
> > > >   * This function calls read_spec_entry() to do the actual string
processing.
> > > >   * As such, can return anything from that function as well.
> > > >   */
> > > > -int hidden read_spec_entries(char *line_buf, const char **errbuf,
int num_args, ...)
> > > > +int hidden read_spec_entries(char *line_buf, const char **errbuf,
unsigned int num, char *found[])
> > > >  {
> > > > -       char **spec_entry, *buf_p;
> > > > -       int len, rc, items, entry_len = 0;
> > > > -       va_list ap;
> > > > -
> > > > +       char*buf_p;
> > > > +       unsigned int items = 0;
> > > >         *errbuf = NULL;
> > > >
> > > > -       len = strlen(line_buf);
> > > > +       size_t len = strlen(line_buf);
> > > >         if (line_buf[len - 1] == '\n')
> > > >                 line_buf[len - 1] = '\0';
> > > >         else
> > > > @@ -89,26 +88,12 @@ int hidden read_spec_entries(char *line_buf,
const char **errbuf, int num_args,
> > > >                 return 0;
> > > >
> > > >         /* Process the spec file entries */
> > > > -       va_start(ap, num_args);
> > > > -
> > > > -       items = 0;
> > > > -       while (items < num_args) {
> > > > -               spec_entry = va_arg(ap, char **);
> > > > -
> > > > -               if (len - 1 == buf_p - line_buf) {
> > > > -                       va_end(ap);
> > > > -                       return items;
> > > > -               }
> > > > -
> > > > -               rc = read_spec_entry(spec_entry, &buf_p,
&entry_len, errbuf);
> > > > -               if (rc < 0) {
> > > > -                       va_end(ap);
> > > > -                       return rc;
> > > > -               }
> > > > -               if (entry_len)
> > > > -                       items++;
> > > > +       while (items < num) {
> > > > +               found[items] = read_spec_entry(&buf_p, errbuf);
> > > > +               if (!found[items])
> > > > +                       break;
> > > > +               items++;
> > > >         }
> > > > -       va_end(ap);
> > > >         return items;
> > > >  }
> > > >
> > > > --
> > > > 1.9.1
> > > >
> > > > _______________________________________________
> > > > Selinux mailing list
> > > > [e-mail]
> > > > To unsubscribe, send email to [e-mail].
> > > > To get help, send an email containing "help" to
[e-mail].
> > >
> > >
> > > _______________________________________________
> > > Selinux mailing list
> > > [e-mail]
> > > To unsubscribe, send email to [e-mail].
> > > To get help, send an email containing "help" to
[e-mail].

Stephen Smalley Sept. 20, 2016, 12:47 p.m. UTC | #6

On 09/20/2016 02:27 AM, William Roberts wrote:
> On Sep 19, 2016 22:25, "Jason Zaman" <[e-mail]> wrote:
>>
>> On 20 Sep 2016 12:50 pm, "William Roberts" <[e-mail]>
> wrote:
>>>
>>> On Sep 19, 2016 21:16, "Jason Zaman" <[e-mail]> wrote:
>>>>
>>>> On 20 Sep 2016 5:47 am, <[e-mail]> wrote:
>>>>>
>>>>> From: William Roberts <[e-mail]>
>>>>>
>>>>> THIS IS WIP...
>>>>>
>>>>> Rather than using stdio and making copies, just mmap the files
>>>>> and use the pointers in place. The affect of this change, is that
>>>>> text file load time is now faster than binary load time by 4.7%
>>>>> when testing with a file_contexts file from the Android tree. Note
>>>>> that the Android doesn't use monstrous regexs.
>>>>>
>>>>> Times are the average of 3 runs.
>>>>>
>>>>> BEFORE:
>>>>> Text file allocs: 114803
>>>>> Text file load time: 0.266101
>>>>> Bin file allocs: 93073
>>>>> Bin file load time: 0.248757667
>>>>>
>>>>> AFTER:
>>>>> Text file allocs: 103933
>>>>> Text file load time: 0.236192667
>>>>> Bin file allocs: 87645
>>>>> Bin file load time: .247607333
>>>>
>>>> Do you have the scripts that generated these stats so I can play with
> it too? These stats are from android right? Do you also have a comparison
> for refpolicy too?
>>>
>>> For generating these I used checkfc.c from the Android tree. I used
> valgrind to measure allocations and clock to measure the time in
> selabel_open().
>>
>> Okay cool I'll fetch that and give it a whirl when I get time.
>>
>>>>
>>>> I haven't looked that closely yet but just realised, will this need
> new perms because of the mmap? If it does, can you send a patch to
> refpolicy?
>>>
>>> I'm confused, mmap is not a permission, even if it was the binary path
> already was doing an mmap, so the permission would have been there. We're
> just making it so it always mmaps.
>>
>> Yeah but mmap needs execute perms sometimes (always?). I am out so just
> wanted to send an email before I forgot. If it was mmaping already then
> there is nothing to worry about :).
> 
> Mmap would only need execute if you attempted to set the prot bits to
> execute it use mprotect to change the mapping. Then things like execmod
> might come I to play if the mapping was ever writable.

The only case where mmap without PROT_EXEC would require execute would
be if the process has READ_IMPLIES_EXEC set in its personality.
Typically only for programs with the executable stack flag set.

Anyway, it is already mmap'ing file_contexts.bin so there shouldn't be
an issue here.

Stephen Smalley Sept. 20, 2016, 12:55 p.m. UTC | #7

On 09/19/2016 05:45 PM, [e-mail] wrote:
> From: William Roberts <[e-mail]>
> 
> THIS IS WIP...
> 
> Rather than using stdio and making copies, just mmap the files
> and use the pointers in place. The affect of this change, is that
> text file load time is now faster than binary load time by 4.7%
> when testing with a file_contexts file from the Android tree. Note
> that the Android doesn't use monstrous regexs.
> 
> Times are the average of 3 runs.
> 
> BEFORE:
> Text file allocs: 114803
> Text file load time: 0.266101
> Bin file allocs: 93073
> Bin file load time: 0.248757667
> 
> AFTER:
> Text file allocs: 103933
> Text file load time: 0.236192667
> Bin file allocs: 87645
> Bin file load time: .247607333
> 
> THINGS TO DO:
> 1. What's arm performance like?
> 2. What interfaces to backends are busted by this (if any)?
> 3. Test Android Properties
> 4. Im pretty sure this breaks sefcontext_compile, fix.
> 5. Test with PCRE2 enabled.
> 6. Spell check this message!
> 7. Run checkpatch
> 
> Signed-off-by: William Roberts <[e-mail]>
> ---
>  libselinux/src/label.c                  |   2 -
>  libselinux/src/label_android_property.c |  22 ++---
>  libselinux/src/label_file.c             | 140 +++++++++++++++++++-------------
>  libselinux/src/label_file.h             |  66 +++++++++------
>  libselinux/src/label_internal.h         |   3 +-
>  libselinux/src/label_support.c          |  79 ++++++++----------
>  6 files changed, 172 insertions(+), 140 deletions(-)
> 

> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> index 7156825..4dc440e 100644
> --- a/libselinux/src/label_file.c
> +++ b/libselinux/src/label_file.c
> @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data, const char *path)
>  	return rc;
>  }
>  
> -static int process_text_file(FILE *fp, const char *prefix,
> -			     struct selabel_handle *rec, const char *path)
> +static inline struct saved_data *rec_to_data(struct selabel_handle *rec)
> +{
> +	return (struct saved_data *)rec->data;
> +}
> +
> +static char *mmap_area_get_line(struct mmap_area *area)
> +{
> +	size_t len = area->next_len;
> +	if (!len)
> +		return NULL;
> +
> +	char *start = area->next_addr;
> +	char *end = memchr(start, '\n', len);
> +
> +	/* the file may not end with a newline */
> +	if (!end)
> +		end = (char *)area->next_addr + len - 1;
> +
> +	*end = '\0';

Couldn't this clobber the last character in the file if the file does
not end with a newline?

> +	/* len includes null byte */
> +	len = end - start;
> +
> +	area->next_len -= len + 1;
> +	area->next_addr = ++end;
> +	return start;
> +}
> +
> +static int process_text_file(const char *path, const char *prefix,
> +		struct selabel_handle *rec)
>  {
>  	int rc;
> -	size_t line_len;
>  	unsigned int lineno = 0;
> -	char *line_buf = NULL;
> +	char *line_buf;
> +	struct mmap_area *areas = rec_to_data(rec)->mmap_areas;
> +
> +	/* mmap_area_get_line() and process_line() require mutable string pointers */
> +	rc = mprotect(areas->addr, areas->len, PROT_READ | PROT_WRITE);

Just map it with PROT_READ|PROT_WRITE in the beginning.

Stephen Smalley Sept. 20, 2016, 1:17 p.m. UTC | #8

On 09/19/2016 05:51 PM, Roberts, William C wrote:
> FYI I only tested this with checkfc...

Evidently.  matchpathcon and sefcontext_compile both report calls to
free() on invalid pointers and abort.

> 
>> -----Original Message-----
>> From: Roberts, William C
>> Sent: Monday, September 19, 2016 2:45 PM
>> To: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]
>> Cc: Roberts, William C <[e-mail]>
>> Subject: [RFC] mmap file_contexts and property_contexts:
>>
>> From: William Roberts <[e-mail]>
>>
>> THIS IS WIP...
>>
>> Rather than using stdio and making copies, just mmap the files and use the
>> pointers in place. The affect of this change, is that text file load time is now faster
>> than binary load time by 4.7% when testing with a file_contexts file from the
>> Android tree. Note that the Android doesn't use monstrous regexs.
>>
>> Times are the average of 3 runs.
>>
>> BEFORE:
>> Text file allocs: 114803
>> Text file load time: 0.266101
>> Bin file allocs: 93073
>> Bin file load time: 0.248757667
>>
>> AFTER:
>> Text file allocs: 103933
>> Text file load time: 0.236192667
>> Bin file allocs: 87645
>> Bin file load time: .247607333
>>
>> THINGS TO DO:
>> 1. What's arm performance like?
>> 2. What interfaces to backends are busted by this (if any)?
>> 3. Test Android Properties
>> 4. Im pretty sure this breaks sefcontext_compile, fix.
>> 5. Test with PCRE2 enabled.
>> 6. Spell check this message!
>> 7. Run checkpatch
>>
>> Signed-off-by: William Roberts <[e-mail]>
>> ---
>>  libselinux/src/label.c                  |   2 -
>>  libselinux/src/label_android_property.c |  22 ++---
>>  libselinux/src/label_file.c             | 140 +++++++++++++++++++-------------
>>  libselinux/src/label_file.h             |  66 +++++++++------
>>  libselinux/src/label_internal.h         |   3 +-
>>  libselinux/src/label_support.c          |  79 ++++++++----------
>>  6 files changed, 172 insertions(+), 140 deletions(-)
>>
>> diff --git a/libselinux/src/label.c b/libselinux/src/label.c index 963bfcb..d767b49
>> 100644
>> --- a/libselinux/src/label.c
>> +++ b/libselinux/src/label.c
>> @@ -15,8 +15,6 @@
>>  #include "callbacks.h"
>>  #include "label_internal.h"
>>
>> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>> -
>>  typedef int (*selabel_initfunc)(struct selabel_handle *rec,
>>  				const struct selinux_opt *opts,
>>  				unsigned nopts);
>> diff --git a/libselinux/src/label_android_property.c
>> b/libselinux/src/label_android_property.c
>> index 290b438..2aac394 100644
>> --- a/libselinux/src/label_android_property.c
>> +++ b/libselinux/src/label_android_property.c
>> @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec,
>>  			int pass, unsigned lineno)
>>  {
>>  	int items;
>> -	char *prop = NULL, *context = NULL;
>> +	union {
>> +		struct {
>> +			char *prop;
>> +			char *context;
>> +		};
>> +		char *array[2];
>> +	} found = { .array = { 0 } };
>>  	struct saved_data *data = (struct saved_data *)rec->data;
>>  	spec_t *spec_arr = data->spec_arr;
>>  	unsigned int nspec = data->nspec;
>>  	const char *errbuf = NULL;
>>
>> -	items = read_spec_entries(line_buf, &errbuf, 2, &prop, &context);
>> +	items = read_spec_entries(line_buf, &errbuf, ARRAY_SIZE(found.array),
>> +found.array);
>>  	if (items < 0) {
>>  		items = errno;
>>  		selinux_log(SELINUX_ERROR,
>> @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle *rec,
>>  		selinux_log(SELINUX_ERROR,
>>  			    "%s:  line %u is missing fields\n", path,
>>  			    lineno);
>> -		free(prop);
>>  		errno = EINVAL;
>>  		return -1;
>>  	}
>>
>> -	if (pass == 0) {
>> -		free(prop);
>> -		free(context);
>> -	} else if (pass == 1) {
>> +	if (pass == 1) {
>>  		/* On the second pass, process and store the specification in
>> spec. */
>> -		spec_arr[nspec].property_key = prop;
>> -		spec_arr[nspec].lr.ctx_raw = context;
>> +		spec_arr[nspec].property_key = found.prop;
>> +		spec_arr[nspec].lr.ctx_raw = found.context;
>>
>>  		if (rec->validating) {
>>  			if (selabel_validate(rec, &spec_arr[nspec].lr) < 0) { @@ -
>> 234,7 +236,7 @@ static void closef(struct selabel_handle *rec)
>>  	for (i = 0; i < data->nspec; i++) {
>>  		spec = &data->spec_arr[i];
>>  		free(spec->property_key);
>> -		free(spec->lr.ctx_raw);
>> +		//free(spec->lr.ctx_raw);
>>  		free(spec->lr.ctx_trans);
>>  	}
>>
>> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c index
>> 7156825..4dc440e 100644
>> --- a/libselinux/src/label_file.c
>> +++ b/libselinux/src/label_file.c
>> @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data, const
>> char *path)
>>  	return rc;
>>  }
>>
>> -static int process_text_file(FILE *fp, const char *prefix,
>> -			     struct selabel_handle *rec, const char *path)
>> +static inline struct saved_data *rec_to_data(struct selabel_handle
>> +*rec) {
>> +	return (struct saved_data *)rec->data; }
>> +
>> +static char *mmap_area_get_line(struct mmap_area *area) {
>> +	size_t len = area->next_len;
>> +	if (!len)
>> +		return NULL;
>> +
>> +	char *start = area->next_addr;
>> +	char *end = memchr(start, '\n', len);
>> +
>> +	/* the file may not end with a newline */
>> +	if (!end)
>> +		end = (char *)area->next_addr + len - 1;
>> +
>> +	*end = '\0';
>> +	/* len includes null byte */
>> +	len = end - start;
>> +
>> +	area->next_len -= len + 1;
>> +	area->next_addr = ++end;
>> +	return start;
>> +}
>> +
>> +static int process_text_file(const char *path, const char *prefix,
>> +		struct selabel_handle *rec)
>>  {
>>  	int rc;
>> -	size_t line_len;
>>  	unsigned int lineno = 0;
>> -	char *line_buf = NULL;
>> +	char *line_buf;
>> +	struct mmap_area *areas = rec_to_data(rec)->mmap_areas;
>> +
>> +	/* mmap_area_get_line() and process_line() require mutable string
>> pointers */
>> +	rc = mprotect(areas->addr, areas->len, PROT_READ | PROT_WRITE);
>> +	if (rc < 0)
>> +		goto out;
>>
>> -	while (getline(&line_buf, &line_len, fp) > 0) {
>> +	while ( (line_buf = mmap_area_get_line(areas)) ) {
>>  		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)
>> +static int mmap_file(FILE *fp, size_t len, struct saved_data *data)
>>  {
>> -	struct saved_data *data = (struct saved_data *)rec->data;
>> -	int rc;
>> -	char *addr, *str_buf;
>> -	int *stem_map;
>> -	struct mmap_area *mmap_area;
>> -	uint32_t i, magic, version;
>> -	uint32_t entry_len, stem_map_len, regex_array_len;
>> -	const char *reg_version;
>> -
>> -	mmap_area = malloc(sizeof(*mmap_area));
>> +	struct mmap_area *mmap_area = malloc(sizeof(*mmap_area));
>>  	if (!mmap_area) {
>>  		return -1;
>>  	}
>>
>> -	addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
>> +	void *addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
>>  	if (addr == MAP_FAILED) {
>>  		free(mmap_area);
>>  		perror("mmap");
>> @@ -145,17 +166,26 @@ static int load_mmap(FILE *fp, size_t len, struct
>> selabel_handle *rec,
>>  	mmap_area->next = data->mmap_areas;
>>  	data->mmap_areas = mmap_area;
>>
>> -	/* 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)
>> -		return -1;
>> +	return 0;
>> +}
>> +
>> +static int process_binary_file(const char *path, struct selabel_handle
>> +*rec) {
>> +	struct saved_data *data = (struct saved_data *)rec->data;
>> +	int rc;
>> +	char *str_buf;
>> +	int *stem_map;
>> +	struct mmap_area *mmap_area = data->mmap_areas;
>> +	uint32_t i, version;
>> +	uint32_t entry_len, stem_map_len, regex_array_len;
>> +	size_t len;
>>
>>  	/* check if this version is higher than we understand */
>>  	rc = next_entry(&version, mmap_area, sizeof(uint32_t));
>>  	if (rc < 0 || version > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
>>  		return -1;
>>
>> -	reg_version = regex_version();
>> +	const char *reg_version = regex_version();
>>  	if (!reg_version)
>>  		return -1;
>>
>> @@ -235,12 +265,11 @@ static int load_mmap(FILE *fp, size_t len, struct
>> selabel_handle *rec,
>>  		/* store the mapping between old and new */
>>  		newid = find_stem(data, buf, stem_len);
>>  		if (newid < 0) {
>> -			newid = store_stem(data, buf, stem_len);
>> +			newid = store_stem(data, buf, stem_len, true);
>>  			if (newid < 0) {
>>  				rc = newid;
>>  				goto out;
>>  			}
>> -			data->stem_arr[newid].from_mmap = 1;
>>  		}
>>  		stem_map[i] = newid;
>>  	}
>> @@ -262,7 +291,6 @@ static int load_mmap(FILE *fp, size_t len, struct
>> selabel_handle *rec,
>>  			goto out;
>>
>>  		spec = &data->spec_arr[data->nspec];
>> -		spec->from_mmap = 1;
>>
>>  		/* Process context */
>>  		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t)); @@ -
>> 271,22 +299,10 @@ static int load_mmap(FILE *fp, size_t len, struct
>> selabel_handle *rec,
>>  			goto out;
>>  		}
>>
>> -		str_buf = malloc(entry_len);
>> -		if (!str_buf) {
>> -			rc = -1;
>> -			goto out;
>> -		}
>> -		rc = next_entry(str_buf, mmap_area, entry_len);
>> +		rc = next_pstr(&spec->lr.ctx_raw, mmap_area, entry_len);
>>  		if (rc < 0)
>>  			goto out;
>>
>> -		if (str_buf[entry_len - 1] != '\0') {
>> -			free(str_buf);
>> -			rc = -1;
>> -			goto out;
>> -		}
>> -		spec->lr.ctx_raw = str_buf;
>> -
>>  		if (strcmp(spec->lr.ctx_raw, "<<none>>") && rec->validating) {
>>  			if (selabel_validate(rec, &spec->lr) < 0) {
>>  				selinux_log(SELINUX_ERROR,
>> @@ -401,17 +417,29 @@ static char *rolling_append(char *current, const char
>> *suffix, size_t max)
>>  	return current;
>>  }
>>
>> -static bool fcontext_is_binary(FILE *fp)
>> +static inline int mmap_area_rewind(struct mmap_area *area, size_t
>> +bytes) {
>> +	if (area->next_len + bytes > area->len)
>> +		return -1;
>> +
>> +	area->next_addr = (char *) area->next_addr - bytes;
>> +	area->next_len += bytes;
>> +	return 0;
>> +}
>> +
>> +static bool fcontext_is_binary(struct mmap_area *area)
>>  {
>>  	uint32_t magic;
>> +	bool is_binary = false;
>>
>> -	size_t len = fread(&magic, sizeof(magic), 1, fp);
>> -	rewind(fp);
>> +	int rc = next_entry(&magic, area, sizeof(magic));
>>
>> -	return (len && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
>> -}
>> +	is_binary = (!rc && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
>> +	if (!is_binary)
>> +		mmap_area_rewind(area, sizeof(magic));
>>
>> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>> +	return is_binary;
>> +}
>>
>>  static FILE *open_file(const char *path, const char *suffix,
>>  	       char *save_path, size_t len, struct stat *sb, bool open_oldest) @@ -
>> 504,12 +532,17 @@ static int process_file(const char *path, const char *suffix,
>>  		if (fp == NULL)
>>  			return -1;
>>
>> -		rc = fcontext_is_binary(fp) ?
>> -				load_mmap(fp, sb.st_size, rec, found_path) :
>> -				process_text_file(fp, prefix, rec, found_path);
>> +		rc = mmap_file(fp, sb.st_size, rec_to_data(rec));
>> +		if (rc < 0) {
>> +			fclose(fp);
>> +			return -1;
>> +		}
>> +
>> +		rc = fcontext_is_binary(rec_to_data(rec)->mmap_areas) ?
>> +				process_binary_file(found_path, rec) :
>> +				process_text_file(found_path, prefix, rec);
>>  		if (!rc)
>> -			rc = digest_add_specfile(digest, fp, NULL, sb.st_size,
>> -				found_path);
>> +			rc = digest_add_specfile(digest, fp, NULL, sb.st_size,
>> found_path);
>>
>>  		fclose(fp);
>>
>> @@ -613,12 +646,7 @@ static void closef(struct selabel_handle *rec)
>>  	for (i = 0; i < data->nspec; i++) {
>>  		spec = &data->spec_arr[i];
>>  		free(spec->lr.ctx_trans);
>> -		free(spec->lr.ctx_raw);
>>  		regex_data_free(spec->regex);
>> -		if (spec->from_mmap)
>> -			continue;
>> -		free(spec->regex_str);
>> -		free(spec->type_str);
>>  	}
>>
>>  	for (i = 0; i < (unsigned int)data->num_stems; i++) { diff --git
>> a/libselinux/src/label_file.h b/libselinux/src/label_file.h index 88f4294..2704906
>> 100644
>> --- a/libselinux/src/label_file.h
>> +++ b/libselinux/src/label_file.h
>> @@ -37,7 +37,6 @@ struct spec {
>>  	int matches;		/* number of matching pathnames */
>>  	int stem_id;		/* indicates which stem-compression item */
>>  	char hasMetaChars;	/* regular expression has meta-chars */
>> -	char from_mmap;		/* this spec is from an mmap of the data
>> */
>>  	size_t prefix_len;      /* length of fixed path prefix */
>>  };
>>
>> @@ -255,7 +254,7 @@ static inline int find_stem(struct saved_data *data, const
>> char *buf,  }
>>
>>  /* returns the index of the new stored object */ -static inline int
>> store_stem(struct saved_data *data, char *buf, int stem_len)
>> +static inline int store_stem(struct saved_data *data, char *buf, int
>> +stem_len, bool from_mmap)
>>  {
>>  	int num = data->num_stems;
>>
>> @@ -271,7 +270,7 @@ static inline int store_stem(struct saved_data *data, char
>> *buf, int stem_len)
>>  	}
>>  	data->stem_arr[num].len = stem_len;
>>  	data->stem_arr[num].buf = buf;
>> -	data->stem_arr[num].from_mmap = 0;
>> +	data->stem_arr[num].from_mmap = from_mmap;
>>  	data->num_stems++;
>>
>>  	return num;
>> @@ -298,7 +297,7 @@ static inline int find_stem_from_spec(struct saved_data
>> *data, const char *buf)
>>  	if (!stem)
>>  		return -1;
>>
>> -	return store_stem(data, stem, stem_len);
>> +	return store_stem(data, stem, stem_len, false);
>>  }
>>
>>  /* This will always check for buffer over-runs and either read the next entry @@
>> -317,6 +316,22 @@ static inline int next_entry(void *buf, struct mmap_area *fp,
>> size_t bytes)
>>  	return 0;
>>  }
>>
>> +static inline int next_pstr(char **str, struct mmap_area *fp, size_t
>> +len) {
>> +	if (len > fp->next_len)
>> +		return -1;
>> +
>> +	char *tmp = (char *)fp->next_addr;
>> +	if (tmp[len-1] != '\0')
>> +		return -1;
>> +
>> +	*str = tmp;
>> +
>> +	fp->next_addr = (char *)fp->next_addr + len;
>> +	fp->next_len -= len;
>> +	return 0;
>> +}
>> +
>>  static inline int compile_regex(struct saved_data *data, struct spec *spec,
>>  					    const char **errbuf)
>>  {
>> @@ -375,13 +390,21 @@ static inline int process_line(struct selabel_handle *rec,
>>  			char *line_buf, unsigned lineno)
>>  {
>>  	int items, len, rc;
>> -	char *regex = NULL, *type = NULL, *context = NULL;
>> +	union {
>> +		struct {
>> +			char *regex;
>> +			char *type;
>> +			char *context;
>> +		};
>> +		char *array[3];
>> +	} found = { .array = { 0 } };
>> +
>>  	struct saved_data *data = (struct saved_data *)rec->data;
>>  	struct spec *spec_arr;
>>  	unsigned int nspec = data->nspec;
>>  	const char *errbuf = NULL;
>>
>> -	items = read_spec_entries(line_buf, &errbuf, 3, &regex, &type,
>> &context);
>> +	items = read_spec_entries(line_buf, &errbuf, ARRAY_SIZE(found.array),
>> +found.array);
>>  	if (items < 0) {
>>  		rc = errno;
>>  		selinux_log(SELINUX_ERROR,
>> @@ -398,22 +421,17 @@ static inline int process_line(struct selabel_handle *rec,
>>  		COMPAT_LOG(SELINUX_ERROR,
>>  			    "%s:  line %u is missing fields\n", path,
>>  			    lineno);
>> -		if (items == 1)
>> -			free(regex);
>>  		errno = EINVAL;
>>  		return -1;
>>  	} else if (items == 2) {
>>  		/* The type field is optional. */
>> -		context = type;
>> -		type = 0;
>> +		found.context = found.type;
>> +		found.type = NULL;
>>  	}
>>
>> -	len = get_stem_from_spec(regex);
>> -	if (len && prefix && strncmp(prefix, regex, len)) {
>> +	len = get_stem_from_spec(found.regex);
>> +	if (len && prefix && strncmp(prefix, found.regex, len)) {
>>  		/* Stem of regex does not match requested prefix, discard. */
>> -		free(regex);
>> -		free(type);
>> -		free(context);
>>  		return 0;
>>  	}
>>
>> @@ -424,13 +442,13 @@ static inline int process_line(struct selabel_handle *rec,
>>  	spec_arr = data->spec_arr;
>>
>>  	/* process and store the specification in spec. */
>> -	spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
>> -	spec_arr[nspec].regex_str = regex;
>> +	spec_arr[nspec].stem_id = find_stem_from_spec(data, found.regex);
>> +	spec_arr[nspec].regex_str = found.regex;
>>
>> -	spec_arr[nspec].type_str = type;
>> +	spec_arr[nspec].type_str = found.type;
>>  	spec_arr[nspec].mode = 0;
>>
>> -	spec_arr[nspec].lr.ctx_raw = context;
>> +	spec_arr[nspec].lr.ctx_raw = found.context;
>>
>>  	/*
>>  	 * bump data->nspecs to cause closef() to cover it in its free @@ -442,18
>> +460,18 @@ static inline int process_line(struct selabel_handle *rec,
>>  			&& compile_regex(data, &spec_arr[nspec], &errbuf)) {
>>  		COMPAT_LOG(SELINUX_ERROR,
>>  			   "%s:  line %u has invalid regex %s:  %s\n",
>> -			   path, lineno, regex, errbuf);
>> +			   path, lineno, found.regex, errbuf);
>>  		errno = EINVAL;
>>  		return -1;
>>  	}
>>
>> -	if (type) {
>> -		mode_t mode = string_to_mode(type);
>> +	if (found.type) {
>> +		mode_t mode = string_to_mode(found.type);
>>
>>  		if (mode == (mode_t)-1) {
>>  			COMPAT_LOG(SELINUX_ERROR,
>>  				   "%s:  line %u has invalid file type %s\n",
>> -				   path, lineno, type);
>> +				   path, lineno, found.type);
>>  			errno = EINVAL;
>>  			return -1;
>>  		}
>> @@ -464,7 +482,7 @@ static inline int process_line(struct selabel_handle *rec,
>>  	 * any meta characters in the RE */
>>  	spec_hasMetaChars(&spec_arr[nspec]);
>>
>> -	if (strcmp(context, "<<none>>") && rec->validating)
>> +	if (strcmp(found.context, "<<none>>") && rec->validating)
>>  		compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
>>
>>  	return 0;
>> diff --git a/libselinux/src/label_internal.h b/libselinux/src/label_internal.h index
>> aa48fff..e40d68b 100644
>> --- a/libselinux/src/label_internal.h
>> +++ b/libselinux/src/label_internal.h
>> @@ -136,10 +136,11 @@ compat_validate(struct selabel_handle *rec,
>>  		struct selabel_lookup_rec *contexts,
>>  		const char *path, unsigned lineno) hidden;
>>
>> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>>  /*
>>   * The read_spec_entries function may be used to
>>   * replace sscanf to read entries from spec files.
>>   */
>> -extern int read_spec_entries(char *line_buf, const char **errbuf, int num_args,
>> ...);
>> +extern int hidden read_spec_entries(char *line_buf, const char
>> +**errbuf, unsigned int num, char *found[]);
>>
>>  #endif				/* _SELABEL_INTERNAL_H_ */
>> diff --git a/libselinux/src/label_support.c b/libselinux/src/label_support.c index
>> 26f9ef1..3f5728d 100644
>> --- a/libselinux/src/label_support.c
>> +++ b/libselinux/src/label_support.c
>> @@ -17,39 +17,40 @@
>>   * Read an entry from a spec file (e.g. file_contexts)
>>   * entry - Buffer to allocate for the entry.
>>   * ptr - current location of the line to be processed.
>> - * returns  - 0 on success and *entry is set to be a null
>> - *            terminated value. On Error it returns -1 and
>> - *            errno will be set.
>> + * returns  - a pointer to the begining of the string.
>> + *            that is the
>>   *
>>   */
>> -static inline int read_spec_entry(char **entry, char **ptr, int *len, const char
>> **errbuf)
>> +static inline char *read_spec_entry(char **current, const char
>> +**errbuf)
>>  {
>> -	*entry = NULL;
>> -	char *tmp_buf = NULL;
>> -
>> -	while (isspace(**ptr) && **ptr != '\0')
>> -		(*ptr)++;
>> +	if(!**current) {
>> +		(*current)++;
>> +		return NULL;
>> +	}
>>
>> -	tmp_buf = *ptr;
>> -	*len = 0;
>> +	while (isspace(**current) && **current != '\0')
>> +		(*current)++;
>>
>> -	while (!isspace(**ptr) && **ptr != '\0') {
>> -		if (!isascii(**ptr)) {
>> +	char *start = *current;
>> +	while (!isspace(**current) && **current != '\0') {
>> +		if (!isascii(**current)) {
>>  			errno = EINVAL;
>>  			*errbuf = "Non-ASCII characters found";
>> -			return -1;
>> +			return NULL;
>>  		}
>> -		(*ptr)++;
>> -		(*len)++;
>> +		(*current)++;
>>  	}
>>
>> -	if (*len) {
>> -		*entry = strndup(tmp_buf, *len);
>> -		if (!*entry)
>> -			return -1;
>> -	}
>> +	char *end = *current;
>>
>> -	return 0;
>> +	if (start == end)
>> +		return NULL;
>> +
>> +	if (*end != '\0') {
>> +		*end = '\0';
>> +		(*current)++;
>> +	}
>> +	return start;
>>  }
>>
>>  /*
>> @@ -63,15 +64,13 @@ static inline int read_spec_entry(char **entry, char **ptr,
>> int *len, const char
>>   * This function calls read_spec_entry() to do the actual string processing.
>>   * As such, can return anything from that function as well.
>>   */
>> -int hidden read_spec_entries(char *line_buf, const char **errbuf, int num_args,
>> ...)
>> +int hidden read_spec_entries(char *line_buf, const char **errbuf,
>> +unsigned int num, char *found[])
>>  {
>> -	char **spec_entry, *buf_p;
>> -	int len, rc, items, entry_len = 0;
>> -	va_list ap;
>> -
>> +	char*buf_p;
>> +	unsigned int items = 0;
>>  	*errbuf = NULL;
>>
>> -	len = strlen(line_buf);
>> +	size_t len = strlen(line_buf);
>>  	if (line_buf[len - 1] == '\n')
>>  		line_buf[len - 1] = '\0';
>>  	else
>> @@ -89,26 +88,12 @@ int hidden read_spec_entries(char *line_buf, const char
>> **errbuf, int num_args,
>>  		return 0;
>>
>>  	/* Process the spec file entries */
>> -	va_start(ap, num_args);
>> -
>> -	items = 0;
>> -	while (items < num_args) {
>> -		spec_entry = va_arg(ap, char **);
>> -
>> -		if (len - 1 == buf_p - line_buf) {
>> -			va_end(ap);
>> -			return items;
>> -		}
>> -
>> -		rc = read_spec_entry(spec_entry, &buf_p, &entry_len, errbuf);
>> -		if (rc < 0) {
>> -			va_end(ap);
>> -			return rc;
>> -		}
>> -		if (entry_len)
>> -			items++;
>> +	while (items < num) {
>> +		found[items] = read_spec_entry(&buf_p, errbuf);
>> +		if (!found[items])
>> +			break;
>> +		items++;
>>  	}
>> -	va_end(ap);
>>  	return items;
>>  }
>>
>> --
>> 1.9.1
> 
>

Roberts, William C Sept. 20, 2016, 8:28 p.m. UTC | #9

> -----Original Message-----
> From: Stephen Smalley [mailto:[e-mail]]
> Sent: Tuesday, September 20, 2016 6:18 AM
> To: Roberts, William C <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [RFC] mmap file_contexts and property_contexts:
> 
> On 09/19/2016 05:51 PM, Roberts, William C wrote:
> > FYI I only tested this with checkfc...
> 
> Evidently.  matchpathcon and sefcontext_compile both report calls to
> free() on invalid pointers and abort.

That doesn’t surprise me, I only tested the checkfc usages. Hence #4 in todo's

> 
> >
> >> -----Original Message-----
> >> From: Roberts, William C
> >> Sent: Monday, September 19, 2016 2:45 PM
> >> To: [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> Cc: Roberts, William C <[e-mail]>
> >> Subject: [RFC] mmap file_contexts and property_contexts:
> >>
> >> From: William Roberts <[e-mail]>
> >>
> >> THIS IS WIP...
> >>
> >> Rather than using stdio and making copies, just mmap the files and
> >> use the pointers in place. The affect of this change, is that text
> >> file load time is now faster than binary load time by 4.7% when
> >> testing with a file_contexts file from the Android tree. Note that the Android
> doesn't use monstrous regexs.
> >>
> >> Times are the average of 3 runs.
> >>
> >> BEFORE:
> >> Text file allocs: 114803
> >> Text file load time: 0.266101
> >> Bin file allocs: 93073
> >> Bin file load time: 0.248757667
> >>
> >> AFTER:
> >> Text file allocs: 103933
> >> Text file load time: 0.236192667
> >> Bin file allocs: 87645
> >> Bin file load time: .247607333
> >>
> >> THINGS TO DO:
> >> 1. What's arm performance like?
> >> 2. What interfaces to backends are busted by this (if any)?
> >> 3. Test Android Properties
> >> 4. Im pretty sure this breaks sefcontext_compile, fix.
> >> 5. Test with PCRE2 enabled.
> >> 6. Spell check this message!
> >> 7. Run checkpatch
> >>
> >> Signed-off-by: William Roberts <[e-mail]>
> >> ---
> >>  libselinux/src/label.c                  |   2 -
> >>  libselinux/src/label_android_property.c |  22 ++---
> >>  libselinux/src/label_file.c             | 140 +++++++++++++++++++-------------
> >>  libselinux/src/label_file.h             |  66 +++++++++------
> >>  libselinux/src/label_internal.h         |   3 +-
> >>  libselinux/src/label_support.c          |  79 ++++++++----------
> >>  6 files changed, 172 insertions(+), 140 deletions(-)
> >>
> >> diff --git a/libselinux/src/label.c b/libselinux/src/label.c index
> >> 963bfcb..d767b49
> >> 100644
> >> --- a/libselinux/src/label.c
> >> +++ b/libselinux/src/label.c
> >> @@ -15,8 +15,6 @@
> >>  #include "callbacks.h"
> >>  #include "label_internal.h"
> >>
> >> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> >> -
> >>  typedef int (*selabel_initfunc)(struct selabel_handle *rec,
> >>  				const struct selinux_opt *opts,
> >>  				unsigned nopts);
> >> diff --git a/libselinux/src/label_android_property.c
> >> b/libselinux/src/label_android_property.c
> >> index 290b438..2aac394 100644
> >> --- a/libselinux/src/label_android_property.c
> >> +++ b/libselinux/src/label_android_property.c
> >> @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec,
> >>  			int pass, unsigned lineno)
> >>  {
> >>  	int items;
> >> -	char *prop = NULL, *context = NULL;
> >> +	union {
> >> +		struct {
> >> +			char *prop;
> >> +			char *context;
> >> +		};
> >> +		char *array[2];
> >> +	} found = { .array = { 0 } };
> >>  	struct saved_data *data = (struct saved_data *)rec->data;
> >>  	spec_t *spec_arr = data->spec_arr;
> >>  	unsigned int nspec = data->nspec;
> >>  	const char *errbuf = NULL;
> >>
> >> -	items = read_spec_entries(line_buf, &errbuf, 2, &prop, &context);
> >> +	items = read_spec_entries(line_buf, &errbuf,
> >> +ARRAY_SIZE(found.array), found.array);
> >>  	if (items < 0) {
> >>  		items = errno;
> >>  		selinux_log(SELINUX_ERROR,
> >> @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle *rec,
> >>  		selinux_log(SELINUX_ERROR,
> >>  			    "%s:  line %u is missing fields\n", path,
> >>  			    lineno);
> >> -		free(prop);
> >>  		errno = EINVAL;
> >>  		return -1;
> >>  	}
> >>
> >> -	if (pass == 0) {
> >> -		free(prop);
> >> -		free(context);
> >> -	} else if (pass == 1) {
> >> +	if (pass == 1) {
> >>  		/* On the second pass, process and store the specification in
> >> spec. */
> >> -		spec_arr[nspec].property_key = prop;
> >> -		spec_arr[nspec].lr.ctx_raw = context;
> >> +		spec_arr[nspec].property_key = found.prop;
> >> +		spec_arr[nspec].lr.ctx_raw = found.context;
> >>
> >>  		if (rec->validating) {
> >>  			if (selabel_validate(rec, &spec_arr[nspec].lr) < 0) { @@ -
> >> 234,7 +236,7 @@ static void closef(struct selabel_handle *rec)
> >>  	for (i = 0; i < data->nspec; i++) {
> >>  		spec = &data->spec_arr[i];
> >>  		free(spec->property_key);
> >> -		free(spec->lr.ctx_raw);
> >> +		//free(spec->lr.ctx_raw);
> >>  		free(spec->lr.ctx_trans);
> >>  	}
> >>
> >> diff --git a/libselinux/src/label_file.c
> >> b/libselinux/src/label_file.c index 7156825..4dc440e 100644
> >> --- a/libselinux/src/label_file.c
> >> +++ b/libselinux/src/label_file.c
> >> @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data,
> >> const char *path)
> >>  	return rc;
> >>  }
> >>
> >> -static int process_text_file(FILE *fp, const char *prefix,
> >> -			     struct selabel_handle *rec, const char *path)
> >> +static inline struct saved_data *rec_to_data(struct selabel_handle
> >> +*rec) {
> >> +	return (struct saved_data *)rec->data; }
> >> +
> >> +static char *mmap_area_get_line(struct mmap_area *area) {
> >> +	size_t len = area->next_len;
> >> +	if (!len)
> >> +		return NULL;
> >> +
> >> +	char *start = area->next_addr;
> >> +	char *end = memchr(start, '\n', len);
> >> +
> >> +	/* the file may not end with a newline */
> >> +	if (!end)
> >> +		end = (char *)area->next_addr + len - 1;
> >> +
> >> +	*end = '\0';
> >> +	/* len includes null byte */
> >> +	len = end - start;
> >> +
> >> +	area->next_len -= len + 1;
> >> +	area->next_addr = ++end;
> >> +	return start;
> >> +}
> >> +
> >> +static int process_text_file(const char *path, const char *prefix,
> >> +		struct selabel_handle *rec)
> >>  {
> >>  	int rc;
> >> -	size_t line_len;
> >>  	unsigned int lineno = 0;
> >> -	char *line_buf = NULL;
> >> +	char *line_buf;
> >> +	struct mmap_area *areas = rec_to_data(rec)->mmap_areas;
> >> +
> >> +	/* mmap_area_get_line() and process_line() require mutable string
> >> pointers */
> >> +	rc = mprotect(areas->addr, areas->len, PROT_READ | PROT_WRITE);
> >> +	if (rc < 0)
> >> +		goto out;
> >>
> >> -	while (getline(&line_buf, &line_len, fp) > 0) {
> >> +	while ( (line_buf = mmap_area_get_line(areas)) ) {
> >>  		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)
> >> +static int mmap_file(FILE *fp, size_t len, struct saved_data *data)
> >>  {
> >> -	struct saved_data *data = (struct saved_data *)rec->data;
> >> -	int rc;
> >> -	char *addr, *str_buf;
> >> -	int *stem_map;
> >> -	struct mmap_area *mmap_area;
> >> -	uint32_t i, magic, version;
> >> -	uint32_t entry_len, stem_map_len, regex_array_len;
> >> -	const char *reg_version;
> >> -
> >> -	mmap_area = malloc(sizeof(*mmap_area));
> >> +	struct mmap_area *mmap_area = malloc(sizeof(*mmap_area));
> >>  	if (!mmap_area) {
> >>  		return -1;
> >>  	}
> >>
> >> -	addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
> >> +	void *addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp),
> >> +0);
> >>  	if (addr == MAP_FAILED) {
> >>  		free(mmap_area);
> >>  		perror("mmap");
> >> @@ -145,17 +166,26 @@ static int load_mmap(FILE *fp, size_t len,
> >> struct selabel_handle *rec,
> >>  	mmap_area->next = data->mmap_areas;
> >>  	data->mmap_areas = mmap_area;
> >>
> >> -	/* 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)
> >> -		return -1;
> >> +	return 0;
> >> +}
> >> +
> >> +static int process_binary_file(const char *path, struct
> >> +selabel_handle
> >> +*rec) {
> >> +	struct saved_data *data = (struct saved_data *)rec->data;
> >> +	int rc;
> >> +	char *str_buf;
> >> +	int *stem_map;
> >> +	struct mmap_area *mmap_area = data->mmap_areas;
> >> +	uint32_t i, version;
> >> +	uint32_t entry_len, stem_map_len, regex_array_len;
> >> +	size_t len;
> >>
> >>  	/* check if this version is higher than we understand */
> >>  	rc = next_entry(&version, mmap_area, sizeof(uint32_t));
> >>  	if (rc < 0 || version > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
> >>  		return -1;
> >>
> >> -	reg_version = regex_version();
> >> +	const char *reg_version = regex_version();
> >>  	if (!reg_version)
> >>  		return -1;
> >>
> >> @@ -235,12 +265,11 @@ static int load_mmap(FILE *fp, size_t len,
> >> struct selabel_handle *rec,
> >>  		/* store the mapping between old and new */
> >>  		newid = find_stem(data, buf, stem_len);
> >>  		if (newid < 0) {
> >> -			newid = store_stem(data, buf, stem_len);
> >> +			newid = store_stem(data, buf, stem_len, true);
> >>  			if (newid < 0) {
> >>  				rc = newid;
> >>  				goto out;
> >>  			}
> >> -			data->stem_arr[newid].from_mmap = 1;
> >>  		}
> >>  		stem_map[i] = newid;
> >>  	}
> >> @@ -262,7 +291,6 @@ static int load_mmap(FILE *fp, size_t len, struct
> >> selabel_handle *rec,
> >>  			goto out;
> >>
> >>  		spec = &data->spec_arr[data->nspec];
> >> -		spec->from_mmap = 1;
> >>
> >>  		/* Process context */
> >>  		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t)); @@ -
> >> 271,22 +299,10 @@ static int load_mmap(FILE *fp, size_t len, struct
> >> selabel_handle *rec,
> >>  			goto out;
> >>  		}
> >>
> >> -		str_buf = malloc(entry_len);
> >> -		if (!str_buf) {
> >> -			rc = -1;
> >> -			goto out;
> >> -		}
> >> -		rc = next_entry(str_buf, mmap_area, entry_len);
> >> +		rc = next_pstr(&spec->lr.ctx_raw, mmap_area, entry_len);
> >>  		if (rc < 0)
> >>  			goto out;
> >>
> >> -		if (str_buf[entry_len - 1] != '\0') {
> >> -			free(str_buf);
> >> -			rc = -1;
> >> -			goto out;
> >> -		}
> >> -		spec->lr.ctx_raw = str_buf;
> >> -
> >>  		if (strcmp(spec->lr.ctx_raw, "<<none>>") && rec->validating) {
> >>  			if (selabel_validate(rec, &spec->lr) < 0) {
> >>  				selinux_log(SELINUX_ERROR,
> >> @@ -401,17 +417,29 @@ static char *rolling_append(char *current,
> >> const char *suffix, size_t max)
> >>  	return current;
> >>  }
> >>
> >> -static bool fcontext_is_binary(FILE *fp)
> >> +static inline int mmap_area_rewind(struct mmap_area *area, size_t
> >> +bytes) {
> >> +	if (area->next_len + bytes > area->len)
> >> +		return -1;
> >> +
> >> +	area->next_addr = (char *) area->next_addr - bytes;
> >> +	area->next_len += bytes;
> >> +	return 0;
> >> +}
> >> +
> >> +static bool fcontext_is_binary(struct mmap_area *area)
> >>  {
> >>  	uint32_t magic;
> >> +	bool is_binary = false;
> >>
> >> -	size_t len = fread(&magic, sizeof(magic), 1, fp);
> >> -	rewind(fp);
> >> +	int rc = next_entry(&magic, area, sizeof(magic));
> >>
> >> -	return (len && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
> >> -}
> >> +	is_binary = (!rc && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
> >> +	if (!is_binary)
> >> +		mmap_area_rewind(area, sizeof(magic));
> >>
> >> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> >> +	return is_binary;
> >> +}
> >>
> >>  static FILE *open_file(const char *path, const char *suffix,
> >>  	       char *save_path, size_t len, struct stat *sb, bool
> >> open_oldest) @@ -
> >> 504,12 +532,17 @@ static int process_file(const char *path, const char *suffix,
> >>  		if (fp == NULL)
> >>  			return -1;
> >>
> >> -		rc = fcontext_is_binary(fp) ?
> >> -				load_mmap(fp, sb.st_size, rec, found_path) :
> >> -				process_text_file(fp, prefix, rec, found_path);
> >> +		rc = mmap_file(fp, sb.st_size, rec_to_data(rec));
> >> +		if (rc < 0) {
> >> +			fclose(fp);
> >> +			return -1;
> >> +		}
> >> +
> >> +		rc = fcontext_is_binary(rec_to_data(rec)->mmap_areas) ?
> >> +				process_binary_file(found_path, rec) :
> >> +				process_text_file(found_path, prefix, rec);
> >>  		if (!rc)
> >> -			rc = digest_add_specfile(digest, fp, NULL, sb.st_size,
> >> -				found_path);
> >> +			rc = digest_add_specfile(digest, fp, NULL, sb.st_size,
> >> found_path);
> >>
> >>  		fclose(fp);
> >>
> >> @@ -613,12 +646,7 @@ static void closef(struct selabel_handle *rec)
> >>  	for (i = 0; i < data->nspec; i++) {
> >>  		spec = &data->spec_arr[i];
> >>  		free(spec->lr.ctx_trans);
> >> -		free(spec->lr.ctx_raw);
> >>  		regex_data_free(spec->regex);
> >> -		if (spec->from_mmap)
> >> -			continue;
> >> -		free(spec->regex_str);
> >> -		free(spec->type_str);
> >>  	}
> >>
> >>  	for (i = 0; i < (unsigned int)data->num_stems; i++) { diff --git
> >> a/libselinux/src/label_file.h b/libselinux/src/label_file.h index
> >> 88f4294..2704906
> >> 100644
> >> --- a/libselinux/src/label_file.h
> >> +++ b/libselinux/src/label_file.h
> >> @@ -37,7 +37,6 @@ struct spec {
> >>  	int matches;		/* number of matching pathnames */
> >>  	int stem_id;		/* indicates which stem-compression item */
> >>  	char hasMetaChars;	/* regular expression has meta-chars */
> >> -	char from_mmap;		/* this spec is from an mmap of the data
> >> */
> >>  	size_t prefix_len;      /* length of fixed path prefix */
> >>  };
> >>
> >> @@ -255,7 +254,7 @@ static inline int find_stem(struct saved_data
> >> *data, const char *buf,  }
> >>
> >>  /* returns the index of the new stored object */ -static inline int
> >> store_stem(struct saved_data *data, char *buf, int stem_len)
> >> +static inline int store_stem(struct saved_data *data, char *buf, int
> >> +stem_len, bool from_mmap)
> >>  {
> >>  	int num = data->num_stems;
> >>
> >> @@ -271,7 +270,7 @@ static inline int store_stem(struct saved_data
> >> *data, char *buf, int stem_len)
> >>  	}
> >>  	data->stem_arr[num].len = stem_len;
> >>  	data->stem_arr[num].buf = buf;
> >> -	data->stem_arr[num].from_mmap = 0;
> >> +	data->stem_arr[num].from_mmap = from_mmap;
> >>  	data->num_stems++;
> >>
> >>  	return num;
> >> @@ -298,7 +297,7 @@ static inline int find_stem_from_spec(struct
> >> saved_data *data, const char *buf)
> >>  	if (!stem)
> >>  		return -1;
> >>
> >> -	return store_stem(data, stem, stem_len);
> >> +	return store_stem(data, stem, stem_len, false);
> >>  }
> >>
> >>  /* This will always check for buffer over-runs and either read the
> >> next entry @@
> >> -317,6 +316,22 @@ static inline int next_entry(void *buf, struct
> >> mmap_area *fp, size_t bytes)
> >>  	return 0;
> >>  }
> >>
> >> +static inline int next_pstr(char **str, struct mmap_area *fp, size_t
> >> +len) {
> >> +	if (len > fp->next_len)
> >> +		return -1;
> >> +
> >> +	char *tmp = (char *)fp->next_addr;
> >> +	if (tmp[len-1] != '\0')
> >> +		return -1;
> >> +
> >> +	*str = tmp;
> >> +
> >> +	fp->next_addr = (char *)fp->next_addr + len;
> >> +	fp->next_len -= len;
> >> +	return 0;
> >> +}
> >> +
> >>  static inline int compile_regex(struct saved_data *data, struct spec *spec,
> >>  					    const char **errbuf)
> >>  {
> >> @@ -375,13 +390,21 @@ static inline int process_line(struct selabel_handle
> *rec,
> >>  			char *line_buf, unsigned lineno)
> >>  {
> >>  	int items, len, rc;
> >> -	char *regex = NULL, *type = NULL, *context = NULL;
> >> +	union {
> >> +		struct {
> >> +			char *regex;
> >> +			char *type;
> >> +			char *context;
> >> +		};
> >> +		char *array[3];
> >> +	} found = { .array = { 0 } };
> >> +
> >>  	struct saved_data *data = (struct saved_data *)rec->data;
> >>  	struct spec *spec_arr;
> >>  	unsigned int nspec = data->nspec;
> >>  	const char *errbuf = NULL;
> >>
> >> -	items = read_spec_entries(line_buf, &errbuf, 3, &regex, &type,
> >> &context);
> >> +	items = read_spec_entries(line_buf, &errbuf,
> >> +ARRAY_SIZE(found.array), found.array);
> >>  	if (items < 0) {
> >>  		rc = errno;
> >>  		selinux_log(SELINUX_ERROR,
> >> @@ -398,22 +421,17 @@ static inline int process_line(struct selabel_handle
> *rec,
> >>  		COMPAT_LOG(SELINUX_ERROR,
> >>  			    "%s:  line %u is missing fields\n", path,
> >>  			    lineno);
> >> -		if (items == 1)
> >> -			free(regex);
> >>  		errno = EINVAL;
> >>  		return -1;
> >>  	} else if (items == 2) {
> >>  		/* The type field is optional. */
> >> -		context = type;
> >> -		type = 0;
> >> +		found.context = found.type;
> >> +		found.type = NULL;
> >>  	}
> >>
> >> -	len = get_stem_from_spec(regex);
> >> -	if (len && prefix && strncmp(prefix, regex, len)) {
> >> +	len = get_stem_from_spec(found.regex);
> >> +	if (len && prefix && strncmp(prefix, found.regex, len)) {
> >>  		/* Stem of regex does not match requested prefix, discard. */
> >> -		free(regex);
> >> -		free(type);
> >> -		free(context);
> >>  		return 0;
> >>  	}
> >>
> >> @@ -424,13 +442,13 @@ static inline int process_line(struct selabel_handle
> *rec,
> >>  	spec_arr = data->spec_arr;
> >>
> >>  	/* process and store the specification in spec. */
> >> -	spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
> >> -	spec_arr[nspec].regex_str = regex;
> >> +	spec_arr[nspec].stem_id = find_stem_from_spec(data, found.regex);
> >> +	spec_arr[nspec].regex_str = found.regex;
> >>
> >> -	spec_arr[nspec].type_str = type;
> >> +	spec_arr[nspec].type_str = found.type;
> >>  	spec_arr[nspec].mode = 0;
> >>
> >> -	spec_arr[nspec].lr.ctx_raw = context;
> >> +	spec_arr[nspec].lr.ctx_raw = found.context;
> >>
> >>  	/*
> >>  	 * bump data->nspecs to cause closef() to cover it in its free @@
> >> -442,18
> >> +460,18 @@ static inline int process_line(struct selabel_handle *rec,
> >>  			&& compile_regex(data, &spec_arr[nspec], &errbuf)) {
> >>  		COMPAT_LOG(SELINUX_ERROR,
> >>  			   "%s:  line %u has invalid regex %s:  %s\n",
> >> -			   path, lineno, regex, errbuf);
> >> +			   path, lineno, found.regex, errbuf);
> >>  		errno = EINVAL;
> >>  		return -1;
> >>  	}
> >>
> >> -	if (type) {
> >> -		mode_t mode = string_to_mode(type);
> >> +	if (found.type) {
> >> +		mode_t mode = string_to_mode(found.type);
> >>
> >>  		if (mode == (mode_t)-1) {
> >>  			COMPAT_LOG(SELINUX_ERROR,
> >>  				   "%s:  line %u has invalid file type %s\n",
> >> -				   path, lineno, type);
> >> +				   path, lineno, found.type);
> >>  			errno = EINVAL;
> >>  			return -1;
> >>  		}
> >> @@ -464,7 +482,7 @@ static inline int process_line(struct selabel_handle
> *rec,
> >>  	 * any meta characters in the RE */
> >>  	spec_hasMetaChars(&spec_arr[nspec]);
> >>
> >> -	if (strcmp(context, "<<none>>") && rec->validating)
> >> +	if (strcmp(found.context, "<<none>>") && rec->validating)
> >>  		compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
> >>
> >>  	return 0;
> >> diff --git a/libselinux/src/label_internal.h
> >> b/libselinux/src/label_internal.h index aa48fff..e40d68b 100644
> >> --- a/libselinux/src/label_internal.h
> >> +++ b/libselinux/src/label_internal.h
> >> @@ -136,10 +136,11 @@ compat_validate(struct selabel_handle *rec,
> >>  		struct selabel_lookup_rec *contexts,
> >>  		const char *path, unsigned lineno) hidden;
> >>
> >> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> >>  /*
> >>   * The read_spec_entries function may be used to
> >>   * replace sscanf to read entries from spec files.
> >>   */
> >> -extern int read_spec_entries(char *line_buf, const char **errbuf,
> >> int num_args, ...);
> >> +extern int hidden read_spec_entries(char *line_buf, const char
> >> +**errbuf, unsigned int num, char *found[]);
> >>
> >>  #endif				/* _SELABEL_INTERNAL_H_ */
> >> diff --git a/libselinux/src/label_support.c
> >> b/libselinux/src/label_support.c index 26f9ef1..3f5728d 100644
> >> --- a/libselinux/src/label_support.c
> >> +++ b/libselinux/src/label_support.c
> >> @@ -17,39 +17,40 @@
> >>   * Read an entry from a spec file (e.g. file_contexts)
> >>   * entry - Buffer to allocate for the entry.
> >>   * ptr - current location of the line to be processed.
> >> - * returns  - 0 on success and *entry is set to be a null
> >> - *            terminated value. On Error it returns -1 and
> >> - *            errno will be set.
> >> + * returns  - a pointer to the begining of the string.
> >> + *            that is the
> >>   *
> >>   */
> >> -static inline int read_spec_entry(char **entry, char **ptr, int
> >> *len, const char
> >> **errbuf)
> >> +static inline char *read_spec_entry(char **current, const char
> >> +**errbuf)
> >>  {
> >> -	*entry = NULL;
> >> -	char *tmp_buf = NULL;
> >> -
> >> -	while (isspace(**ptr) && **ptr != '\0')
> >> -		(*ptr)++;
> >> +	if(!**current) {
> >> +		(*current)++;
> >> +		return NULL;
> >> +	}
> >>
> >> -	tmp_buf = *ptr;
> >> -	*len = 0;
> >> +	while (isspace(**current) && **current != '\0')
> >> +		(*current)++;
> >>
> >> -	while (!isspace(**ptr) && **ptr != '\0') {
> >> -		if (!isascii(**ptr)) {
> >> +	char *start = *current;
> >> +	while (!isspace(**current) && **current != '\0') {
> >> +		if (!isascii(**current)) {
> >>  			errno = EINVAL;
> >>  			*errbuf = "Non-ASCII characters found";
> >> -			return -1;
> >> +			return NULL;
> >>  		}
> >> -		(*ptr)++;
> >> -		(*len)++;
> >> +		(*current)++;
> >>  	}
> >>
> >> -	if (*len) {
> >> -		*entry = strndup(tmp_buf, *len);
> >> -		if (!*entry)
> >> -			return -1;
> >> -	}
> >> +	char *end = *current;
> >>
> >> -	return 0;
> >> +	if (start == end)
> >> +		return NULL;
> >> +
> >> +	if (*end != '\0') {
> >> +		*end = '\0';
> >> +		(*current)++;
> >> +	}
> >> +	return start;
> >>  }
> >>
> >>  /*
> >> @@ -63,15 +64,13 @@ static inline int read_spec_entry(char **entry,
> >> char **ptr, int *len, const char
> >>   * This function calls read_spec_entry() to do the actual string processing.
> >>   * As such, can return anything from that function as well.
> >>   */
> >> -int hidden read_spec_entries(char *line_buf, const char **errbuf,
> >> int num_args,
> >> ...)
> >> +int hidden read_spec_entries(char *line_buf, const char **errbuf,
> >> +unsigned int num, char *found[])
> >>  {
> >> -	char **spec_entry, *buf_p;
> >> -	int len, rc, items, entry_len = 0;
> >> -	va_list ap;
> >> -
> >> +	char*buf_p;
> >> +	unsigned int items = 0;
> >>  	*errbuf = NULL;
> >>
> >> -	len = strlen(line_buf);
> >> +	size_t len = strlen(line_buf);
> >>  	if (line_buf[len - 1] == '\n')
> >>  		line_buf[len - 1] = '\0';
> >>  	else
> >> @@ -89,26 +88,12 @@ int hidden read_spec_entries(char *line_buf,
> >> const char **errbuf, int num_args,
> >>  		return 0;
> >>
> >>  	/* Process the spec file entries */
> >> -	va_start(ap, num_args);
> >> -
> >> -	items = 0;
> >> -	while (items < num_args) {
> >> -		spec_entry = va_arg(ap, char **);
> >> -
> >> -		if (len - 1 == buf_p - line_buf) {
> >> -			va_end(ap);
> >> -			return items;
> >> -		}
> >> -
> >> -		rc = read_spec_entry(spec_entry, &buf_p, &entry_len, errbuf);
> >> -		if (rc < 0) {
> >> -			va_end(ap);
> >> -			return rc;
> >> -		}
> >> -		if (entry_len)
> >> -			items++;
> >> +	while (items < num) {
> >> +		found[items] = read_spec_entry(&buf_p, errbuf);
> >> +		if (!found[items])
> >> +			break;
> >> +		items++;
> >>  	}
> >> -	va_end(ap);
> >>  	return items;
> >>  }
> >>
> >> --
> >> 1.9.1
> >
> >

Roberts, William C Sept. 20, 2016, 8:36 p.m. UTC | #10

> -----Original Message-----
> From: Stephen Smalley [mailto:[e-mail]]
> Sent: Tuesday, September 20, 2016 5:56 AM
> To: Roberts, William C <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [RFC] mmap file_contexts and property_contexts:
> 
> On 09/19/2016 05:45 PM, [e-mail] wrote:
> > From: William Roberts <[e-mail]>
> >
> > THIS IS WIP...
> >
> > Rather than using stdio and making copies, just mmap the files and use
> > the pointers in place. The affect of this change, is that text file

s/affect/effect

> > load time is now faster than binary load time by 4.7% when testing
> > with a file_contexts file from the Android tree. Note that the Android
> > doesn't use monstrous regexs.
> >
> > Times are the average of 3 runs.
> >
> > BEFORE:
> > Text file allocs: 114803
> > Text file load time: 0.266101
> > Bin file allocs: 93073
> > Bin file load time: 0.248757667
> >
> > AFTER:
> > Text file allocs: 103933
> > Text file load time: 0.236192667
> > Bin file allocs: 87645
> > Bin file load time: .247607333
> >
> > THINGS TO DO:
> > 1. What's arm performance like?
> > 2. What interfaces to backends are busted by this (if any)?
> > 3. Test Android Properties
> > 4. Im pretty sure this breaks sefcontext_compile, fix.
> > 5. Test with PCRE2 enabled.
> > 6. Spell check this message!
> > 7. Run checkpatch
> >
> > Signed-off-by: William Roberts <[e-mail]>
> > ---
> >  libselinux/src/label.c                  |   2 -
> >  libselinux/src/label_android_property.c |  22 ++---
> >  libselinux/src/label_file.c             | 140 +++++++++++++++++++-------------
> >  libselinux/src/label_file.h             |  66 +++++++++------
> >  libselinux/src/label_internal.h         |   3 +-
> >  libselinux/src/label_support.c          |  79 ++++++++----------
> >  6 files changed, 172 insertions(+), 140 deletions(-)
> >
> 
> > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > index 7156825..4dc440e 100644
> > --- a/libselinux/src/label_file.c
> > +++ b/libselinux/src/label_file.c
> > @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data, const
> char *path)
> >  	return rc;
> >  }
> >
> > -static int process_text_file(FILE *fp, const char *prefix,
> > -			     struct selabel_handle *rec, const char *path)
> > +static inline struct saved_data *rec_to_data(struct selabel_handle
> > +*rec) {
> > +	return (struct saved_data *)rec->data; }
> > +
> > +static char *mmap_area_get_line(struct mmap_area *area) {
> > +	size_t len = area->next_len;
> > +	if (!len)
> > +		return NULL;
> > +
> > +	char *start = area->next_addr;
> > +	char *end = memchr(start, '\n', len);
> > +
> > +	/* the file may not end with a newline */
> > +	if (!end)
> > +		end = (char *)area->next_addr + len - 1;
> > +
> > +	*end = '\0';
> 
> Couldn't this clobber the last character in the file if the file does not end with a
> newline?

Yeah I guess I really can't handle this case without increasing the mapping size or allocating
A new buffer to copy into. Considering, that the only time a file not ending with a newline
was my own stupid mistake, I am not too worried about the not ending newline, especially
considering that the Android build process handles it if it's missing.

Perhaps I could always add a terminating \0 to the mapping by growing it by one during mmap
And explicitly setting the byte.

> 
> > +	/* len includes null byte */
> > +	len = end - start;
> > +
> > +	area->next_len -= len + 1;
> > +	area->next_addr = ++end;
> > +	return start;
> > +}
> > +
> > +static int process_text_file(const char *path, const char *prefix,
> > +		struct selabel_handle *rec)
> >  {
> >  	int rc;
> > -	size_t line_len;
> >  	unsigned int lineno = 0;
> > -	char *line_buf = NULL;
> > +	char *line_buf;
> > +	struct mmap_area *areas = rec_to_data(rec)->mmap_areas;
> > +
> > +	/* mmap_area_get_line() and process_line() require mutable string
> pointers */
> > +	rc = mprotect(areas->addr, areas->len, PROT_READ | PROT_WRITE);
> 
> Just map it with PROT_READ|PROT_WRITE in the beginning.

Yep.

Roberts, William C Sept. 20, 2016, 9:21 p.m. UTC | #11

> > On 09/19/2016 05:51 PM, Roberts, William C wrote:
> > > FYI I only tested this with checkfc...
> >
> > Evidently.  matchpathcon and sefcontext_compile both report calls to
> > free() on invalid pointers and abort.
> 
> That doesn’t surprise me, I only tested the checkfc usages. Hence #4 in todo's
> 

I looked at the sefcontext_compile bug, and its yet another conglomeration where internal
Interfaces into libselinux are presumed and duplicate code exists (for loading a file).
It implements its own routine for the freeing of Spec data. Why doesn't it use proper
libselinux interfaces, or libselinux expose the proper Interfaces for this type of work?
Is this just an example of something that should be fixed or is there some deeper
reasoning to its construction?

I could not get matchpatchcon to reproduce (built with ASAN):
./selinux/libselinux/utils/matchpathcon /etc -f file_contexts
/etc	u:object_r:rootfs:s0

Bill

Roberts, William C Sept. 20, 2016, 10:33 p.m. UTC | #12

<snip>

> > -----Original Message-----
> > From: Roberts, William C
> > Sent: Monday, September 19, 2016 2:45 PM
> > To: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]
> > Cc: Roberts, William C <[e-mail]>
> > Subject: [RFC] mmap file_contexts and property_contexts:
> >
> > From: William Roberts <[e-mail]>
> >
> > THIS IS WIP...
> >
> > Rather than using stdio and making copies, just mmap the files and use
> > the pointers in place. The affect of this change, is that text file
> > load time is now faster than binary load time by 4.7% when testing
> > with a file_contexts file from the Android tree. Note that the Android doesn't
> use monstrous regexs.
> >
> > Times are the average of 3 runs.
> >
> > BEFORE:
> > Text file allocs: 114803
> > Text file load time: 0.266101
> > Bin file allocs: 93073
> > Bin file load time: 0.248757667
> >
> > AFTER:
> > Text file allocs: 103933
> > Text file load time: 0.236192667
> > Bin file allocs: 87645
> > Bin file load time: .247607333

Another potential issue, is that mmaping with textfiles also has the sideffect
of mapping in all the spaces or tabs, etc used, between fields, so on files with
heavy whitespace, it could be really memory intensive. 

I think Janis's patch that has -r option is a more useful way to go. Without precompiled
Regex's its essentially a textfile without the whitespace, and it automatically would
get pulled into the mmap interface.

I'm abandoning this, since we already get this functionality, without the negatives.
Janis, IIRC, you are re-configuring your patch so -r omits pre-compiled regex's (submitted
I believe on the list and awaiting other patches) and adding an arch string so we can
Detect when we need to recompile the regular expressions if they are supplied but
unusable.

I think we may want to investigate to have the "omit pre-compiled regex's" work on PCRE
library variants as well, but I doubt it would be used since Android is moving to PCRE2.

<snip>

Patch

diff --git a/libselinux/src/label.c b/libselinux/src/label.c
index 963bfcb..d767b49 100644
--- a/libselinux/src/label.c
+++ b/libselinux/src/label.c
@@ -15,8 +15,6 @@ 
 #include "callbacks.h"
 #include "label_internal.h"
 
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
-
 typedef int (*selabel_initfunc)(struct selabel_handle *rec,
 				const struct selinux_opt *opts,
 				unsigned nopts);
diff --git a/libselinux/src/label_android_property.c b/libselinux/src/label_android_property.c
index 290b438..2aac394 100644
--- a/libselinux/src/label_android_property.c
+++ b/libselinux/src/label_android_property.c
@@ -85,13 +85,19 @@  static int process_line(struct selabel_handle *rec,
 			int pass, unsigned lineno)
 {
 	int items;
-	char *prop = NULL, *context = NULL;
+	union {
+		struct {
+			char *prop;
+			char *context;
+		};
+		char *array[2];
+	} found = { .array = { 0 } };
 	struct saved_data *data = (struct saved_data *)rec->data;
 	spec_t *spec_arr = data->spec_arr;
 	unsigned int nspec = data->nspec;
 	const char *errbuf = NULL;
 
-	items = read_spec_entries(line_buf, &errbuf, 2, &prop, &context);
+	items = read_spec_entries(line_buf, &errbuf, ARRAY_SIZE(found.array), found.array);
 	if (items < 0) {
 		items = errno;
 		selinux_log(SELINUX_ERROR,
@@ -108,18 +114,14 @@  static int process_line(struct selabel_handle *rec,
 		selinux_log(SELINUX_ERROR,
 			    "%s:  line %u is missing fields\n", path,
 			    lineno);
-		free(prop);
 		errno = EINVAL;
 		return -1;
 	}
 
-	if (pass == 0) {
-		free(prop);
-		free(context);
-	} else if (pass == 1) {
+	if (pass == 1) {
 		/* On the second pass, process and store the specification in spec. */
-		spec_arr[nspec].property_key = prop;
-		spec_arr[nspec].lr.ctx_raw = context;
+		spec_arr[nspec].property_key = found.prop;
+		spec_arr[nspec].lr.ctx_raw = found.context;
 
 		if (rec->validating) {
 			if (selabel_validate(rec, &spec_arr[nspec].lr) < 0) {
@@ -234,7 +236,7 @@  static void closef(struct selabel_handle *rec)
 	for (i = 0; i < data->nspec; i++) {
 		spec = &data->spec_arr[i];
 		free(spec->property_key);
-		free(spec->lr.ctx_raw);
+		//free(spec->lr.ctx_raw);
 		free(spec->lr.ctx_trans);
 	}
 
diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index 7156825..4dc440e 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -96,43 +96,64 @@  static int nodups_specs(struct saved_data *data, const char *path)
 	return rc;
 }
 
-static int process_text_file(FILE *fp, const char *prefix,
-			     struct selabel_handle *rec, const char *path)
+static inline struct saved_data *rec_to_data(struct selabel_handle *rec)
+{
+	return (struct saved_data *)rec->data;
+}
+
+static char *mmap_area_get_line(struct mmap_area *area)
+{
+	size_t len = area->next_len;
+	if (!len)
+		return NULL;
+
+	char *start = area->next_addr;
+	char *end = memchr(start, '\n', len);
+
+	/* the file may not end with a newline */
+	if (!end)
+		end = (char *)area->next_addr + len - 1;
+
+	*end = '\0';
+	/* len includes null byte */
+	len = end - start;
+
+	area->next_len -= len + 1;
+	area->next_addr = ++end;
+	return start;
+}
+
+static int process_text_file(const char *path, const char *prefix,
+		struct selabel_handle *rec)
 {
 	int rc;
-	size_t line_len;
 	unsigned int lineno = 0;
-	char *line_buf = NULL;
+	char *line_buf;
+	struct mmap_area *areas = rec_to_data(rec)->mmap_areas;
+
+	/* mmap_area_get_line() and process_line() require mutable string pointers */
+	rc = mprotect(areas->addr, areas->len, PROT_READ | PROT_WRITE);
+	if (rc < 0)
+		goto out;
 
-	while (getline(&line_buf, &line_len, fp) > 0) {
+	while ( (line_buf = mmap_area_get_line(areas)) ) {
 		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)
+static int mmap_file(FILE *fp, size_t len, struct saved_data *data)
 {
-	struct saved_data *data = (struct saved_data *)rec->data;
-	int rc;
-	char *addr, *str_buf;
-	int *stem_map;
-	struct mmap_area *mmap_area;
-	uint32_t i, magic, version;
-	uint32_t entry_len, stem_map_len, regex_array_len;
-	const char *reg_version;
-
-	mmap_area = malloc(sizeof(*mmap_area));
+	struct mmap_area *mmap_area = malloc(sizeof(*mmap_area));
 	if (!mmap_area) {
 		return -1;
 	}
 
-	addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
+	void *addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fileno(fp), 0);
 	if (addr == MAP_FAILED) {
 		free(mmap_area);
 		perror("mmap");
@@ -145,17 +166,26 @@  static int load_mmap(FILE *fp, size_t len, struct selabel_handle *rec,
 	mmap_area->next = data->mmap_areas;
 	data->mmap_areas = mmap_area;
 
-	/* 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)
-		return -1;
+	return 0;
+}
+
+static int process_binary_file(const char *path, struct selabel_handle *rec)
+{
+	struct saved_data *data = (struct saved_data *)rec->data;
+	int rc;
+	char *str_buf;
+	int *stem_map;
+	struct mmap_area *mmap_area = data->mmap_areas;
+	uint32_t i, version;
+	uint32_t entry_len, stem_map_len, regex_array_len;
+	size_t len;
 
 	/* check if this version is higher than we understand */
 	rc = next_entry(&version, mmap_area, sizeof(uint32_t));
 	if (rc < 0 || version > SELINUX_COMPILED_FCONTEXT_MAX_VERS)
 		return -1;
 
-	reg_version = regex_version();
+	const char *reg_version = regex_version();
 	if (!reg_version)
 		return -1;
 
@@ -235,12 +265,11 @@  static int load_mmap(FILE *fp, size_t len, struct selabel_handle *rec,
 		/* store the mapping between old and new */
 		newid = find_stem(data, buf, stem_len);
 		if (newid < 0) {
-			newid = store_stem(data, buf, stem_len);
+			newid = store_stem(data, buf, stem_len, true);
 			if (newid < 0) {
 				rc = newid;
 				goto out;
 			}
-			data->stem_arr[newid].from_mmap = 1;
 		}
 		stem_map[i] = newid;
 	}
@@ -262,7 +291,6 @@  static int load_mmap(FILE *fp, size_t len, struct selabel_handle *rec,
 			goto out;
 
 		spec = &data->spec_arr[data->nspec];
-		spec->from_mmap = 1;
 
 		/* Process context */
 		rc = next_entry(&entry_len, mmap_area, sizeof(uint32_t));
@@ -271,22 +299,10 @@  static int load_mmap(FILE *fp, size_t len, struct selabel_handle *rec,
 			goto out;
 		}
 
-		str_buf = malloc(entry_len);
-		if (!str_buf) {
-			rc = -1;
-			goto out;
-		}
-		rc = next_entry(str_buf, mmap_area, entry_len);
+		rc = next_pstr(&spec->lr.ctx_raw, mmap_area, entry_len);
 		if (rc < 0)
 			goto out;
 
-		if (str_buf[entry_len - 1] != '\0') {
-			free(str_buf);
-			rc = -1;
-			goto out;
-		}
-		spec->lr.ctx_raw = str_buf;
-
 		if (strcmp(spec->lr.ctx_raw, "<<none>>") && rec->validating) {
 			if (selabel_validate(rec, &spec->lr) < 0) {
 				selinux_log(SELINUX_ERROR,
@@ -401,17 +417,29 @@  static char *rolling_append(char *current, const char *suffix, size_t max)
 	return current;
 }
 
-static bool fcontext_is_binary(FILE *fp)
+static inline int mmap_area_rewind(struct mmap_area *area, size_t bytes)
+{
+	if (area->next_len + bytes > area->len)
+		return -1;
+
+	area->next_addr = (char *) area->next_addr - bytes;
+	area->next_len += bytes;
+	return 0;
+}
+
+static bool fcontext_is_binary(struct mmap_area *area)
 {
 	uint32_t magic;
+	bool is_binary = false;
 
-	size_t len = fread(&magic, sizeof(magic), 1, fp);
-	rewind(fp);
+	int rc = next_entry(&magic, area, sizeof(magic));
 
-	return (len && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
-}
+	is_binary = (!rc && (magic == SELINUX_MAGIC_COMPILED_FCONTEXT));
+	if (!is_binary)
+		mmap_area_rewind(area, sizeof(magic));
 
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+	return is_binary;
+}
 
 static FILE *open_file(const char *path, const char *suffix,
 	       char *save_path, size_t len, struct stat *sb, bool open_oldest)
@@ -504,12 +532,17 @@  static int process_file(const char *path, const char *suffix,
 		if (fp == NULL)
 			return -1;
 
-		rc = fcontext_is_binary(fp) ?
-				load_mmap(fp, sb.st_size, rec, found_path) :
-				process_text_file(fp, prefix, rec, found_path);
+		rc = mmap_file(fp, sb.st_size, rec_to_data(rec));
+		if (rc < 0) {
+			fclose(fp);
+			return -1;
+		}
+
+		rc = fcontext_is_binary(rec_to_data(rec)->mmap_areas) ?
+				process_binary_file(found_path, rec) :
+				process_text_file(found_path, prefix, rec);
 		if (!rc)
-			rc = digest_add_specfile(digest, fp, NULL, sb.st_size,
-				found_path);
+			rc = digest_add_specfile(digest, fp, NULL, sb.st_size, found_path);
 
 		fclose(fp);
 
@@ -613,12 +646,7 @@  static void closef(struct selabel_handle *rec)
 	for (i = 0; i < data->nspec; i++) {
 		spec = &data->spec_arr[i];
 		free(spec->lr.ctx_trans);
-		free(spec->lr.ctx_raw);
 		regex_data_free(spec->regex);
-		if (spec->from_mmap)
-			continue;
-		free(spec->regex_str);
-		free(spec->type_str);
 	}
 
 	for (i = 0; i < (unsigned int)data->num_stems; i++) {
diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
index 88f4294..2704906 100644
--- a/libselinux/src/label_file.h
+++ b/libselinux/src/label_file.h
@@ -37,7 +37,6 @@  struct spec {
 	int matches;		/* number of matching pathnames */
 	int stem_id;		/* indicates which stem-compression item */
 	char hasMetaChars;	/* regular expression has meta-chars */
-	char from_mmap;		/* this spec is from an mmap of the data */
 	size_t prefix_len;      /* length of fixed path prefix */
 };
 
@@ -255,7 +254,7 @@  static inline int find_stem(struct saved_data *data, const char *buf,
 }
 
 /* returns the index of the new stored object */
-static inline int store_stem(struct saved_data *data, char *buf, int stem_len)
+static inline int store_stem(struct saved_data *data, char *buf, int stem_len, bool from_mmap)
 {
 	int num = data->num_stems;
 
@@ -271,7 +270,7 @@  static inline int store_stem(struct saved_data *data, char *buf, int stem_len)
 	}
 	data->stem_arr[num].len = stem_len;
 	data->stem_arr[num].buf = buf;
-	data->stem_arr[num].from_mmap = 0;
+	data->stem_arr[num].from_mmap = from_mmap;
 	data->num_stems++;
 
 	return num;
@@ -298,7 +297,7 @@  static inline int find_stem_from_spec(struct saved_data *data, const char *buf)
 	if (!stem)
 		return -1;
 
-	return store_stem(data, stem, stem_len);
+	return store_stem(data, stem, stem_len, false);
 }
 
 /* This will always check for buffer over-runs and either read the next entry
@@ -317,6 +316,22 @@  static inline int next_entry(void *buf, struct mmap_area *fp, size_t bytes)
 	return 0;
 }
 
+static inline int next_pstr(char **str, struct mmap_area *fp, size_t len)
+{
+	if (len > fp->next_len)
+		return -1;
+
+	char *tmp = (char *)fp->next_addr;
+	if (tmp[len-1] != '\0')
+		return -1;
+
+	*str = tmp;
+
+	fp->next_addr = (char *)fp->next_addr + len;
+	fp->next_len -= len;
+	return 0;
+}
+
 static inline int compile_regex(struct saved_data *data, struct spec *spec,
 					    const char **errbuf)
 {
@@ -375,13 +390,21 @@  static inline int process_line(struct selabel_handle *rec,
 			char *line_buf, unsigned lineno)
 {
 	int items, len, rc;
-	char *regex = NULL, *type = NULL, *context = NULL;
+	union {
+		struct {
+			char *regex;
+			char *type;
+			char *context;
+		};
+		char *array[3];
+	} found = { .array = { 0 } };
+
 	struct saved_data *data = (struct saved_data *)rec->data;
 	struct spec *spec_arr;
 	unsigned int nspec = data->nspec;
 	const char *errbuf = NULL;
 
-	items = read_spec_entries(line_buf, &errbuf, 3, &regex, &type, &context);
+	items = read_spec_entries(line_buf, &errbuf, ARRAY_SIZE(found.array), found.array);
 	if (items < 0) {
 		rc = errno;
 		selinux_log(SELINUX_ERROR,
@@ -398,22 +421,17 @@  static inline int process_line(struct selabel_handle *rec,
 		COMPAT_LOG(SELINUX_ERROR,
 			    "%s:  line %u is missing fields\n", path,
 			    lineno);
-		if (items == 1)
-			free(regex);
 		errno = EINVAL;
 		return -1;
 	} else if (items == 2) {
 		/* The type field is optional. */
-		context = type;
-		type = 0;
+		found.context = found.type;
+		found.type = NULL;
 	}
 
-	len = get_stem_from_spec(regex);
-	if (len && prefix && strncmp(prefix, regex, len)) {
+	len = get_stem_from_spec(found.regex);
+	if (len && prefix && strncmp(prefix, found.regex, len)) {
 		/* Stem of regex does not match requested prefix, discard. */
-		free(regex);
-		free(type);
-		free(context);
 		return 0;
 	}
 
@@ -424,13 +442,13 @@  static inline int process_line(struct selabel_handle *rec,
 	spec_arr = data->spec_arr;
 
 	/* process and store the specification in spec. */
-	spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
-	spec_arr[nspec].regex_str = regex;
+	spec_arr[nspec].stem_id = find_stem_from_spec(data, found.regex);
+	spec_arr[nspec].regex_str = found.regex;
 
-	spec_arr[nspec].type_str = type;
+	spec_arr[nspec].type_str = found.type;
 	spec_arr[nspec].mode = 0;
 
-	spec_arr[nspec].lr.ctx_raw = context;
+	spec_arr[nspec].lr.ctx_raw = found.context;
 
 	/*
 	 * bump data->nspecs to cause closef() to cover it in its free
@@ -442,18 +460,18 @@  static inline int process_line(struct selabel_handle *rec,
 			&& compile_regex(data, &spec_arr[nspec], &errbuf)) {
 		COMPAT_LOG(SELINUX_ERROR,
 			   "%s:  line %u has invalid regex %s:  %s\n",
-			   path, lineno, regex, errbuf);
+			   path, lineno, found.regex, errbuf);
 		errno = EINVAL;
 		return -1;
 	}
 
-	if (type) {
-		mode_t mode = string_to_mode(type);
+	if (found.type) {
+		mode_t mode = string_to_mode(found.type);
 
 		if (mode == (mode_t)-1) {
 			COMPAT_LOG(SELINUX_ERROR,
 				   "%s:  line %u has invalid file type %s\n",
-				   path, lineno, type);
+				   path, lineno, found.type);
 			errno = EINVAL;
 			return -1;
 		}
@@ -464,7 +482,7 @@  static inline int process_line(struct selabel_handle *rec,
 	 * any meta characters in the RE */
 	spec_hasMetaChars(&spec_arr[nspec]);
 
-	if (strcmp(context, "<<none>>") && rec->validating)
+	if (strcmp(found.context, "<<none>>") && rec->validating)
 		compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
 
 	return 0;
diff --git a/libselinux/src/label_internal.h b/libselinux/src/label_internal.h
index aa48fff..e40d68b 100644
--- a/libselinux/src/label_internal.h
+++ b/libselinux/src/label_internal.h
@@ -136,10 +136,11 @@  compat_validate(struct selabel_handle *rec,
 		struct selabel_lookup_rec *contexts,
 		const char *path, unsigned lineno) hidden;
 
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 /*
  * The read_spec_entries function may be used to
  * replace sscanf to read entries from spec files.
  */
-extern int read_spec_entries(char *line_buf, const char **errbuf, int num_args, ...);
+extern int hidden read_spec_entries(char *line_buf, const char **errbuf, unsigned int num, char *found[]);
 
 #endif				/* _SELABEL_INTERNAL_H_ */
diff --git a/libselinux/src/label_support.c b/libselinux/src/label_support.c
index 26f9ef1..3f5728d 100644
--- a/libselinux/src/label_support.c
+++ b/libselinux/src/label_support.c
@@ -17,39 +17,40 @@ 
  * Read an entry from a spec file (e.g. file_contexts)
  * entry - Buffer to allocate for the entry.
  * ptr - current location of the line to be processed.
- * returns  - 0 on success and *entry is set to be a null
- *            terminated value. On Error it returns -1 and
- *            errno will be set.
+ * returns  - a pointer to the begining of the string.
+ *            that is the
  *
  */
-static inline int read_spec_entry(char **entry, char **ptr, int *len, const char **errbuf)
+static inline char *read_spec_entry(char **current, const char **errbuf)
 {
-	*entry = NULL;
-	char *tmp_buf = NULL;
-
-	while (isspace(**ptr) && **ptr != '\0')
-		(*ptr)++;
+	if(!**current) {
+		(*current)++;
+		return NULL;
+	}
 
-	tmp_buf = *ptr;
-	*len = 0;
+	while (isspace(**current) && **current != '\0')
+		(*current)++;
 
-	while (!isspace(**ptr) && **ptr != '\0') {
-		if (!isascii(**ptr)) {
+	char *start = *current;
+	while (!isspace(**current) && **current != '\0') {
+		if (!isascii(**current)) {
 			errno = EINVAL;
 			*errbuf = "Non-ASCII characters found";
-			return -1;
+			return NULL;
 		}
-		(*ptr)++;
-		(*len)++;
+		(*current)++;
 	}
 
-	if (*len) {
-		*entry = strndup(tmp_buf, *len);
-		if (!*entry)
-			return -1;
-	}
+	char *end = *current;
 
-	return 0;
+	if (start == end)
+		return NULL;
+
+	if (*end != '\0') {
+		*end = '\0';
+		(*current)++;
+	}
+	return start;
 }
 
 /*
@@ -63,15 +64,13 @@  static inline int read_spec_entry(char **entry, char **ptr, int *len, const char
  * This function calls read_spec_entry() to do the actual string processing.
  * As such, can return anything from that function as well.
  */
-int hidden read_spec_entries(char *line_buf, const char **errbuf, int num_args, ...)
+int hidden read_spec_entries(char *line_buf, const char **errbuf, unsigned int num, char *found[])
 {
-	char **spec_entry, *buf_p;
-	int len, rc, items, entry_len = 0;
-	va_list ap;
-
+	char*buf_p;
+	unsigned int items = 0;
 	*errbuf = NULL;
 
-	len = strlen(line_buf);
+	size_t len = strlen(line_buf);
 	if (line_buf[len - 1] == '\n')
 		line_buf[len - 1] = '\0';
 	else
@@ -89,26 +88,12 @@  int hidden read_spec_entries(char *line_buf, const char **errbuf, int num_args,
 		return 0;
 
 	/* Process the spec file entries */
-	va_start(ap, num_args);
-
-	items = 0;
-	while (items < num_args) {
-		spec_entry = va_arg(ap, char **);
-
-		if (len - 1 == buf_p - line_buf) {
-			va_end(ap);
-			return items;
-		}
-
-		rc = read_spec_entry(spec_entry, &buf_p, &entry_len, errbuf);
-		if (rc < 0) {
-			va_end(ap);
-			return rc;
-		}
-		if (entry_len)
-			items++;
+	while (items < num) {
+		found[items] = read_spec_entry(&buf_p, errbuf);
+		if (!found[items])
+			break;
+		items++;
 	}
-	va_end(ap);
 	return items;
 }