[v7,4/8] bpftool: Ensure task comm is always NUL-terminated

Commit Message

Yafang Shao Aug. 17, 2024, 2:56 a.m. UTC

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 <[e-mail]>
Reviewed-by: Quentin Monnet <[e-mail]>
---
 tools/bpf/bpftool/pids.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Alejandro Colomar Aug. 17, 2024, 8:38 a.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Quentin Monnet <[e-mail]>
> ---
>  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
>

Yafang Shao Aug. 18, 2024, 2:27 a.m. UTC | #2

On Sat, Aug 17, 2024 at 4:39 PM Alejandro Colomar <[e-mail]> 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 <[e-mail]>
> > Reviewed-by: Quentin Monnet <[e-mail]>
> > ---
> >  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

Alejandro Colomar Aug. 18, 2024, 8:25 a.m. UTC | #3

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 <[e-mail]> 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 <[e-mail]>
> > > Reviewed-by: Quentin Monnet <[e-mail]>
> > > ---
> > >  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

Patch

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;