diff mbox series

[bpf-next] selftests/bpf: Fix strncpy() fortify warning

Message ID tencent_EE3E19F80ACD66955D26A878BC768CFA210A@qq.com
State New
Headers
Series [bpf-next] selftests/bpf: Fix strncpy() fortify warning |

Commit Message

Rong Tao Oct. 27, 2022, 11:33 a.m. UTC 
  From: Rong Tao <rongtao@cestc.cn>

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 <rongtao@cestc.cn>
---
 tools/testing/selftests/bpf/cgroup_helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrii Nakryiko Oct. 27, 2022, 8:09 p.m. UTC | #1 
On Thu, Oct 27, 2022 at 4:34 AM Rong Tao <rtoax@foxmail.com> wrote:
>
> From: Rong Tao <rongtao@cestc.cn>
>
> 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 <rongtao@cestc.cn>
> ---
>  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
>
Rong Tao Oct. 28, 2022, 12:26 a.m. UTC | #2 
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.
Andrii Nakryiko Oct. 28, 2022, 5:08 p.m. UTC | #3 
On Thu, Oct 27, 2022 at 5:26 PM Rong Tao <rtoax@foxmail.com> 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
diff mbox series

Patch

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);