On Wed, 6 Nov 2024 at 22:42, James Carter <jwcart2@xxxxxxxxx> wrote:
>
> On Tue, Nov 5, 2024 at 1:46 PM Christian Göttsche
> <cgoettsche@xxxxxxxxxxxxx> wrote:
> >
> > From: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > ---
> > 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
> >
> >
