diff mbox series

[2/2] libselinux: avoid dynamic allocation in openattr()

Message ID 20241105183936.252530-2-cgoettsche@seltendoof.de (mailing list archive)
State New
Delegated to: Petr Lautrbach
Headers show
Series [1/2] libselinux: make use of calloc(3) | expand

Commit Message

Christian Göttsche Nov. 5, 2024, 6:39 p.m. UTC 
From: Christian Göttsche <cgzones@googlemail.com>

openattr() supplies the simplementation for the getcon(3) interface
family.  Use a short local buffer instead of descend into memory
allocation.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/procattr.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Comments

James Carter Nov. 6, 2024, 9:42 p.m. UTC | #1 
On Tue, Nov 5, 2024 at 1:46 PM Christian Göttsche
<cgoettsche@seltendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@googlemail.com>
>
> openattr() supplies the simplementation for the getcon(3) interface

implementation

> family.  Use a short local buffer instead of descend into memory

"Use a short local buffer instead of dynamic memory allocation"?

> allocation.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libselinux/src/procattr.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
> index ddcc7f8d..ee1f48af 100644
> --- a/libselinux/src/procattr.c
> +++ b/libselinux/src/procattr.c
> @@ -86,32 +86,32 @@ static void init_procattr(void)
>  static int openattr(pid_t pid, const char *attr, int flags)
>  {
>         int fd, rc;
> -       char *path;
> +       char path[56];  /* must hold "/proc/self/task/%d/attr/sockcreate" */

Why 56? I understand that sockcreate is the largest attr, but it looks
to me like you are giving way more space than is needed for the pid. I
am just curious. Maybe I am missing something.

Thanks,
Jim


>         pid_t tid;
>
>         if (pid > 0) {
> -               rc = asprintf(&path, "/proc/%d/attr/%s", pid, attr);
> +               rc = snprintf(path, sizeof(path), "/proc/%d/attr/%s", pid, attr);
>         } else if (pid == 0) {
> -               rc = asprintf(&path, "/proc/thread-self/attr/%s", attr);
> -               if (rc < 0)
> +               rc = snprintf(path, sizeof(path), "/proc/thread-self/attr/%s", attr);
> +               if (rc < 0 || (size_t)rc >= sizeof(path)) {
> +                       errno = EOVERFLOW;
>                         return -1;
> +               }
>                 fd = open(path, flags | O_CLOEXEC);
>                 if (fd >= 0 || errno != ENOENT)
> -                       goto out;
> -               free(path);
> +                       return fd;
>                 tid = selinux_gettid();
> -               rc = asprintf(&path, "/proc/self/task/%d/attr/%s", tid, attr);
> +               rc = snprintf(path, sizeof(path), "/proc/self/task/%d/attr/%s", tid, attr);
>         } else {
>                 errno = EINVAL;
>                 return -1;
>         }
> -       if (rc < 0)
> +       if (rc < 0 || (size_t)rc >= sizeof(path)) {
> +               errno = EOVERFLOW;
>                 return -1;
> +       }
>
> -       fd = open(path, flags | O_CLOEXEC);
> -out:
> -       free(path);
> -       return fd;
> +       return open(path, flags | O_CLOEXEC);
>  }
>
>  static int getprocattrcon_raw(char **context, pid_t pid, const char *attr,
> --
> 2.45.2
>
>
Christian Göttsche Nov. 11, 2024, 1:57 p.m. UTC | #2 
On Wed, 6 Nov 2024 at 22:42, James Carter <jwcart2@gmail.com> wrote:
>
> On Tue, Nov 5, 2024 at 1:46 PM Christian Göttsche
> <cgoettsche@seltendoof.de> wrote:
> >
> > From: Christian Göttsche <cgzones@googlemail.com>
> >
> > openattr() supplies the simplementation for the getcon(3) interface
>
> implementation

Tkanks, I'll fix in v2.

> > family.  Use a short local buffer instead of descend into memory
>
> "Use a short local buffer instead of dynamic memory allocation"?

Dito

> > allocation.
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > ---
> >  libselinux/src/procattr.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
> > index ddcc7f8d..ee1f48af 100644
> > --- a/libselinux/src/procattr.c
> > +++ b/libselinux/src/procattr.c
> > @@ -86,32 +86,32 @@ static void init_procattr(void)
> >  static int openattr(pid_t pid, const char *attr, int flags)
> >  {
> >         int fd, rc;
> > -       char *path;
> > +       char path[56];  /* must hold "/proc/self/task/%d/attr/sockcreate" */
>
> Why 56? I understand that sockcreate is the largest attr, but it looks
> to me like you are giving way more space than is needed for the pid. I
> am just curious. Maybe I am missing something.

My calculation was:
  32 character for the length of "/proc/self/task//attr/sockcreate"
  +1 for NUL terminator
  +20 for the unlikely case of pid_t being ULONG_MAX or LONG_MIN
  round to next multiple of 8

> Thanks,
> Jim
>
>
> >         pid_t tid;
> >
> >         if (pid > 0) {
> > -               rc = asprintf(&path, "/proc/%d/attr/%s", pid, attr);
> > +               rc = snprintf(path, sizeof(path), "/proc/%d/attr/%s", pid, attr);
> >         } else if (pid == 0) {
> > -               rc = asprintf(&path, "/proc/thread-self/attr/%s", attr);
> > -               if (rc < 0)
> > +               rc = snprintf(path, sizeof(path), "/proc/thread-self/attr/%s", attr);
> > +               if (rc < 0 || (size_t)rc >= sizeof(path)) {
> > +                       errno = EOVERFLOW;
> >                         return -1;
> > +               }
> >                 fd = open(path, flags | O_CLOEXEC);
> >                 if (fd >= 0 || errno != ENOENT)
> > -                       goto out;
> > -               free(path);
> > +                       return fd;
> >                 tid = selinux_gettid();
> > -               rc = asprintf(&path, "/proc/self/task/%d/attr/%s", tid, attr);
> > +               rc = snprintf(path, sizeof(path), "/proc/self/task/%d/attr/%s", tid, attr);
> >         } else {
> >                 errno = EINVAL;
> >                 return -1;
> >         }
> > -       if (rc < 0)
> > +       if (rc < 0 || (size_t)rc >= sizeof(path)) {
> > +               errno = EOVERFLOW;
> >                 return -1;
> > +       }
> >
> > -       fd = open(path, flags | O_CLOEXEC);
> > -out:
> > -       free(path);
> > -       return fd;
> > +       return open(path, flags | O_CLOEXEC);
> >  }
> >
> >  static int getprocattrcon_raw(char **context, pid_t pid, const char *attr,
> > --
> > 2.45.2
> >
> >
diff mbox series

Patch

diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
index ddcc7f8d..ee1f48af 100644
--- a/libselinux/src/procattr.c
+++ b/libselinux/src/procattr.c
@@ -86,32 +86,32 @@  static void init_procattr(void)
 static int openattr(pid_t pid, const char *attr, int flags)
 {
 	int fd, rc;
-	char *path;
+	char path[56];  /* must hold "/proc/self/task/%d/attr/sockcreate" */
 	pid_t tid;
 
 	if (pid > 0) {
-		rc = asprintf(&path, "/proc/%d/attr/%s", pid, attr);
+		rc = snprintf(path, sizeof(path), "/proc/%d/attr/%s", pid, attr);
 	} else if (pid == 0) {
-		rc = asprintf(&path, "/proc/thread-self/attr/%s", attr);
-		if (rc < 0)
+		rc = snprintf(path, sizeof(path), "/proc/thread-self/attr/%s", attr);
+		if (rc < 0 || (size_t)rc >= sizeof(path)) {
+			errno = EOVERFLOW;
 			return -1;
+		}
 		fd = open(path, flags | O_CLOEXEC);
 		if (fd >= 0 || errno != ENOENT)
-			goto out;
-		free(path);
+			return fd;
 		tid = selinux_gettid();
-		rc = asprintf(&path, "/proc/self/task/%d/attr/%s", tid, attr);
+		rc = snprintf(path, sizeof(path), "/proc/self/task/%d/attr/%s", tid, attr);
 	} else {
 		errno = EINVAL;
 		return -1;
 	}
-	if (rc < 0)
+	if (rc < 0 || (size_t)rc >= sizeof(path)) {
+		errno = EOVERFLOW;
 		return -1;
+	}
 
-	fd = open(path, flags | O_CLOEXEC);
-out:
-	free(path);
-	return fd;
+	return open(path, flags | O_CLOEXEC);
 }
 
 static int getprocattrcon_raw(char **context, pid_t pid, const char *attr,