On Sat, Aug 17, 2024 at 4:39 PM Alejandro Colomar [off-list ref] wrote:

Hi Yafang,

On Sat, Aug 17, 2024 at 10:56:20AM GMT, Yafang Shao wrote:

quoted

Let's explicitly ensure the destination string is NUL-terminated. This way,
it won't be affected by changes to the source string.

Signed-off-by: Yafang Shao <redacted>
Reviewed-by: Quentin Monnet <qmo@kernel.org>
---
 tools/bpf/bpftool/pids.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
index 9b898571b49e..23f488cf1740 100644
--- a/tools/bpf/bpftool/pids.c
+++ b/tools/bpf/bpftool/pids.c

@@ -54,6 +54,7 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
              ref = &refs->refs[refs->ref_cnt];
              ref->pid = e->pid;
              memcpy(ref->comm, e->comm, sizeof(ref->comm));
+             ref->comm[sizeof(ref->comm) - 1] = '\0';

Why doesn't this use strscpy()?

bpftool is a userspace tool, so strscpy() is only applicable in kernel
code, correct?

Isn't the source terminated?

It is currently terminated, but I believe we should avoid relying on
the source. Making it independent of the source would reduce potential
errors.

Both the source and the destination measure 16 characters.  If it is
true that the source is not terminated, then this copy might truncate
the (non-)string by overwriting the last byte with a NUL.  Is that
truncation a good thing?

It's not ideal, but we should still convert it to a string, even if it
ends up being truncated.

--
Regards
Yafang