| Message ID | 20241205144439.127564-2-jmarchan@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | trace-cmd: Fix misc issues uncoverd by static analysis | expand |
On Thu, 5 Dec 2024 15:44:33 +0100 "Jerome Marchand" <jmarchan@redhat.com> wrote: > The behavior of create_buffer_recorder_fd2() wrt closing the file > descriptors is inconsistent. They aren't close if the function fails > early when allocating recorder, but they are closed in > tracecmd_free_recorder() if it fails later. > > This cause use-after-free access when the caller tries to close the > FDs afterwards. > > Always close the FDs in create_buffer_recorder_fd2() when it fails and > stop the caller to close them themselves. > > Signed-off-by: Jerome Marchand <jmarchan@redhat.com> > --- > lib/trace-cmd/trace-recorder.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/lib/trace-cmd/trace-recorder.c b/lib/trace-cmd/trace-recorder.c > index 44f245d5..0413e529 100644 > --- a/lib/trace-cmd/trace-recorder.c > +++ b/lib/trace-cmd/trace-recorder.c > @@ -114,8 +114,12 @@ create_buffer_recorder_fd2(int fd, int fd2, int cpu, unsigned flags, > bool nonblock = false; > > recorder = malloc(sizeof(*recorder)); > - if (!recorder) > + if (!recorder) { > + close(fd); > + if (fd2 != -1) > + close(fd2); > return NULL; > + } No. If a function does not open a file descriptor, it should not close it. It's bad programming style if a called function closes a file descriptor on error that was passed to it. That can easily introduce new bugs. The fix is to not have it close a file descriptor at all! -- Steve diff --git a/lib/trace-cmd/trace-recorder.c b/lib/trace-cmd/trace-recorder.c index 0413e5293766..df971e4adf97 100644 --- a/lib/trace-cmd/trace-recorder.c +++ b/lib/trace-cmd/trace-recorder.c @@ -69,7 +69,7 @@ void tracecmd_free_recorder(struct tracecmd_recorder *recorder) if (!recorder) return; - if (recorder->max) { + if (recorder->max && recorder->fd >= 0) { /* Need to put everything into fd1 */ if (recorder->fd == recorder->fd1) { int ret; @@ -140,14 +140,12 @@ create_buffer_recorder_fd2(int fd, int fd2, int cpu, unsigned flags, recorder->count = 0; recorder->pages = 0; - /* fd always points to what to write to */ - recorder->fd = fd; - recorder->fd1 = fd; - recorder->fd2 = fd2; - if (recorder->flags & TRACECMD_RECORD_POLL) nonblock = true; + /* In case of error */ + recorder->fd = -1; + if (tfd >= 0) recorder->tcpu = tracefs_cpu_alloc_fd(tfd, recorder->page_size, nonblock); else @@ -156,6 +154,11 @@ create_buffer_recorder_fd2(int fd, int fd2, int cpu, unsigned flags, if (!recorder->tcpu) goto out_free; + /* fd always points to what to write to */ + recorder->fd = fd; + recorder->fd1 = fd; + recorder->fd2 = fd2; + recorder->subbuf_size = tracefs_cpu_read_size(recorder->tcpu); return recorder;
On Wed, 18 Dec 2024 17:37:31 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > No. If a function does not open a file descriptor, it should not close it. > It's bad programming style if a called function closes a file descriptor on > error that was passed to it. That can easily introduce new bugs. > > The fix is to not have it close a file descriptor at all! And this means all the callers should close their fd if the recorder fails. It should have never done that in the first place. :-/ Oh well, that was written over 10 years ago ;-) -- Steve
diff --git a/lib/trace-cmd/trace-recorder.c b/lib/trace-cmd/trace-recorder.c index 44f245d5..0413e529 100644 --- a/lib/trace-cmd/trace-recorder.c +++ b/lib/trace-cmd/trace-recorder.c @@ -114,8 +114,12 @@ create_buffer_recorder_fd2(int fd, int fd2, int cpu, unsigned flags, bool nonblock = false; recorder = malloc(sizeof(*recorder)); - if (!recorder) + if (!recorder) { + close(fd); + if (fd2 != -1) + close(fd2); return NULL; + } recorder->flags = flags; @@ -204,12 +208,8 @@ __tracecmd_create_buffer_recorder(const char *file, int cpu, unsigned flags, } recorder = create_buffer_recorder_fd2(fd, fd2, cpu, flags, instance, maxkb, tfd); - if (!recorder) { - close(fd); + if (!recorder) unlink(file); - if (fd2 != -1) - close(fd2); - } if (fd2 != -1) { /* Unlink file2, we need to add everything to file at the end */ @@ -257,10 +257,9 @@ tracecmd_create_buffer_recorder_maxkb(const char *file, int cpu, unsigned flags, free(file2); return recorder; - err2: - close(fd2); err: close(fd); + err2: unlink(file); goto out; }
The behavior of create_buffer_recorder_fd2() wrt closing the file descriptors is inconsistent. They aren't close if the function fails early when allocating recorder, but they are closed in tracecmd_free_recorder() if it fails later. This cause use-after-free access when the caller tries to close the FDs afterwards. Always close the FDs in create_buffer_recorder_fd2() when it fails and stop the caller to close them themselves. Signed-off-by: Jerome Marchand <jmarchan@redhat.com> --- lib/trace-cmd/trace-recorder.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)