| Message ID | patch-v2-4.6-1aa0dbc394e-20210826T121820Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tr2: plug memory leaks + logic errors + Win32 & Linux feature parity | expand |
On Thu, Aug 26, 2021 at 8:22 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > [...] > Let's move all the freeing of the memory to the end of the > function. If we're still at STRBUF_INIT with "name" due to not haven s/haven/having/ > taken the branch where the strbuf_read_file() succeeds freeing it is > redundant, so we could move it into the body of the "if", but just > handling freeing the same way for all branches of the function makes > it more readable. > [...] > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Subject: Re: [PATCH v2 4/6] tr2: fix memory leak & logic error in 2f732bf15e6 Please remember to write your commit titles to help readers of "git shortlog". This commit fixes get_ancestry_names() function. get_ancestry_names(): leave the parent list empty upon failure or something like that. The rest of the proposed log message looks perfect and so do the changes to the code. > 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. I do have some WIP changes to > make strvec_push_nodup() non-static, which makes this and some other > callsites nicer, but let's just follow the prevailing pattern of using > strvec_push() for now. > > We'll also 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. > > Let's move all the freeing of the memory to the end of the > function. If we're still at STRBUF_INIT with "name" due to not haven > taken the branch where the strbuf_read_file() succeeds freeing it is > redundant, so we could move it into the body of the "if", but just > handling freeing the same way for all branches of the function makes > it more readable. > > 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 <avarab@gmail.com> > --- > compat/linux/procinfo.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/compat/linux/procinfo.c b/compat/linux/procinfo.c > index 62f8aaed4cc..bd01f017bc7 100644 > --- a/compat/linux/procinfo.c > +++ b/compat/linux/procinfo.c > @@ -18,12 +18,14 @@ 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(&procfs_path); > + strbuf_release(&name); > + > return; > }
diff --git a/compat/linux/procinfo.c b/compat/linux/procinfo.c index 62f8aaed4cc..bd01f017bc7 100644 --- a/compat/linux/procinfo.c +++ b/compat/linux/procinfo.c @@ -18,12 +18,14 @@ 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(&procfs_path); + strbuf_release(&name); + return; }
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. I do have some WIP changes to make strvec_push_nodup() non-static, which makes this and some other callsites nicer, but let's just follow the prevailing pattern of using strvec_push() for now. We'll also 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. Let's move all the freeing of the memory to the end of the function. If we're still at STRBUF_INIT with "name" due to not haven taken the branch where the strbuf_read_file() succeeds freeing it is redundant, so we could move it into the body of the "if", but just handling freeing the same way for all branches of the function makes it more readable. 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 <avarab@gmail.com> --- compat/linux/procinfo.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)