On Fri, Jan 31, 2025 at 1:03 PM Christian Göttsche
<cgoettsche@xxxxxxxxxxxxx> wrote:
>
> From: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
>
> Use a function local, and thus thread-safe, buffer for error messages
> instead of a shared global one.
>
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> ---
> libselinux/src/label_file.c | 4 ++--
> libselinux/src/label_file.h | 19 +++++++------------
> 2 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> index 56e20949..85d42ff2 100644
> --- a/libselinux/src/label_file.c
> +++ b/libselinux/src/label_file.c
> @@ -1662,7 +1662,7 @@ static struct lookup_result *lookup_check_node(struct spec_node *node, const cha
> for (uint32_t i = n->regex_specs_num; i > 0; i--) {
> /* search in reverse order */
> struct regex_spec *rspec = &n->regex_specs[i - 1];
> - const char *errbuf = NULL;
> + char errbuf[256];
> int rc;
>
> if (child_regex_match &&
> @@ -1673,7 +1673,7 @@ static struct lookup_result *lookup_check_node(struct spec_node *node, const cha
> if (file_kind != LABEL_FILE_KIND_ALL && rspec->file_kind != LABEL_FILE_KIND_ALL && file_kind != rspec->file_kind)
> continue;
>
> - if (compile_regex(rspec, &errbuf) < 0) {
> + if (compile_regex(rspec, errbuf, sizeof(errbuf)) < 0) {
> COMPAT_LOG(SELINUX_ERROR, "Failed to compile regular expression '%s': %s\n",
> rspec->regex_str, errbuf);
> goto fail;
> diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
> index 60ebbb47..ad7699e6 100644
> --- a/libselinux/src/label_file.h
> +++ b/libselinux/src/label_file.h
> @@ -388,16 +388,17 @@ static inline void sort_specs(struct saved_data *data)
> sort_spec_node(data->root, NULL);
> }
>
> -static inline int compile_regex(struct regex_spec *spec, const char **errbuf)
> +static inline int compile_regex(struct regex_spec *spec, char *errbuf, size_t errbuf_size)
> {
> const char *reg_buf;
> char *anchored_regex, *cp;
> struct regex_error_data error_data;
> - static char regex_error_format_buffer[256];
> size_t len;
> int rc;
> bool regex_compiled;
>
> + *errbuf = '\0';
> +
I know that this will never be null because there is only one caller
of this function, but since it is in a header file, there is no
guarantee that it won't be called from somewhere else in the future.
Can either let the caller deal with it or add a check here that errbuf
is not NULL.
Thanks,
Jim
> /* We really want pthread_once() here, but since its
> * init_routine does not take a parameter, it's not possible
> * to use, so we generate the same effect with atomics and a
> @@ -435,9 +436,8 @@ static inline int compile_regex(struct regex_spec *spec, const char **errbuf)
> len = strlen(reg_buf);
> cp = anchored_regex = malloc(len + 3);
> if (!anchored_regex) {
> - if (errbuf)
> - *errbuf = "out of memory";
> __pthread_mutex_unlock(&spec->regex_lock);
> + snprintf(errbuf, errbuf_size, "out of memory");
> return -1;
> }
>
> @@ -452,12 +452,7 @@ static inline int compile_regex(struct regex_spec *spec, const char **errbuf)
> rc = regex_prepare_data(&spec->regex, anchored_regex, &error_data);
> free(anchored_regex);
> if (rc < 0) {
> - if (errbuf) {
> - regex_format_error(&error_data,
> - regex_error_format_buffer,
> - sizeof(regex_error_format_buffer));
> - *errbuf = ®ex_error_format_buffer[0];
> - }
> + regex_format_error(&error_data, errbuf, errbuf_size);
> __pthread_mutex_unlock(&spec->regex_lock);
> errno = EINVAL;
> return -1;
> @@ -624,9 +619,9 @@ static int insert_spec(const struct selabel_handle *rec, struct saved_data *data
> data->num_specs++;
>
> if (rec->validating) {
> - const char *errbuf = NULL;
> + char errbuf[256];
>
> - if (compile_regex(&node->regex_specs[id], &errbuf)) {
> + if (compile_regex(&node->regex_specs[id], errbuf, sizeof(errbuf))) {
> COMPAT_LOG(SELINUX_ERROR,
> "%s: line %u has invalid regex %s: %s\n",
> path, lineno, regex, errbuf);
> --
> 2.45.2
>
>
