Message ID | 20241105183936.252530-2-cgoettsche@seltendoof.de (mailing list archive) |
---|---|
State | Accepted |
Commit | 8efed460bc9f |
Delegated to: | Petr Lautrbach |
Headers | show |
Series | [1/2] libselinux: make use of calloc(3) | expand |
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 > >
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 --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,