On Thu, 5 Dec 2024 15:44:33 +0100
"Jerome Marchand" <jmarchan@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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;
|