diff mbox series

[1/4] libselinux: avoid errno modification by fclose(3)

Message ID 20241018151300.445618-1-cgoettsche@seltendoof.de (mailing list archive)
State Accepted
Commit 6376f90d5e4d
Headers show
Series [1/4] libselinux: avoid errno modification by fclose(3) | expand

Commit Message

Christian Göttsche Oct. 18, 2024, 3:12 p.m. UTC 
From: Christian Göttsche <cgzones@googlemail.com>

In case fclose(3) might modify the global variable errno, use a wrapper
retaining the errno value.  In the affected cases the success of
fclose(3) itself is not important, since the underlying descriptor is
only read from.

Reported-by: clang-analyzer
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/get_context_list.c |  4 ++--
 libselinux/src/label_file.c       |  4 ++--
 libselinux/src/selinux_internal.h | 11 +++++++++++
 3 files changed, 15 insertions(+), 4 deletions(-)

Comments

James Carter Oct. 21, 2024, 7:30 p.m. UTC | #1 
On Fri, Oct 18, 2024 at 11:13 AM Christian Göttsche
<cgoettsche@seltendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@googlemail.com>
>
> In case fclose(3) might modify the global variable errno, use a wrapper
> retaining the errno value.  In the affected cases the success of
> fclose(3) itself is not important, since the underlying descriptor is
> only read from.
>
> Reported-by: clang-analyzer
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

