diff mbox series

libselinux: preserve errno in selinux_log()

Message ID 20220511184132.217891-1-cgzones@googlemail.com (mailing list archive)
State Accepted
Commit 88d43a8dc278
Headers show
Series libselinux: preserve errno in selinux_log() | expand

Commit Message

Christian Göttsche May 11, 2022, 6:41 p.m. UTC 
selinux_log() is used in many error branches, where the caller might
expect errno to bet set, e.g. label_file.c::lookup_all():

    if (match_count) {
		*match_count = 0;
		result = calloc(data->nspec, sizeof(struct spec*));
	} else {
		result = calloc(1, sizeof(struct spec*));
	}
	if (!result) {
		selinux_log(SELINUX_ERROR, "Failed to allocate %zu bytes of data\n",
			    data->nspec * sizeof(struct spec*));
		goto finish;
	}

Preserve errno in the macro wrapper itself, also preventing accidental
errno modifications in client specified SELINUX_CB_LOG callbacks.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/callbacks.h              | 3 +++
 libselinux/src/label_backends_android.c | 2 --
 libselinux/src/label_file.h             | 2 --
 libselinux/src/selinux_restorecon.c     | 6 +-----
 4 files changed, 4 insertions(+), 9 deletions(-)

Comments

James Carter May 12, 2022, 5:58 p.m. UTC | #1 
On Thu, May 12, 2022 at 12:02 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> selinux_log() is used in many error branches, where the caller might
> expect errno to bet set, e.g. label_file.c::lookup_all():
>
>     if (match_count) {
>                 *match_count = 0;
>                 result = calloc(data->nspec, sizeof(struct spec*));
>         } else {
>                 result = calloc(1, sizeof(struct spec*));
>         }
>         if (!result) {
>                 selinux_log(SELINUX_ERROR, "Failed to allocate %zu bytes of data\n",
>                             data->nspec * sizeof(struct spec*));
>                 goto finish;
>         }
>
> Preserve errno in the macro wrapper itself, also preventing accidental
> errno modifications in client specified SELINUX_CB_LOG callbacks.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libselinux/src/callbacks.h              | 3 +++
>  libselinux/src/label_backends_android.c | 2 --
>  libselinux/src/label_file.h             | 2 --
>  libselinux/src/selinux_restorecon.c     | 6 +-----
>  4 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/libselinux/src/callbacks.h b/libselinux/src/callbacks.h
> index f4dab157..5a4d0f8a 100644
> --- a/libselinux/src/callbacks.h
> +++ b/libselinux/src/callbacks.h
> @@ -5,6 +5,7 @@
>  #ifndef _SELINUX_CALLBACKS_H_
>  #define _SELINUX_CALLBACKS_H_
>
> +#include <errno.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -32,9 +33,11 @@ extern int
>  extern pthread_mutex_t log_mutex;
>
>  #define selinux_log(type, ...) do { \
> +       int saved_errno__ = errno; \
>         __pthread_mutex_lock(&log_mutex); \
>         selinux_log_direct(type, __VA_ARGS__); \
>         __pthread_mutex_unlock(&log_mutex); \
> +       errno = saved_errno__; \
>  } while(0)
>
>  #endif                         /* _SELINUX_CALLBACKS_H_ */
> diff --git a/libselinux/src/label_backends_android.c b/libselinux/src/label_backends_android.c
> index 66d4df2d..c2d78360 100644
> --- a/libselinux/src/label_backends_android.c
> +++ b/libselinux/src/label_backends_android.c
> @@ -93,7 +93,6 @@ static int process_line(struct selabel_handle *rec,
>
>         items = read_spec_entries(line_buf, &errbuf, 2, &prop, &context);
>         if (items < 0) {
> -               items = errno;
>                 if (errbuf) {
>                         selinux_log(SELINUX_ERROR,
>                                     "%s:  line %u error due to: %s\n", path,
> @@ -103,7 +102,6 @@ static int process_line(struct selabel_handle *rec,
>                                     "%s:  line %u error due to: %m\n", path,
>                                     lineno);
>                 }
> -               errno = items;
>                 return -1;
>         }
>
> diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
> index b453e13f..190bc175 100644
> --- a/libselinux/src/label_file.h
> +++ b/libselinux/src/label_file.h
> @@ -444,7 +444,6 @@ static inline int process_line(struct selabel_handle *rec,
>
>         items = read_spec_entries(line_buf, &errbuf, 3, &regex, &type, &context);
>         if (items < 0) {
> -               rc = errno;
>                 if (errbuf) {
>                         selinux_log(SELINUX_ERROR,
>                                     "%s:  line %u error due to: %s\n", path,
> @@ -454,7 +453,6 @@ static inline int process_line(struct selabel_handle *rec,
>                                     "%s:  line %u error due to: %m\n", path,
>                                     lineno);
>                 }
> -               errno = rc;
>                 return -1;
>         }
>
> diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
> index e6192912..ba7b3692 100644
> --- a/libselinux/src/selinux_restorecon.c
> +++ b/libselinux/src/selinux_restorecon.c
> @@ -1032,7 +1032,7 @@ static int selinux_restorecon_common(const char *pathname_orig,
>         struct stat sb;
>         char *pathname = NULL, *pathdnamer = NULL, *pathdname, *pathbname;
>         char *paths[2] = { NULL, NULL };
> -       int fts_flags, error, sverrno;
> +       int fts_flags, error;
>         struct dir_hash_node *current = NULL;
>
>         if (state.flags.verbose && state.flags.progress)
> @@ -1286,18 +1286,14 @@ cleanup:
>         return error;
>
>  oom:
> -       sverrno = errno;
>         selinux_log(SELINUX_ERROR, "%s:  Out of memory\n", __func__);
> -       errno = sverrno;
>         error = -1;
>         goto cleanup;
>
>  realpatherr:
> -       sverrno = errno;
>         selinux_log(SELINUX_ERROR,
>                     "SELinux: Could not get canonical path for %s restorecon: %m.\n",
>                     pathname_orig);
> -       errno = sverrno;
>         error = -1;
>         goto cleanup;
>
> --
> 2.36.1
>
James Carter May 16, 2022, 5:13 p.m. UTC | #2 
On Thu, May 12, 2022 at 1:58 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Thu, May 12, 2022 at 12:02 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > selinux_log() is used in many error branches, where the caller might
> > expect errno to bet set, e.g. label_file.c::lookup_all():
> >
> >     if (match_count) {
> >                 *match_count = 0;
> >                 result = calloc(data->nspec, sizeof(struct spec*));
> >         } else {
> >                 result = calloc(1, sizeof(struct spec*));
> >         }
> >         if (!result) {
> >                 selinux_log(SELINUX_ERROR, "Failed to allocate %zu bytes of data\n",
> >                             data->nspec * sizeof(struct spec*));
> >                 goto finish;
> >         }
> >
> > Preserve errno in the macro wrapper itself, also preventing accidental
> > errno modifications in client specified SELINUX_CB_LOG callbacks.
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> Acked-by: James Carter <jwcart2@gmail.com>
>

Merged.
Thanks,
Jim

> > ---
> >  libselinux/src/callbacks.h              | 3 +++
> >  libselinux/src/label_backends_android.c | 2 --
> >  libselinux/src/label_file.h             | 2 --
> >  libselinux/src/selinux_restorecon.c     | 6 +-----
> >  4 files changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/libselinux/src/callbacks.h b/libselinux/src/callbacks.h
> > index f4dab157..5a4d0f8a 100644
> > --- a/libselinux/src/callbacks.h
> > +++ b/libselinux/src/callbacks.h
> > @@ -5,6 +5,7 @@
> >  #ifndef _SELINUX_CALLBACKS_H_
> >  #define _SELINUX_CALLBACKS_H_
> >
> > +#include <errno.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > @@ -32,9 +33,11 @@ extern int
> >  extern pthread_mutex_t log_mutex;
> >
> >  #define selinux_log(type, ...) do { \
> > +       int saved_errno__ = errno; \
> >         __pthread_mutex_lock(&log_mutex); \
> >         selinux_log_direct(type, __VA_ARGS__); \
> >         __pthread_mutex_unlock(&log_mutex); \
> > +       errno = saved_errno__; \
> >  } while(0)
> >
> >  #endif                         /* _SELINUX_CALLBACKS_H_ */
> > diff --git a/libselinux/src/label_backends_android.c b/libselinux/src/label_backends_android.c
> > index 66d4df2d..c2d78360 100644
> > --- a/libselinux/src/label_backends_android.c
> > +++ b/libselinux/src/label_backends_android.c
> > @@ -93,7 +93,6 @@ static int process_line(struct selabel_handle *rec,
> >
> >         items = read_spec_entries(line_buf, &errbuf, 2, &prop, &context);
> >         if (items < 0) {
> > -               items = errno;
> >                 if (errbuf) {
> >                         selinux_log(SELINUX_ERROR,
> >                                     "%s:  line %u error due to: %s\n", path,
> > @@ -103,7 +102,6 @@ static int process_line(struct selabel_handle *rec,
> >                                     "%s:  line %u error due to: %m\n", path,
> >                                     lineno);
> >                 }
> > -               errno = items;
> >                 return -1;
> >         }
> >
> > diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
> > index b453e13f..190bc175 100644
> > --- a/libselinux/src/label_file.h
> > +++ b/libselinux/src/label_file.h
> > @@ -444,7 +444,6 @@ static inline int process_line(struct selabel_handle *rec,
> >
> >         items = read_spec_entries(line_buf, &errbuf, 3, &regex, &type, &context);
> >         if (items < 0) {
> > -               rc = errno;
> >                 if (errbuf) {
> >                         selinux_log(SELINUX_ERROR,
> >                                     "%s:  line %u error due to: %s\n", path,
> > @@ -454,7 +453,6 @@ static inline int process_line(struct selabel_handle *rec,
> >                                     "%s:  line %u error due to: %m\n", path,
> >                                     lineno);
> >                 }
> > -               errno = rc;
> >                 return -1;
> >         }
> >
> > diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
> > index e6192912..ba7b3692 100644
> > --- a/libselinux/src/selinux_restorecon.c
> > +++ b/libselinux/src/selinux_restorecon.c
> > @@ -1032,7 +1032,7 @@ static int selinux_restorecon_common(const char *pathname_orig,
> >         struct stat sb;
> >         char *pathname = NULL, *pathdnamer = NULL, *pathdname, *pathbname;
> >         char *paths[2] = { NULL, NULL };
> > -       int fts_flags, error, sverrno;
> > +       int fts_flags, error;
> >         struct dir_hash_node *current = NULL;
> >
> >         if (state.flags.verbose && state.flags.progress)
> > @@ -1286,18 +1286,14 @@ cleanup:
> >         return error;
> >
> >  oom:
> > -       sverrno = errno;
> >         selinux_log(SELINUX_ERROR, "%s:  Out of memory\n", __func__);
> > -       errno = sverrno;
> >         error = -1;
> >         goto cleanup;
> >
> >  realpatherr:
> > -       sverrno = errno;
> >         selinux_log(SELINUX_ERROR,
> >                     "SELinux: Could not get canonical path for %s restorecon: %m.\n",
> >                     pathname_orig);
> > -       errno = sverrno;
> >         error = -1;
> >         goto cleanup;
> >
> > --
> > 2.36.1
> >
diff mbox series

Patch

diff --git a/libselinux/src/callbacks.h b/libselinux/src/callbacks.h
index f4dab157..5a4d0f8a 100644
--- a/libselinux/src/callbacks.h
+++ b/libselinux/src/callbacks.h
@@ -5,6 +5,7 @@ 
 #ifndef _SELINUX_CALLBACKS_H_
 #define _SELINUX_CALLBACKS_H_
 
+#include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -32,9 +33,11 @@  extern int
 extern pthread_mutex_t log_mutex;
 
 #define selinux_log(type, ...) do { \
+	int saved_errno__ = errno; \
 	__pthread_mutex_lock(&log_mutex); \
 	selinux_log_direct(type, __VA_ARGS__); \
 	__pthread_mutex_unlock(&log_mutex); \
+	errno = saved_errno__; \
 } while(0)
 
 #endif				/* _SELINUX_CALLBACKS_H_ */
diff --git a/libselinux/src/label_backends_android.c b/libselinux/src/label_backends_android.c
index 66d4df2d..c2d78360 100644
--- a/libselinux/src/label_backends_android.c
+++ b/libselinux/src/label_backends_android.c
@@ -93,7 +93,6 @@  static int process_line(struct selabel_handle *rec,
 
 	items = read_spec_entries(line_buf, &errbuf, 2, &prop, &context);
 	if (items < 0) {
-		items = errno;
 		if (errbuf) {
 			selinux_log(SELINUX_ERROR,
 				    "%s:  line %u error due to: %s\n", path,
@@ -103,7 +102,6 @@  static int process_line(struct selabel_handle *rec,
 				    "%s:  line %u error due to: %m\n", path,
 				    lineno);
 		}
-		errno = items;
 		return -1;
 	}
 
diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
index b453e13f..190bc175 100644
--- a/libselinux/src/label_file.h
+++ b/libselinux/src/label_file.h
@@ -444,7 +444,6 @@  static inline int process_line(struct selabel_handle *rec,
 
 	items = read_spec_entries(line_buf, &errbuf, 3, &regex, &type, &context);
 	if (items < 0) {
-		rc = errno;
 		if (errbuf) {
 			selinux_log(SELINUX_ERROR,
 				    "%s:  line %u error due to: %s\n", path,
@@ -454,7 +453,6 @@  static inline int process_line(struct selabel_handle *rec,
 				    "%s:  line %u error due to: %m\n", path,
 				    lineno);
 		}
-		errno = rc;
 		return -1;
 	}
 
diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index e6192912..ba7b3692 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -1032,7 +1032,7 @@  static int selinux_restorecon_common(const char *pathname_orig,
 	struct stat sb;
 	char *pathname = NULL, *pathdnamer = NULL, *pathdname, *pathbname;
 	char *paths[2] = { NULL, NULL };
-	int fts_flags, error, sverrno;
+	int fts_flags, error;
 	struct dir_hash_node *current = NULL;
 
 	if (state.flags.verbose && state.flags.progress)
@@ -1286,18 +1286,14 @@  cleanup:
 	return error;
 
 oom:
-	sverrno = errno;
 	selinux_log(SELINUX_ERROR, "%s:  Out of memory\n", __func__);
-	errno = sverrno;
 	error = -1;
 	goto cleanup;
 
 realpatherr:
-	sverrno = errno;
 	selinux_log(SELINUX_ERROR,
 		    "SELinux: Could not get canonical path for %s restorecon: %m.\n",
 		    pathname_orig);
-	errno = sverrno;
 	error = -1;
 	goto cleanup;