From: Rong Tao <[email protected]>
Compile samples/bpf, error:
$ cd samples/bpf
$ make
...
In function ‘__enable_controllers’:
samples/bpf/../../tools/testing/selftests/bpf/cgroup_helpers.c:80:17: warning: ‘strncpy’ specified bound 4097 equals destination size [-Wstringop-truncation]
80 | strncpy(enable, controllers, sizeof(enable));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Rong Tao <[email protected]>
---
tools/testing/selftests/bpf/cgroup_helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
index e914cc45b766..a70e873b267e 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.c
+++ b/tools/testing/selftests/bpf/cgroup_helpers.c
@@ -77,7 +77,7 @@ static int __enable_controllers(const char *cgroup_path, const char *controllers
enable[len] = 0;
close(fd);
} else {
- strncpy(enable, controllers, sizeof(enable));
+ strncpy(enable, controllers, sizeof(enable) - 1);
}
snprintf(path, sizeof(path), "%s/cgroup.subtree_control", cgroup_path);
--
2.31.1
On Thu, Oct 27, 2022 at 4:34 AM Rong Tao <[email protected]> wrote:
>
> From: Rong Tao <[email protected]>
>
> Compile samples/bpf, error:
> $ cd samples/bpf
> $ make
> ...
> In function ‘__enable_controllers’:
> samples/bpf/../../tools/testing/selftests/bpf/cgroup_helpers.c:80:17: warning: ‘strncpy’ specified bound 4097 equals destination size [-Wstringop-truncation]
> 80 | strncpy(enable, controllers, sizeof(enable));
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Rong Tao <[email protected]>
> ---
> tools/testing/selftests/bpf/cgroup_helpers.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
> index e914cc45b766..a70e873b267e 100644
> --- a/tools/testing/selftests/bpf/cgroup_helpers.c
> +++ b/tools/testing/selftests/bpf/cgroup_helpers.c
> @@ -77,7 +77,7 @@ static int __enable_controllers(const char *cgroup_path, const char *controllers
> enable[len] = 0;
> close(fd);
> } else {
> - strncpy(enable, controllers, sizeof(enable));
> + strncpy(enable, controllers, sizeof(enable) - 1);
enable is not initialized, so we might end up with non-zero-terminated
string. Let's enable[0] = '\0'; at the beginning and then strncat()
here?
> }
>
> snprintf(path, sizeof(path), "%s/cgroup.subtree_control", cgroup_path);
> --
> 2.31.1
>
Thanks for your reply, `enable[0] = '\0';` at the beginning and then
strncat() still has the same compile warning
--- a/tools/testing/selftests/bpf/cgroup_helpers.c
+++ b/tools/testing/selftests/bpf/cgroup_helpers.c
@@ -77,7 +77,8 @@ static int __enable_controllers(const char *cgroup_path, const char *controllers
enable[len] = 0;
close(fd);
} else {
- strncpy(enable, controllers, sizeof(enable));
+ enable[0] = '\0';
+ strncat(enable, controllers, sizeof(enable));
}
In function ‘__enable_controllers’:
tools/testing/selftests/bpf/cgroup_helpers.c:81:17: warning: ‘strncat’ specified bound 4097 equals destination size [-Wstringop-truncation]
81 | strncat(enable, controllers, sizeof(enable));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/testing/selftests/bpf/cgroup_helpers.c:81:17: warning: ‘strncat’ specified bound 4097 equals destination size [-Wstringop-overflow=]
So, i think just add '-1' for strncpy() is a good way.
On Thu, Oct 27, 2022 at 5:26 PM Rong Tao <[email protected]> wrote:
>
> Thanks for your reply, `enable[0] = '\0';` at the beginning and then
> strncat() still has the same compile warning
>
> --- a/tools/testing/selftests/bpf/cgroup_helpers.c
> +++ b/tools/testing/selftests/bpf/cgroup_helpers.c
> @@ -77,7 +77,8 @@ static int __enable_controllers(const char *cgroup_path, const char *controllers
> enable[len] = 0;
> close(fd);
> } else {
> - strncpy(enable, controllers, sizeof(enable));
> + enable[0] = '\0';
> + strncat(enable, controllers, sizeof(enable));
> }
>
> In function ‘__enable_controllers’:
> tools/testing/selftests/bpf/cgroup_helpers.c:81:17: warning: ‘strncat’ specified bound 4097 equals destination size [-Wstringop-truncation]
> 81 | strncat(enable, controllers, sizeof(enable));
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> tools/testing/selftests/bpf/cgroup_helpers.c:81:17: warning: ‘strncat’ specified bound 4097 equals destination size [-Wstringop-overflow=]
>
> So, i think just add '-1' for strncpy() is a good way.
no, it's not, see my previous email about ending up with
non-zero-terminated C string.
check strncat() API, it leaves the dst string zero terminated, and
yes, you need -1 for strncat as well, your compiler is right
From: Rong Tao <[email protected]>
Replace strncpy() with strncat(), strncat() leaves the dst string zero
terminated. Compile samples/bpf warning:
$ cd samples/bpf
$ make
...
In function ‘__enable_controllers’:
samples/bpf/../../tools/testing/selftests/bpf/cgroup_helpers.c:80:17: warning: ‘strncpy’ specified bound 4097 equals destination size [-Wstringop-truncation]
80 | strncpy(enable, controllers, sizeof(enable));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Rong Tao <[email protected]>
---
tools/testing/selftests/bpf/cgroup_helpers.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
index a70e873b267e..912e6522c7c5 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.c
+++ b/tools/testing/selftests/bpf/cgroup_helpers.c
@@ -77,7 +77,8 @@ static int __enable_controllers(const char *cgroup_path, const char *controllers
enable[len] = 0;
close(fd);
} else {
- strncpy(enable, controllers, sizeof(enable) - 1);
+ enable[0] = '\0';
+ strncat(enable, controllers, sizeof(enable) - 1);
}
snprintf(path, sizeof(path), "%s/cgroup.subtree_control", cgroup_path);
--
2.31.1
From: Rong Tao <[email protected]>
Replace strncpy() with strncat(), strncat() leaves the dst string zero
terminated. Compile samples/bpf warning:
$ cd samples/bpf
$ make
...
In function ‘__enable_controllers’:
samples/bpf/../../tools/testing/selftests/bpf/cgroup_helpers.c:80:17: warning: ‘strncpy’ specified bound 4097 equals destination size [-Wstringop-truncation]
80 | strncpy(enable, controllers, sizeof(enable));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Rong Tao <[email protected]>
---
tools/testing/selftests/bpf/cgroup_helpers.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
index e914cc45b766..912e6522c7c5 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.c
+++ b/tools/testing/selftests/bpf/cgroup_helpers.c
@@ -77,7 +77,8 @@ static int __enable_controllers(const char *cgroup_path, const char *controllers
enable[len] = 0;
close(fd);
} else {
- strncpy(enable, controllers, sizeof(enable));
+ enable[0] = '\0';
+ strncat(enable, controllers, sizeof(enable) - 1);
}
snprintf(path, sizeof(path), "%s/cgroup.subtree_control", cgroup_path);
--
2.31.1
From: Rong Tao
> Sent: 29 October 2022 04:00
>
> From: Rong Tao <[email protected]>
>
> Replace strncpy() with strncat(), strncat() leaves the dst string zero
> terminated. Compile samples/bpf warning:
This only makes a difference because tests for strncat() have not been added.
srtncat() really is never the string function you are looking for.
David
>
> $ cd samples/bpf
> $ make
> ...
> In function ‘__enable_controllers’:
> samples/bpf/../../tools/testing/selftests/bpf/cgroup_helpers.c:80:17: warning: ‘strncpy’ specified
> bound 4097 equals destination size [-Wstringop-truncation]
> 80 | strncpy(enable, controllers, sizeof(enable));
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Rong Tao <[email protected]>
> ---
> tools/testing/selftests/bpf/cgroup_helpers.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c
> b/tools/testing/selftests/bpf/cgroup_helpers.c
> index a70e873b267e..912e6522c7c5 100644
> --- a/tools/testing/selftests/bpf/cgroup_helpers.c
> +++ b/tools/testing/selftests/bpf/cgroup_helpers.c
> @@ -77,7 +77,8 @@ static int __enable_controllers(const char *cgroup_path, const char *controllers
> enable[len] = 0;
> close(fd);
> } else {
> - strncpy(enable, controllers, sizeof(enable) - 1);
> + enable[0] = '\0';
> + strncat(enable, controllers, sizeof(enable) - 1);
> }
>
> snprintf(path, sizeof(path), "%s/cgroup.subtree_control", cgroup_path);
> --
> 2.31.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Can we just use the first patch?
https://lore.kernel.org/lkml/[email protected]/
From: Rong Tao <[email protected]>
> Sent: 01 November 2022 09:25
>
> Can we just use the first patch?
> https://lore.kernel.org/lkml/[email protected]/
IIRC strlcpy() does the copy you want.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: Rong Tao <[email protected]>
move libbpf_strlcpy() to bpf_util.h, and replace strncpy() with
libbpf_strlcpy(), fix compile warning.
Compile samples/bpf, warning:
$ cd samples/bpf
$ make
...
cgroup_helpers.c: In function ‘__enable_controllers’:
cgroup_helpers.c:80:17: warning: ‘strncpy’ specified bound 4097 equals destination size [-Wstringop-truncation]
80 | strncpy(enable, controllers, sizeof(enable));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Rong Tao <[email protected]>
---
tools/testing/selftests/bpf/bpf_util.h | 19 +++++++++++++++++++
tools/testing/selftests/bpf/cgroup_helpers.c | 3 ++-
tools/testing/selftests/bpf/xsk.c | 19 -------------------
3 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/bpf/bpf_util.h b/tools/testing/selftests/bpf/bpf_util.h
index a3352a64c067..bf78212ff6e9 100644
--- a/tools/testing/selftests/bpf/bpf_util.h
+++ b/tools/testing/selftests/bpf/bpf_util.h
@@ -20,6 +20,25 @@ static inline unsigned int bpf_num_possible_cpus(void)
return possible_cpus;
}
+/* Copy up to sz - 1 bytes from zero-terminated src string and ensure that dst
+ * is zero-terminated string no matter what (unless sz == 0, in which case
+ * it's a no-op). It's conceptually close to FreeBSD's strlcpy(), but differs
+ * in what is returned. Given this is internal helper, it's trivial to extend
+ * this, when necessary. Use this instead of strncpy inside libbpf source code.
+ */
+static inline void libbpf_strlcpy(char *dst, const char *src, size_t sz)
+{
+ size_t i;
+
+ if (sz == 0)
+ return;
+
+ sz--;
+ for (i = 0; i < sz && src[i]; i++)
+ dst[i] = src[i];
+ dst[i] = '\0';
+}
+
#define __bpf_percpu_val_align __attribute__((__aligned__(8)))
#define BPF_DECLARE_PERCPU(type, name) \
diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
index e914cc45b766..e33b70e509da 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.c
+++ b/tools/testing/selftests/bpf/cgroup_helpers.c
@@ -13,6 +13,7 @@
#include <ftw.h>
#include "cgroup_helpers.h"
+#include "bpf_util.h"
/*
* To avoid relying on the system setup, when setup_cgroup_env is called
@@ -77,7 +78,7 @@ static int __enable_controllers(const char *cgroup_path, const char *controllers
enable[len] = 0;
close(fd);
} else {
- strncpy(enable, controllers, sizeof(enable));
+ libbpf_strlcpy(enable, controllers, sizeof(enable));
}
snprintf(path, sizeof(path), "%s/cgroup.subtree_control", cgroup_path);
diff --git a/tools/testing/selftests/bpf/xsk.c b/tools/testing/selftests/bpf/xsk.c
index 0b3ff49c740d..4b6890e2a0a9 100644
--- a/tools/testing/selftests/bpf/xsk.c
+++ b/tools/testing/selftests/bpf/xsk.c
@@ -521,25 +521,6 @@ static int xsk_create_bpf_link(struct xsk_socket *xsk)
return 0;
}
-/* Copy up to sz - 1 bytes from zero-terminated src string and ensure that dst
- * is zero-terminated string no matter what (unless sz == 0, in which case
- * it's a no-op). It's conceptually close to FreeBSD's strlcpy(), but differs
- * in what is returned. Given this is internal helper, it's trivial to extend
- * this, when necessary. Use this instead of strncpy inside libbpf source code.
- */
-static inline void libbpf_strlcpy(char *dst, const char *src, size_t sz)
-{
- size_t i;
-
- if (sz == 0)
- return;
-
- sz--;
- for (i = 0; i < sz && src[i]; i++)
- dst[i] = src[i];
- dst[i] = '\0';
-}
-
static int xsk_get_max_queues(struct xsk_socket *xsk)
{
struct ethtool_channels channels = { .cmd = ETHTOOL_GCHANNELS };
--
2.31.1
On 11/1/22 1:21 PM, Rong Tao wrote:
> From: Rong Tao <[email protected]>
>
> move libbpf_strlcpy() to bpf_util.h, and replace strncpy() with
> libbpf_strlcpy(), fix compile warning.
>
> Compile samples/bpf, warning:
> $ cd samples/bpf
> $ make
> ...
> cgroup_helpers.c: In function ‘__enable_controllers’:
> cgroup_helpers.c:80:17: warning: ‘strncpy’ specified bound 4097 equals destination size [-Wstringop-truncation]
> 80 | strncpy(enable, controllers, sizeof(enable));
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
BPF CI now throws a new warning after your patch:
https://github.com/kernel-patches/bpf/actions/runs/3369416153/jobs/5589052613
[...]
xsk.c:536:2: error: call to undeclared function 'libbpf_strlcpy'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
libbpf_strlcpy(ifr.ifr_name, ctx->ifname, IFNAMSIZ);
^
xsk.c:755:2: error: call to undeclared function 'libbpf_strlcpy'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
libbpf_strlcpy(ctx->ifname, ifname, IFNAMSIZ);
^
xsk.c:942:2: error: call to undeclared function 'libbpf_strlcpy'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
libbpf_strlcpy(ctx->ifname, ifname, IFNAMSIZ);
^
3 errors generated.
make: *** [Makefile:166: /tmp/work/bpf/bpf/tools/testing/selftests/bpf/xsk.o] Error 1
make: *** Waiting for unfinished jobs....