| Message ID | 20240814093853.48657-1-kunwu.chan@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | SUNRPC: Fix -Wformat-truncation warning | expand |
On Wed, 14 Aug 2024, kunwu.chan@linux.dev wrote: > From: Kunwu Chan <chentao@kylinos.cn> > > Increase size of the servername array to avoid truncated output warning. > > net/sunrpc/clnt.c:582:75: error：‘%s’ directive output may be truncated > writing up to 107 bytes into a region of size 48 > [-Werror=format-truncation=] > 582 | snprintf(servername, sizeof(servername), "%s", > | ^~ > > net/sunrpc/clnt.c:582:33: note:‘snprintf’ output > between 1 and 108 bytes into a destination of size 48 > 582 | snprintf(servername, sizeof(servername), "%s", > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 583 | sun->sun_path); > > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > --- > net/sunrpc/clnt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c > index 09f29a95f2bc..874085f3ed50 100644 > --- a/net/sunrpc/clnt.c > +++ b/net/sunrpc/clnt.c > @@ -546,7 +546,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args) > .connect_timeout = args->connect_timeout, > .reconnect_timeout = args->reconnect_timeout, > }; > - char servername[48]; > + char servername[108]; If we choose this approach to removing the warning, then we should use UNIX_PATH_MAX rather than 108. However the longest server name copied in here will in practice be /var/run/rpcbind.sock so the extra 60 bytes on the stack is wasted ... maybe that doesn't matter. The string is only used by xprt_create_transport() which requires it to be less than RPC_MAXNETNAMELEN - which is 256. So maybe that would be a better value to use for the array size .... if we assume that stack space isn't a problem. What ever number we use, I'd rather it was a defined constant, and not an apparently arbitrary number. Thanks, NeilBrown > struct rpc_clnt *clnt; > int i; > > -- > 2.40.1 > >
Thanks for your reply. On 2024/8/14 18:28, NeilBrown wrote: > On Wed, 14 Aug 2024, kunwu.chan@linux.dev wrote: >> From: Kunwu Chan <chentao@kylinos.cn> >> >> Increase size of the servername array to avoid truncated output warning. >> >> net/sunrpc/clnt.c:582:75: error：‘%s’ directive output may be truncated >> writing up to 107 bytes into a region of size 48 >> [-Werror=format-truncation=] >> 582 | snprintf(servername, sizeof(servername), "%s", >> | ^~ >> >> net/sunrpc/clnt.c:582:33: note:‘snprintf’ output >> between 1 and 108 bytes into a destination of size 48 >> 582 | snprintf(servername, sizeof(servername), "%s", >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> 583 | sun->sun_path); >> >> Signed-off-by: Kunwu Chan <chentao@kylinos.cn> >> --- >> net/sunrpc/clnt.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c >> index 09f29a95f2bc..874085f3ed50 100644 >> --- a/net/sunrpc/clnt.c >> +++ b/net/sunrpc/clnt.c >> @@ -546,7 +546,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args) >> .connect_timeout = args->connect_timeout, >> .reconnect_timeout = args->reconnect_timeout, >> }; >> - char servername[48]; >> + char servername[108]; > If we choose this approach to removing the warning, then we should use > UNIX_PATH_MAX rather than 108. My negligence. > > However the longest server name copied in here will in practice be > /var/run/rpcbind.sock > > so the extra 60 bytes on the stack is wasted ... maybe that doesn't > matter. I'm thinking about use a dynamic space alloc method like kasprintf to avoid space waste. > The string is only used by xprt_create_transport() which requires it to > be less than RPC_MAXNETNAMELEN - which is 256. > So maybe that would be a better value to use for the array size .... if > we assume that stack space isn't a problem. Thank you for the detailed explanation. I read the xprt_create_transport, the RPC_MAXNETNAMELEN is only use to xprt_create_transport . > What ever number we use, I'd rather it was a defined constant, and not > an apparently arbitrary number. Whether we could check the sun->sun_path length before using snprintf? The array size should smaller than the minimum of sun->sun_path and RPC_MAXNETNAMELEN. Or use the dynamic space allocate method to save space. > > Thanks, > NeilBrown > > >> struct rpc_clnt *clnt; >> int i; >> >> -- >> 2.40.1 >> >>
On Thu, 15 Aug 2024, Kunwu Chan wrote: > Thanks for your reply. > > On 2024/8/14 18:28, NeilBrown wrote: > > On Wed, 14 Aug 2024, kunwu.chan@linux.dev wrote: > >> From: Kunwu Chan <chentao@kylinos.cn> > >> > >> Increase size of the servername array to avoid truncated output warning. > >> > >> net/sunrpc/clnt.c:582:75: error：‘%s’ directive output may be truncated > >> writing up to 107 bytes into a region of size 48 > >> [-Werror=format-truncation=] > >> 582 | snprintf(servername, sizeof(servername), "%s", > >> | ^~ > >> > >> net/sunrpc/clnt.c:582:33: note:‘snprintf’ output > >> between 1 and 108 bytes into a destination of size 48 > >> 582 | snprintf(servername, sizeof(servername), "%s", > >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >> 583 | sun->sun_path); > >> > >> Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > >> --- > >> net/sunrpc/clnt.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c > >> index 09f29a95f2bc..874085f3ed50 100644 > >> --- a/net/sunrpc/clnt.c > >> +++ b/net/sunrpc/clnt.c > >> @@ -546,7 +546,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args) > >> .connect_timeout = args->connect_timeout, > >> .reconnect_timeout = args->reconnect_timeout, > >> }; > >> - char servername[48]; > >> + char servername[108]; > > If we choose this approach to removing the warning, then we should use > > UNIX_PATH_MAX rather than 108. > My negligence. > > > > However the longest server name copied in here will in practice be > > /var/run/rpcbind.sock > > > > so the extra 60 bytes on the stack is wasted ... maybe that doesn't > > matter. > I'm thinking about use a dynamic space alloc method like kasprintf to > avoid space waste. > > The string is only used by xprt_create_transport() which requires it to > > be less than RPC_MAXNETNAMELEN - which is 256. > > So maybe that would be a better value to use for the array size .... if > > we assume that stack space isn't a problem. > > Thank you for the detailed explanation. I read the > xprt_create_transport, the RPC_MAXNETNAMELEN > > is only use to xprt_create_transport . > > > What ever number we use, I'd rather it was a defined constant, and not > > an apparently arbitrary number. > > Whether we could check the sun->sun_path length before using snprintf? > The array size should smaller > > than the minimum of sun->sun_path and RPC_MAXNETNAMELEN. > > Or use the dynamic space allocate method to save space. I think that dynamically allocating space is not a good idea. It means you have to handle failure which is just a waste of code. I'd suggest simply changing the array to RPC_MAXNETNAMELEN. NeilBrown > > > > > Thanks, > > NeilBrown > > > > > >> struct rpc_clnt *clnt; > >> int i; > >> > >> -- > >> 2.40.1 > >> > >> > -- > Thanks, > Kunwu.Chan > >
Thanks for your reply. On 2024/8/15 19:39, NeilBrown wrote: > On Thu, 15 Aug 2024, Kunwu Chan wrote: >> Thanks for your reply. >> >> On 2024/8/14 18:28, NeilBrown wrote: >>> On Wed, 14 Aug 2024, kunwu.chan@linux.dev wrote: >>>> From: Kunwu Chan <chentao@kylinos.cn> >>>> >>>> Increase size of the servername array to avoid truncated output warning. >>>> >>>> net/sunrpc/clnt.c:582:75: error：‘%s’ directive output may be truncated >>>> writing up to 107 bytes into a region of size 48 >>>> [-Werror=format-truncation=] >>>> 582 | snprintf(servername, sizeof(servername), "%s", >>>> | ^~ >>>> >>>> net/sunrpc/clnt.c:582:33: note:‘snprintf’ output >>>> between 1 and 108 bytes into a destination of size 48 >>>> 582 | snprintf(servername, sizeof(servername), "%s", >>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>> 583 | sun->sun_path); >>>> >>>> Signed-off-by: Kunwu Chan <chentao@kylinos.cn> >>>> --- >>>> net/sunrpc/clnt.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c >>>> index 09f29a95f2bc..874085f3ed50 100644 >>>> --- a/net/sunrpc/clnt.c >>>> +++ b/net/sunrpc/clnt.c >>>> @@ -546,7 +546,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args) >>>> .connect_timeout = args->connect_timeout, >>>> .reconnect_timeout = args->reconnect_timeout, >>>> }; >>>> - char servername[48]; >>>> + char servername[108]; >>> If we choose this approach to removing the warning, then we should use >>> UNIX_PATH_MAX rather than 108. >> My negligence. >>> However the longest server name copied in here will in practice be >>> /var/run/rpcbind.sock >>> >>> so the extra 60 bytes on the stack is wasted ... maybe that doesn't >>> matter. >> I'm thinking about use a dynamic space alloc method like kasprintf to >> avoid space waste. >>> The string is only used by xprt_create_transport() which requires it to >>> be less than RPC_MAXNETNAMELEN - which is 256. >>> So maybe that would be a better value to use for the array size .... if >>> we assume that stack space isn't a problem. >> Thank you for the detailed explanation. I read the >> xprt_create_transport, the RPC_MAXNETNAMELEN >> >> is only use to xprt_create_transport . >> >>> What ever number we use, I'd rather it was a defined constant, and not >>> an apparently arbitrary number. >> Whether we could check the sun->sun_path length before using snprintf? >> The array size should smaller >> >> than the minimum of sun->sun_path and RPC_MAXNETNAMELEN. >> >> Or use the dynamic space allocate method to save space. > I think that dynamically allocating space is not a good idea. It means > you have to handle failure which is just a waste of code. > > I'd suggest simply changing the array to RPC_MAXNETNAMELEN. I'll follow your suggestion and change it in v2. > > NeilBrown > > > >>> Thanks, >>> NeilBrown >>> >>> >>>> struct rpc_clnt *clnt; >>>> int i; >>>> >>>> -- >>>> 2.40.1 >>>> >>>> >> -- >> Thanks, >> Kunwu.Chan >> >>
On Wed, Aug 14, 2024 at 05:38:53PM +0800, kunwu.chan@linux.dev wrote: > From: Kunwu Chan <chentao@kylinos.cn> > > Increase size of the servername array to avoid truncated output warning. > > net/sunrpc/clnt.c:582:75: error：‘%s’ directive output may be truncated > writing up to 107 bytes into a region of size 48 > [-Werror=format-truncation=] > 582 | snprintf(servername, sizeof(servername), "%s", > | ^~ > > net/sunrpc/clnt.c:582:33: note:‘snprintf’ output > between 1 and 108 bytes into a destination of size 48 > 582 | snprintf(servername, sizeof(servername), "%s", > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 583 | sun->sun_path); I think this has come up before, but i could be mis-remembering. Please could you do a search for the discussion. The fact it is not solved suggests to me it is not so simple to fix. Maybe there is some protocol implications here. Andrew
On Sat, 17 Aug 2024, Andrew Lunn wrote: > On Wed, Aug 14, 2024 at 05:38:53PM +0800, kunwu.chan@linux.dev wrote: > > From: Kunwu Chan <chentao@kylinos.cn> > > > > Increase size of the servername array to avoid truncated output warning. > > > > net/sunrpc/clnt.c:582:75: error：‘%s’ directive output may be truncated > > writing up to 107 bytes into a region of size 48 > > [-Werror=format-truncation=] > > 582 | snprintf(servername, sizeof(servername), "%s", > > | ^~ > > > > net/sunrpc/clnt.c:582:33: note:‘snprintf’ output > > between 1 and 108 bytes into a destination of size 48 > > 582 | snprintf(servername, sizeof(servername), "%s", > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > 583 | sun->sun_path); > > I think this has come up before, but i could be mis-remembering. > Please could you do a search for the discussion. The fact it is not > solved suggests to me it is not so simple to fix. Maybe there is some > protocol implications here. > > Andrew > All I could find was https://lore.kernel.org/all/1648103566-15528-1-git-send-email-baihaowen@meizu.com/ which essentially followed the same path as this conversation. The patch was resubmitted using UNIX_PATH_MAX but never responded to by the relevant maintainers. There are no protocol implications. This string is only used for informational messages. NeilBrown
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c index 09f29a95f2bc..874085f3ed50 100644 --- a/net/sunrpc/clnt.c +++ b/net/sunrpc/clnt.c @@ -546,7 +546,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args) .connect_timeout = args->connect_timeout, .reconnect_timeout = args->reconnect_timeout, }; - char servername[48]; + char servername[108]; struct rpc_clnt *clnt; int i;