[4/6] tr2: fix memory leak & logic error in 2f732bf15e6

Commit Message

Ævar Arnfjörð Bjarmason Aug. 25, 2021, 11:19 p.m. UTC

In a subsequent commit I'll be replacing most of this code to log N
parents, but let's first fix bugs introduced in the recent
2f732bf15e6 (tr2: log parent process name, 2021-07-21).

It was using the strbuf_read_file() in the wrong way, its return value
is either a length or a negative value on error. If we didn't have a
procfs, or otherwise couldn't access it we'd end up pushing an empty
string to the trace2 ancestry array.

It was also using the strvec_push() API the wrong way. That API always
does an xstrdup(), so by detaching the strbuf here we'd leak
memory. Let's instead pass in our pointer for strvec_push() to
xstrdup(), and then free our own strbuf.

Furthermore we need to free that "procfs_path" strbuf whether or not
strbuf_read_file() succeeds, which was another source of memory leaks
in 2f732bf15e6, i.e. we'd leak that memory as well if we weren't on a
system where we could read the file from procfs.

In combination with the preceding commit this makes all of
t[0-9]*trace2*.sh pass under SANITIZE=leak on Linux.

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---
 compat/linux/procinfo.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Taylor Blau Aug. 26, 2021, 3:21 a.m. UTC | #1

On Thu, Aug 26, 2021 at 01:19:22AM +0200, Ævar Arnfjörð Bjarmason wrote:
> In a subsequent commit I'll be replacing most of this code to log N
> parents, but let's first fix bugs introduced in the recent
> 2f732bf15e6 (tr2: log parent process name, 2021-07-21).
>
> It was using the strbuf_read_file() in the wrong way, its return value
> is either a length or a negative value on error. If we didn't have a
> procfs, or otherwise couldn't access it we'd end up pushing an empty
> string to the trace2 ancestry array.

Sure, and I could believe that we didn't ever test the "couldn't open
/proc/pid/comm" case, which explains why this error didn't fail any
tests. I wondered whether it was worth writing this instead as:

    if (strbuf_read_file(...) < 0)
      warning(...);

before trimming and pushing the result onto "names", but we should
probably be swallowing this case and not presenting it as an error
(since it's completely acceptable to not be able to read from comm,
e.g., in versions of procfs on Linux older than 2.6.33).

> It was also using the strvec_push() API the wrong way. That API always
> does an xstrdup(), so by detaching the strbuf here we'd leak
> memory. Let's instead pass in our pointer for strvec_push() to
> xstrdup(), and then free our own strbuf.

Right, you could make strvec_push_nodup() non-static, but I don't think
that kind of churn is worthwhile.

> Furthermore we need to free that "procfs_path" strbuf whether or not
> strbuf_read_file() succeeds, which was another source of memory leaks
> in 2f732bf15e6, i.e. we'd leak that memory as well if we weren't on a
> system where we could read the file from procfs.

Hmm. It's completely correct to only call strbuf_release on names inside
of the body of the if statement, but it's extra work for the reader to
figure that out. Why not put both strbuf_release() calls next to each
other (immediately before the return) to make it more obvious (since
freeing the STRBUF_INIT value is a noop)?

Thanks,
Taylor

Patch

diff --git a/compat/linux/procinfo.c b/compat/linux/procinfo.c
index 62f8aaed4cc..c86daff2b26 100644
--- a/compat/linux/procinfo.c
+++ b/compat/linux/procinfo.c
@@ -18,12 +18,13 @@  static void get_ancestry_names(struct strvec *names)
 
 	/* try to use procfs if it's present. */
 	strbuf_addf(&procfs_path, "/proc/%d/comm", getppid());
-	if (strbuf_read_file(&name, procfs_path.buf, 0)) {
-		strbuf_release(&procfs_path);
+	if (strbuf_read_file(&name, procfs_path.buf, 0) > 0) {
 		strbuf_trim_trailing_newline(&name);
-		strvec_push(names, strbuf_detach(&name, NULL));
+		strvec_push(names, name.buf);
+		strbuf_release(&name);
 	}
 
+	strbuf_release(&procfs_path);
 	return;
 }