| Message ID | 20240817025624.13157-5-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Improve the copy of task comm | expand |
Hi Yafang, On Sat, Aug 17, 2024 at 10:56:20AM GMT, Yafang Shao wrote: > 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 <laoar.shao@gmail.com> > 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()? Isn't the source terminated? 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? > refs->ref_cnt++; > > return; > @@ -77,6 +78,7 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e) > ref = &refs->refs[0]; > ref->pid = e->pid; > memcpy(ref->comm, e->comm, sizeof(ref->comm)); > + ref->comm[sizeof(ref->comm) - 1] = '\0'; Same question here. > refs->ref_cnt = 1; > refs->has_bpf_cookie = e->has_bpf_cookie; > refs->bpf_cookie = e->bpf_cookie; > -- > 2.43.5 >
On Sat, Aug 17, 2024 at 4:39 PM Alejandro Colomar <alx@kernel.org> wrote: > > Hi Yafang, > > On Sat, Aug 17, 2024 at 10:56:20AM GMT, Yafang Shao wrote: > > 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 <laoar.shao@gmail.com> > > 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
Hi Yafang, On Sun, Aug 18, 2024 at 10:27:01AM GMT, Yafang Shao wrote: > On Sat, Aug 17, 2024 at 4:39 PM Alejandro Colomar <alx@kernel.org> wrote: > > > > Hi Yafang, > > > > On Sat, Aug 17, 2024 at 10:56:20AM GMT, Yafang Shao wrote: > > > 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 <laoar.shao@gmail.com> > > > 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? Ahh, makes sense. LGTM, then. Maybe the closest user-space function to strscpy(9) would be strlcpy(3), but I don't know how old of a glibc you support. strlcpy(3) is currently in POSIX, and supported by both glibc and musl, but that's too recent. Have a lovely day! Alex > -- > Regards > Yafang
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'; refs->ref_cnt++; return; @@ -77,6 +78,7 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e) ref = &refs->refs[0]; ref->pid = e->pid; memcpy(ref->comm, e->comm, sizeof(ref->comm)); + ref->comm[sizeof(ref->comm) - 1] = '\0'; refs->ref_cnt = 1; refs->has_bpf_cookie = e->has_bpf_cookie; refs->bpf_cookie = e->bpf_cookie;