diff mbox series

net: sunrpc: sizeof('\0') is 4, not 1

Message ID 4zlmy3qwneijnrsbygfr2wbsnvdvcgvjyvudqnuxq5zvwmyaof@tarta.nabijaczleweli.xyz
State New
Headers
Series net: sunrpc: sizeof('\0') is 4, not 1 |

Commit Message

Ahelenia Ziemiańska Dec. 15, 2023, 11:53 p.m. UTC 
  To make it self-documenting, the referenced commit added the space
for the null terminator as sizeof('\0'). The message elaborates on
why only one byte is needed, so this is clearly a mistake.
Spell it as 1 /* NUL */ instead.

This is the only result for git grep "sizeof.'" in the tree.

Fixes: commit 1e360a60b24a ("SUNRPC: Address  buffer overrun in
 rpc_uaddr2sockaddr()")
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
 net/sunrpc/addr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 26aff849438cebcd05f1a647390c4aa700d5c0f1

Comments

NeilBrown Dec. 16, 2023, 4:27 a.m. UTC | #1 
On Sat, 16 Dec 2023, Ahelenia Ziemiańska wrote:
> To make it self-documenting, the referenced commit added the space
> for the null terminator as sizeof('\0'). The message elaborates on
> why only one byte is needed, so this is clearly a mistake.
> Spell it as 1 /* NUL */ instead.
> 
> This is the only result for git grep "sizeof.'" in the tree.
> 
> Fixes: commit 1e360a60b24a ("SUNRPC: Address  buffer overrun in
>  rpc_uaddr2sockaddr()")

It isn't clear to me that "Fixes" is appropriate as that patch isn't
harmful, just confused and sub-optimal.
But it probably doesn't mattter.

Reviewed-by: NeilBrown <neilb@suse.de>

Thanks,
NeilBrown



> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> ---
>  net/sunrpc/addr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/addr.c b/net/sunrpc/addr.c
> index d435bffc6199..c4ba342f6866 100644
> --- a/net/sunrpc/addr.c
> +++ b/net/sunrpc/addr.c
> @@ -311,7 +311,7 @@ size_t rpc_uaddr2sockaddr(struct net *net, const char *uaddr,
>  			  const size_t uaddr_len, struct sockaddr *sap,
>  			  const size_t salen)
>  {
> -	char *c, buf[RPCBIND_MAXUADDRLEN + sizeof('\0')];
> +	char *c, buf[RPCBIND_MAXUADDRLEN + 1 /* NUL */];
>  	u8 portlo, porthi;
>  	unsigned short port;
>  
> 
> base-commit: 26aff849438cebcd05f1a647390c4aa700d5c0f1
> -- 
> 2.39.2
>
Ahelenia Ziemiańska Dec. 16, 2023, 5:43 a.m. UTC | #2 
On Sat, Dec 16, 2023 at 03:27:16PM +1100, NeilBrown wrote:
> On Sat, 16 Dec 2023, Ahelenia Ziemiańska wrote:
> > To make it self-documenting, the referenced commit added the space
> > for the null terminator as sizeof('\0'). The message elaborates on
> > why only one byte is needed, so this is clearly a mistake.
> > Spell it as 1 /* NUL */ instead.
> > 
> > Fixes: commit 1e360a60b24a ("SUNRPC: Address  buffer overrun in
> >  rpc_uaddr2sockaddr()")
> It isn't clear to me that "Fixes" is appropriate as that patch isn't
> harmful, just confused and sub-optimal.
I definitely agree, I don't like Fixes here at all,
but I don't really see another trailer in the documentation
or in the log that could be used for this.
NeilBrown Dec. 16, 2023, 5:53 a.m. UTC | #3 
On Sat, 16 Dec 2023, Ahelenia Ziemiańska wrote:
> On Sat, Dec 16, 2023 at 03:27:16PM +1100, NeilBrown wrote:
> > On Sat, 16 Dec 2023, Ahelenia Ziemiańska wrote:
> > > To make it self-documenting, the referenced commit added the space
> > > for the null terminator as sizeof('\0'). The message elaborates on
> > > why only one byte is needed, so this is clearly a mistake.
> > > Spell it as 1 /* NUL */ instead.
> > > 
> > > Fixes: commit 1e360a60b24a ("SUNRPC: Address  buffer overrun in
> > >  rpc_uaddr2sockaddr()")
> > It isn't clear to me that "Fixes" is appropriate as that patch isn't
> > harmful, just confused and sub-optimal.
> I definitely agree, I don't like Fixes here at all,
> but I don't really see another trailer in the documentation
> or in the log that could be used for this.
> 

Make up a new Trailer? 

I would probably just write

 To make it self-documenting,
   commit 1e360a60b24a ("SUNRPC: Address  buffer overrun in rpc_uaddr2sockaddr()")
 added the space for the null terminator as sizeof('\0') which is 4.  The commit
 elaborates on  why only one byte is needed, so this is clearly a mistake.
 Spell it as 1 /* NUL */ instead.
 
NeilBrown
Chuck Lever Dec. 16, 2023, 4:45 p.m. UTC | #4 
On Sat, Dec 16, 2023 at 04:53:23PM +1100, NeilBrown wrote:
> On Sat, 16 Dec 2023, Ahelenia Ziemiańska wrote:
> > On Sat, Dec 16, 2023 at 03:27:16PM +1100, NeilBrown wrote:
> > > On Sat, 16 Dec 2023, Ahelenia Ziemiańska wrote:
> > > > To make it self-documenting, the referenced commit added the space
> > > > for the null terminator as sizeof('\0'). The message elaborates on
> > > > why only one byte is needed, so this is clearly a mistake.
> > > > Spell it as 1 /* NUL */ instead.
> > > > 
> > > > Fixes: commit 1e360a60b24a ("SUNRPC: Address  buffer overrun in
> > > >  rpc_uaddr2sockaddr()")
> > > It isn't clear to me that "Fixes" is appropriate as that patch isn't
> > > harmful, just confused and sub-optimal.
> > I definitely agree, I don't like Fixes here at all,
> > but I don't really see another trailer in the documentation
> > or in the log that could be used for this.
> > 
> 
> Make up a new Trailer? 
> 
> I would probably just write
> 
>  To make it self-documenting,
>    commit 1e360a60b24a ("SUNRPC: Address  buffer overrun in rpc_uaddr2sockaddr()")
>  added the space for the null terminator as sizeof('\0') which is 4.  The commit
>  elaborates on  why only one byte is needed, so this is clearly a mistake.
>  Spell it as 1 /* NUL */ instead.

Agreed; if Fixes: is overkill, simply spell out the commit that
introduced the issue in the patch description as Neil does here.
diff mbox series

Patch

diff --git a/net/sunrpc/addr.c b/net/sunrpc/addr.c
index d435bffc6199..c4ba342f6866 100644
--- a/net/sunrpc/addr.c
+++ b/net/sunrpc/addr.c
@@ -311,7 +311,7 @@  size_t rpc_uaddr2sockaddr(struct net *net, const char *uaddr,
 			  const size_t uaddr_len, struct sockaddr *sap,
 			  const size_t salen)
 {
-	char *c, buf[RPCBIND_MAXUADDRLEN + sizeof('\0')];
+	char *c, buf[RPCBIND_MAXUADDRLEN + 1 /* NUL */];
 	u8 portlo, porthi;
 	unsigned short port;