Nov 27, 2024 16:14:50 James Carter <jwcart2@xxxxxxxxx>:
> On Tue, Nov 26, 2024 at 5:45 AM Christian Göttsche
> <cgoettsche@xxxxxxxxxxxxx> wrote:
>>
>> From: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
>>
>> Utilize cache locality for the substitutions by storing them in
>> contiguous memory instead of a linked list.
>>
>> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
>> ---
>> v2: drop unnecessary check for zero length
>> ---
>> libselinux/src/label_file.c | 127 +++++++++++++++++++-----------------
>> libselinux/src/label_file.h | 20 ++++--
>> 2 files changed, 80 insertions(+), 67 deletions(-)
>>
>> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
>> index 4e212aa4..c91a91f7 100644
>> --- a/libselinux/src/label_file.c
>> +++ b/libselinux/src/label_file.c
>> @@ -1120,28 +1120,27 @@ static int process_file(const char *path, const char *suffix,
>> return -1;
>> }
>>
>> -static void selabel_subs_fini(struct selabel_sub *ptr)
>> +static void selabel_subs_fini(struct selabel_sub *subs, uint32_t num)
>> {
>> - struct selabel_sub *next;
>> -
>> - while (ptr) {
>> - next = ptr->next;
>> - free(ptr->src);
>> - free(ptr->dst);
>> - free(ptr);
>> - ptr = next;
>> + for (uint32_t i = 0; i < num; i++) {
>> + free(subs[i].src);
>> + free(subs[i].dst);
>> }
>> +
>> + free(subs);
>> }
>>
>> -static char *selabel_sub(const struct selabel_sub *ptr, const char *src)
>> +static char *selabel_apply_subs(const struct selabel_sub *subs, uint32_t num, const char *src)
>> {
>> - char *dst = NULL;
>> - unsigned int len;
>> + char *dst;
>> + uint32_t len;
>> +
>> + for (uint32_t i = 0; i < num; i++) {
>> + const struct selabel_sub *ptr = &subs[i];
>>
>> - while (ptr) {
>> if (strncmp(src, ptr->src, ptr->slen) == 0 ) {
>> if (src[ptr->slen] == '/' ||
>> - src[ptr->slen] == 0) {
>> + src[ptr->slen] == '\0') {
>> if ((src[ptr->slen] == '/') &&
>> (strcmp(ptr->dst, "/") == 0))
>> len = ptr->slen + 1;
>> @@ -1152,34 +1151,38 @@ static char *selabel_sub(const struct selabel_sub *ptr, const char *src)
>> return dst;
>> }
>> }
>> - ptr = ptr->next;
>> }
>> +
>> return NULL;
>> }
>>
>> #if !defined(BUILD_HOST) && !defined(ANDROID)
>> static int selabel_subs_init(const char *path, struct selabel_digest *digest,
>> - struct selabel_sub **out_subs)
>> + struct selabel_sub **out_subs,
>> + uint32_t *out_num, uint32_t *out_alloc)
>> {
>> char buf[1024];
>> - FILE *cfg = fopen(path, "re");
>> - struct selabel_sub *list = NULL, *sub = NULL;
>> + FILE *cfg;
>> struct stat sb;
>> - int status = -1;
>> + struct selabel_sub *tmp = NULL;
>> + uint32_t tmp_num = 0, tmp_alloc = 0;
>
> Sorry for replying to v2, but I never received v3 (I can download it
> with b4). It doesn't matter for my comment.
>
> tmp_alloc is never updated.
>
>> + char *src_cpy = NULL, *dst_cpy = NULL;
>> + int rc;
>>
>> *out_subs = NULL;
>> + *out_num = 0;
>> + *out_alloc = 0;
>
> Near the end of this function, we have *out_alloc = tmp_alloc, but
> since tmp_alloc is never updated, this is always going to be 0 (unless
> I missed something).
tmp_alloc is updated in the macro GROW_ARRAY(tmp), which modifies the data pointer tmp, its size tmp_num, and its capacity tmp_alloc.
> Thanks,
> Jim
>
>
>> +
>> + cfg = fopen(path, "re");
>> if (!cfg) {
>> /* If the file does not exist, it is not fatal */
>> return (errno == ENOENT) ? 0 : -1;
>> }
>>
>> - if (fstat(fileno(cfg), &sb) < 0)
>> - goto out;
>> -
>> while (fgets_unlocked(buf, sizeof(buf) - 1, cfg)) {
>> - char *ptr = NULL;
>> + char *ptr;
>> char *src = buf;
>> - char *dst = NULL;
>> + char *dst;
>> size_t len;
>>
>> while (*src && isspace((unsigned char)*src))
>> @@ -1207,62 +1210,64 @@ static int selabel_subs_init(const char *path, struct selabel_digest *digest,
>> goto err;
>> }
>>
>> - sub = calloc(1, sizeof(*sub));
>> - if (! sub)
>> + src_cpy = strdup(src);
>> + if (!src_cpy)
>> goto err;
>>
>> - sub->src = strdup(src);
>> - if (! sub->src)
>> + dst_cpy = strdup(dst);
>> + if (!dst_cpy)
>> goto err;
>>
>> - sub->dst = strdup(dst);
>> - if (! sub->dst)
>> + rc = GROW_ARRAY(tmp);
>> + if (rc)
>> goto err;
>>
>> - sub->slen = len;
>> - sub->next = list;
>> - list = sub;
>> - sub = NULL;
>> + tmp[tmp_num++] = (struct selabel_sub) {
>> + .src = src_cpy,
>> + .slen = len,
>> + .dst = dst_cpy,
>> + };
>> + src_cpy = NULL;
>> + dst_cpy = NULL;
>> }
>>
>> + rc = fstat(fileno(cfg), &sb);
>> + if (rc < 0)
>> + goto err;
>> +
>> if (digest_add_specfile(digest, cfg, NULL, sb.st_size, path) < 0)
>> goto err;
>>
>> - *out_subs = list;
>> - status = 0;
>> + *out_subs = tmp;
>> + *out_num = tmp_num;
>> + *out_alloc = tmp_alloc;
>>
>> -out:
>> fclose(cfg);
>> - return status;
>> +
>> + return 0;
>> +
>> err:
>> - if (sub)
>> - free(sub->src);
>> - free(sub);
>> - while (list) {
>> - sub = list->next;
>> - free(list->src);
>> - free(list->dst);
>> - free(list);
>> - list = sub;
>> - }
>> - goto out;
>> + free(dst_cpy);
>> + free(src_cpy);
>> + free(tmp);
>> + fclose_errno_safe(cfg);
>> + return -1;
>> }
>> #endif
>>
>> static char *selabel_sub_key(const struct saved_data *data, const char *key)
>> {
>> - char *ptr = NULL;
>> - char *dptr = NULL;
>> + char *ptr, *dptr;
>>
>> - ptr = selabel_sub(data->subs, key);
>> + ptr = selabel_apply_subs(data->subs, data->subs_num, key);
>> if (ptr) {
>> - dptr = selabel_sub(data->dist_subs, ptr);
>> + dptr = selabel_apply_subs(data->dist_subs, data->dist_subs_num, ptr);
>> if (dptr) {
>> free(ptr);
>> ptr = dptr;
>> }
>> } else {
>> - ptr = selabel_sub(data->dist_subs, key);
>> + ptr = selabel_apply_subs(data->dist_subs, data->dist_subs_num, key);
>> }
>>
>> return ptr;
>> @@ -1307,23 +1312,25 @@ static int init(struct selabel_handle *rec, const struct selinux_opt *opts,
>> if (!path) {
>> status = selabel_subs_init(
>> selinux_file_context_subs_dist_path(),
>> - rec->digest, &data->dist_subs);
>> + rec->digest,
>> + &data->dist_subs, &data->dist_subs_num, &data->dist_subs_alloc);
>> if (status)
>> goto finish;
>> status = selabel_subs_init(selinux_file_context_subs_path(),
>> - rec->digest, &data->subs);
>> + rec->digest,
>> + &data->subs, &data->subs_num, &data->subs_alloc);
>> if (status)
>> goto finish;
>> path = selinux_file_context_path();
>> } else {
>> snprintf(subs_file, sizeof(subs_file), "%s.subs_dist", path);
>> status = selabel_subs_init(subs_file, rec->digest,
>> - &data->dist_subs);
>> + &data->dist_subs, &data->dist_subs_num, &data->dist_subs_alloc);
>> if (status)
>> goto finish;
>> snprintf(subs_file, sizeof(subs_file), "%s.subs", path);
>> status = selabel_subs_init(subs_file, rec->digest,
>> - &data->subs);
>> + &data->subs, &data->subs_num, &data->subs_alloc);
>> if (status)
>> goto finish;
>> }
>> @@ -1391,8 +1398,8 @@ static void closef(struct selabel_handle *rec)
>> if (!data)
>> return;
>>
>> - selabel_subs_fini(data->subs);
>> - selabel_subs_fini(data->dist_subs);
>> + selabel_subs_fini(data->subs, data->subs_num);
>> + selabel_subs_fini(data->dist_subs, data->dist_subs_num);
>>
>> free_spec_node(data->root);
>> free(data->root);
>> diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
>> index de8190f9..436982bf 100644
>> --- a/libselinux/src/label_file.h
>> +++ b/libselinux/src/label_file.h
>> @@ -67,11 +67,11 @@ extern struct lookup_result *lookup_all(struct selabel_handle *rec, const char *
>> extern enum selabel_cmp_result cmp(const struct selabel_handle *h1, const struct selabel_handle *h2);
>> #endif /* FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION */
>>
>> +/* A path substitution entry */
>> struct selabel_sub {
>> - char *src;
>> - unsigned int slen;
>> - char *dst;
>> - struct selabel_sub *next;
>> + char *src; /* source path prefix */
>> + char *dst; /* substituted path prefix */
>> + uint32_t slen; /* length of source path prefix */
>> };
>>
>> /* A regular expression file security context specification */
>> @@ -159,9 +159,17 @@ struct saved_data {
>>
>> struct mmap_area *mmap_areas;
>>
>> - /* substitution support */
>> + /*
>> + * Array of distribution substitutions
>> + */
>> struct selabel_sub *dist_subs;
>> + uint32_t dist_subs_num, dist_subs_alloc;
>> +
>> + /*
>> + * Array of local substitutions
>> + */
>> struct selabel_sub *subs;
>> + uint32_t subs_num, subs_alloc;
>> };
>>
>> static inline mode_t string_to_file_kind(const char *mode)
>> @@ -811,8 +819,6 @@ static int insert_spec(const struct selabel_handle *rec, struct saved_data *data
>> return 0;
>> }
>>
>> -#undef GROW_ARRAY
>> -
>> static inline void free_spec_node(struct spec_node *node)
>> {
>> for (uint32_t i = 0; i < node->literal_specs_num; i++) {
>> --
>> 2.45.2
>>
>>