For these four patches:
Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libselinux/src/get_context_list.c |  4 ++--
>  libselinux/src/label_file.c       |  4 ++--
>  libselinux/src/selinux_internal.h | 11 +++++++++++
>  3 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
> index 0ad24654..222b54c1 100644
> --- a/libselinux/src/get_context_list.c
> +++ b/libselinux/src/get_context_list.c
> @@ -438,7 +438,7 @@ int get_ordered_context_list(const char *user,
>                 __fsetlocking(fp, FSETLOCKING_BYCALLER);
>                 rc = get_context_user(fp, con, user, &reachable, &nreachable);
>
> -               fclose(fp);
> +               fclose_errno_safe(fp);
>                 if (rc < 0 && errno != ENOENT) {
>                         selinux_log(SELINUX_ERROR,
>                                 "%s:  error in processing configuration file %s\n",
> @@ -451,7 +451,7 @@ int get_ordered_context_list(const char *user,
>         if (fp) {
>                 __fsetlocking(fp, FSETLOCKING_BYCALLER);
>                 rc = get_context_user(fp, con, user, &reachable, &nreachable);
> -               fclose(fp);
> +               fclose_errno_safe(fp);
>                 if (rc < 0 && errno != ENOENT) {
>                         selinux_log(SELINUX_ERROR,
>                                 "%s:  error in processing configuration file %s\n",
> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> index 2fad0c93..40628e2c 100644
> --- a/libselinux/src/label_file.c
> +++ b/libselinux/src/label_file.c
> @@ -628,7 +628,7 @@ static int process_file(const char *path, const char *suffix,
>
>                 rc = fcontext_is_binary(fp);
>                 if (rc < 0) {
> -                       (void) fclose(fp);
> +                       fclose_errno_safe(fp);
>                         return -1;
>                 }
>
> @@ -639,7 +639,7 @@ static int process_file(const char *path, const char *suffix,
>                         rc = digest_add_specfile(digest, fp, NULL, sb.st_size,
>                                 found_path);
>
> -               fclose(fp);
> +               fclose_errno_safe(fp);
>
>                 if (!rc)
>                         return 0;
> diff --git a/libselinux/src/selinux_internal.h b/libselinux/src/selinux_internal.h
> index 450a42c2..372837dd 100644
> --- a/libselinux/src/selinux_internal.h
> +++ b/libselinux/src/selinux_internal.h
> @@ -2,7 +2,9 @@
>  #define SELINUX_INTERNAL_H_
>
>  #include <selinux/selinux.h>
> +#include <errno.h>
>  #include <pthread.h>
> +#include <stdio.h>
>
>
>  extern int require_seusers ;
> @@ -131,4 +133,13 @@ void *reallocarray(void *ptr, size_t nmemb, size_t size);
>  #define IGNORE_DEPRECATED_DECLARATION_END
>  #endif
>
> +static inline void fclose_errno_safe(FILE *stream)
> +{
> +       int saved_errno;
> +
> +       saved_errno = errno;
> +       (void) fclose(stream);
> +       errno = saved_errno;
> +}
> +
>  #endif /* SELINUX_INTERNAL_H_ */
> --
> 2.45.2
>
>
James Carter Oct. 30, 2024, 1:13 p.m. UTC | #2 
On Mon, Oct 21, 2024 at 3:30 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Fri, Oct 18, 2024 at 11:13 AM Christian Göttsche
> <cgoettsche@seltendoof.de> wrote:
> >
> > From: Christian Göttsche <cgzones@googlemail.com>
> >
> > In case fclose(3) might modify the global variable errno, use a wrapper
> > retaining the errno value.  In the affected cases the success of
> > fclose(3) itself is not important, since the underlying descriptor is
> > only read from.
> >
> > Reported-by: clang-analyzer
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> For these four patches:
> Acked-by: James Carter <jwcart2@gmail.com>
>

These four patches have been merged.
Thanks,
Jim

> > ---
> >  libselinux/src/get_context_list.c |  4 ++--
> >  libselinux/src/label_file.c       |  4 ++--
> >  libselinux/src/selinux_internal.h | 11 +++++++++++
> >  3 files changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
> > index 0ad24654..222b54c1 100644
> > --- a/libselinux/src/get_context_list.c
> > +++ b/libselinux/src/get_context_list.c
> > @@ -438,7 +438,7 @@ int get_ordered_context_list(const char *user,
> >                 __fsetlocking(fp, FSETLOCKING_BYCALLER);
> >                 rc = get_context_user(fp, con, user, &reachable, &nreachable);
> >
> > -               fclose(fp);
> > +               fclose_errno_safe(fp);
> >                 if (rc < 0 && errno != ENOENT) {
> >                         selinux_log(SELINUX_ERROR,
> >                                 "%s:  error in processing configuration file %s\n",
> > @@ -451,7 +451,7 @@ int get_ordered_context_list(const char *user,
> >         if (fp) {
> >                 __fsetlocking(fp, FSETLOCKING_BYCALLER);
> >                 rc = get_context_user(fp, con, user, &reachable, &nreachable);
> > -               fclose(fp);
> > +               fclose_errno_safe(fp);
> >                 if (rc < 0 && errno != ENOENT) {
> >                         selinux_log(SELINUX_ERROR,
> >                                 "%s:  error in processing configuration file %s\n",
> > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > index 2fad0c93..40628e2c 100644
> > --- a/libselinux/src/label_file.c
> > +++ b/libselinux/src/label_file.c
> > @@ -628,7 +628,7 @@ static int process_file(const char *path, const char *suffix,
> >
> >                 rc = fcontext_is_binary(fp);
> >                 if (rc < 0) {
> > -                       (void) fclose(fp);
> > +                       fclose_errno_safe(fp);
> >                         return -1;
> >                 }
> >
> > @@ -639,7 +639,7 @@ static int process_file(const char *path, const char *suffix,
> >                         rc = digest_add_specfile(digest, fp, NULL, sb.st_size,
> >                                 found_path);
> >
> > -               fclose(fp);
> > +               fclose_errno_safe(fp);
> >
> >                 if (!rc)
> >                         return 0;
> > diff --git a/libselinux/src/selinux_internal.h b/libselinux/src/selinux_internal.h
> > index 450a42c2..372837dd 100644
> > --- a/libselinux/src/selinux_internal.h
> > +++ b/libselinux/src/selinux_internal.h
> > @@ -2,7 +2,9 @@
> >  #define SELINUX_INTERNAL_H_
> >
> >  #include <selinux/selinux.h>
> > +#include <errno.h>
> >  #include <pthread.h>
> > +#include <stdio.h>
> >
> >
> >  extern int require_seusers ;
> > @@ -131,4 +133,13 @@ void *reallocarray(void *ptr, size_t nmemb, size_t size);
> >  #define IGNORE_DEPRECATED_DECLARATION_END
> >  #endif
> >
> > +static inline void fclose_errno_safe(FILE *stream)
> > +{
> > +       int saved_errno;
> > +
> > +       saved_errno = errno;
> > +       (void) fclose(stream);
> > +       errno = saved_errno;
> > +}
> > +
> >  #endif /* SELINUX_INTERNAL_H_ */
> > --
> > 2.45.2
> >
> >
diff mbox series

Patch

diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
index 0ad24654..222b54c1 100644
--- a/libselinux/src/get_context_list.c
+++ b/libselinux/src/get_context_list.c
@@ -438,7 +438,7 @@  int get_ordered_context_list(const char *user,
 		__fsetlocking(fp, FSETLOCKING_BYCALLER);
 		rc = get_context_user(fp, con, user, &reachable, &nreachable);
 
-		fclose(fp);
+		fclose_errno_safe(fp);
 		if (rc < 0 && errno != ENOENT) {
 			selinux_log(SELINUX_ERROR,
 				"%s:  error in processing configuration file %s\n",
@@ -451,7 +451,7 @@  int get_ordered_context_list(const char *user,
 	if (fp) {
 		__fsetlocking(fp, FSETLOCKING_BYCALLER);
 		rc = get_context_user(fp, con, user, &reachable, &nreachable);
-		fclose(fp);
+		fclose_errno_safe(fp);
 		if (rc < 0 && errno != ENOENT) {
 			selinux_log(SELINUX_ERROR,
 				"%s:  error in processing configuration file %s\n",
diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index 2fad0c93..40628e2c 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -628,7 +628,7 @@  static int process_file(const char *path, const char *suffix,
 
 		rc = fcontext_is_binary(fp);
 		if (rc < 0) {
-			(void) fclose(fp);
+			fclose_errno_safe(fp);
 			return -1;
 		}
 
@@ -639,7 +639,7 @@  static int process_file(const char *path, const char *suffix,
 			rc = digest_add_specfile(digest, fp, NULL, sb.st_size,
 				found_path);
 
-		fclose(fp);
+		fclose_errno_safe(fp);
 
 		if (!rc)
 			return 0;
diff --git a/libselinux/src/selinux_internal.h b/libselinux/src/selinux_internal.h
index 450a42c2..372837dd 100644
--- a/libselinux/src/selinux_internal.h
+++ b/libselinux/src/selinux_internal.h
@@ -2,7 +2,9 @@ 
 #define SELINUX_INTERNAL_H_
 
 #include <selinux/selinux.h>
+#include <errno.h>
 #include <pthread.h>
+#include <stdio.h>
 
 
 extern int require_seusers ;
@@ -131,4 +133,13 @@  void *reallocarray(void *ptr, size_t nmemb, size_t size);
 #define IGNORE_DEPRECATED_DECLARATION_END
 #endif
 
+static inline void fclose_errno_safe(FILE *stream)
+{
+	int saved_errno;
+
+	saved_errno = errno;
+	(void) fclose(stream);
+	errno = saved_errno;
+}
+
 #endif /* SELINUX_INTERNAL_H_ */