| Message ID | 20230404031256.78330-1-jiangfeng@kylinos.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/mm: fix resource leaks in child_vmsplice_memcmp_fn | expand |
On 04.04.23 05:12, Feng Jiang wrote: > When the function returns, the 'new' and 'old' are not freed > and the 'fds[]' are not closed, which can lead to resource leaks. > > Signed-off-by: Feng Jiang <jiangfeng@kylinos.cn> > Suggested-by: Ming Xie <xieming@kylinos.cn> > --- > tools/testing/selftests/mm/cow.c | 43 ++++++++++++++++++++++++-------- > 1 file changed, 32 insertions(+), 11 deletions(-) > > diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c > index c0dd2dfca51b..b8aec05d56f4 100644 > --- a/tools/testing/selftests/mm/cow.c > +++ b/tools/testing/selftests/mm/cow.c > @@ -193,26 +193,38 @@ static int child_vmsplice_memcmp_fn(char *mem, size_t size, > char *old, *new; > int fds[2]; > char buf; > + int ret; > > old = malloc(size); > new = malloc(size); > + if (!old || !new) { > + ret = -ENOMEM; > + goto free; > + } > > /* Backup the original content. */ > memcpy(old, mem, size); > > - if (pipe(fds) < 0) > - return -errno; > + if (pipe(fds) < 0) { > + ret = -errno; > + goto free; > + } > > /* Trigger a read-only pin. */ > transferred = vmsplice(fds[1], &iov, 1, 0); > - if (transferred < 0) > - return -errno; > - if (transferred == 0) > - return -EINVAL; > + if (transferred < 0) { > + ret = -errno; > + goto close_pipe; > + } else if (transferred == 0) { > + ret = -EINVAL; > + goto close_pipe; > + } > > /* Unmap it from our page tables. */ > - if (munmap(mem, size) < 0) > - return -errno; > + if (munmap(mem, size) < 0) { > + ret = -errno; > + goto close_pipe; > + } > > /* Wait until the parent modified it. */ > write(comm_pipes->child_ready[1], "0", 1); > @@ -222,11 +234,20 @@ static int child_vmsplice_memcmp_fn(char *mem, size_t size, > /* See if we still read the old values via the pipe. */ > for (total = 0; total < transferred; total += cur) { > cur = read(fds[0], new + total, transferred - total); > - if (cur < 0) > - return -errno; > + if (cur < 0) { > + ret = -errno; > + goto close_pipe; > + } > } > > - return memcmp(old, new, transferred); > + ret = memcmp(old, new, transferred); > +close_pipe: > + close(fds[0]); > + close(fds[1]); > +free: > + free(old); > + free(new); > + return ret; > } > > typedef int (*child_fn)(char *mem, size_t size, struct comm_pipes *comm_pipes); NAK, the whole point of this function is that the child process will exit immediately after executing this function, cleaning up automatically.
diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c index c0dd2dfca51b..b8aec05d56f4 100644 --- a/tools/testing/selftests/mm/cow.c +++ b/tools/testing/selftests/mm/cow.c @@ -193,26 +193,38 @@ static int child_vmsplice_memcmp_fn(char *mem, size_t size, char *old, *new; int fds[2]; char buf; + int ret; old = malloc(size); new = malloc(size); + if (!old || !new) { + ret = -ENOMEM; + goto free; + } /* Backup the original content. */ memcpy(old, mem, size); - if (pipe(fds) < 0) - return -errno; + if (pipe(fds) < 0) { + ret = -errno; + goto free; + } /* Trigger a read-only pin. */ transferred = vmsplice(fds[1], &iov, 1, 0); - if (transferred < 0) - return -errno; - if (transferred == 0) - return -EINVAL; + if (transferred < 0) { + ret = -errno; + goto close_pipe; + } else if (transferred == 0) { + ret = -EINVAL; + goto close_pipe; + } /* Unmap it from our page tables. */ - if (munmap(mem, size) < 0) - return -errno; + if (munmap(mem, size) < 0) { + ret = -errno; + goto close_pipe; + } /* Wait until the parent modified it. */ write(comm_pipes->child_ready[1], "0", 1); @@ -222,11 +234,20 @@ static int child_vmsplice_memcmp_fn(char *mem, size_t size, /* See if we still read the old values via the pipe. */ for (total = 0; total < transferred; total += cur) { cur = read(fds[0], new + total, transferred - total); - if (cur < 0) - return -errno; + if (cur < 0) { + ret = -errno; + goto close_pipe; + } } - return memcmp(old, new, transferred); + ret = memcmp(old, new, transferred); +close_pipe: + close(fds[0]); + close(fds[1]); +free: + free(old); + free(new); + return ret; } typedef int (*child_fn)(char *mem, size_t size, struct comm_pipes *comm_pipes);
When the function returns, the 'new' and 'old' are not freed and the 'fds[]' are not closed, which can lead to resource leaks. Signed-off-by: Feng Jiang <jiangfeng@kylinos.cn> Suggested-by: Ming Xie <xieming@kylinos.cn> --- tools/testing/selftests/mm/cow.c | 43 ++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 11 deletions(-)