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
