Re: [PATCH v2] nfsd: Avoid strlen conflict in nfsd4_encode_components_esc()

[NeilBrown <neilb@ownmail.net>]

On Tue, 30 Sep 2025, Nathan Chancellor wrote:
> On Mon, Sep 29, 2025 at 09:05:15AM -0400, Chuck Lever wrote:
> > On 9/28/25 4:29 PM, NeilBrown wrote:
> > > On Mon, 29 Sep 2025, Nathan Chancellor wrote:
> ...
> > >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > >> index ea91bad4eee2..9fe8a413f688 100644
> > >> --- a/fs/nfsd/nfs4xdr.c
> > >> +++ b/fs/nfsd/nfs4xdr.c
> > >> @@ -2640,11 +2640,9 @@ static __be32 nfsd4_encode_components_esc(struct xdr_stream *xdr, char sep,
> > >>  	__be32 *p;
> > >>  	__be32 pathlen;
> > >>  	int pathlen_offset;
> > >> -	int strlen, count=0;
> > >> +	int str_len, count=0;
> > >>  	char *str, *end, *next;
> > >>  
> > >> -	dprintk("nfsd4_encode_components(%s)\n", components);
> > >> -
> > >>  	pathlen_offset = xdr->buf->len;
> > >>  	p = xdr_reserve_space(xdr, 4);
> > >>  	if (!p)
> > >> @@ -2670,9 +2668,9 @@ static __be32 nfsd4_encode_components_esc(struct xdr_stream *xdr, char sep,
> > >>  			for (; *end && (*end != sep); end++)
> > >>  				/* find sep or end of string */;
> > >>  
> > >> -		strlen = end - str;
> > >> -		if (strlen) {
> > >> -			if (xdr_stream_encode_opaque(xdr, str, strlen) < 0)
> > >> +		str_len = end - str;
> > >> +		if (str_len) {
> > >> +			if (xdr_stream_encode_opaque(xdr, str, str_len) < 0)
> > >>  				return nfserr_resource;
> > > 
> > > I probably should have said something earlier, and this is definitely
> > > bike-shedding material, but .... "str_len" is not a whole lot nicer than
> > > "strlen" (or "i") ...
> > > 
> > > 
> > >    if (end > str) {
> > > 	if (xdr_stream_encode_opaque(xdr, str, end - str) < 0)
> > > 
> > > ??
> > 
> > "len" is actually the typical variable name used in such cases. But I
> > didn't want to bug Nathan for a resend.
> 
> If "len" is truly preferred, I do not mind sending a v3 with such a
> change. I had only kept "str" in the name because there is also
> "pathlen" in this function.


If you are going to send a v3 I would much prefer there were no variable
at all.  Just use
  if (end > str) {
    if (xdr_stream_encode_opaque(xdr, str, end - str) < 0) 
 ...


If we wanted a 'len' variable then I would change "str" to "component"
or some abbreviation there-of and use "componentlen" (or e.g.  cmpnt,
cmpntlen).  This function is encoding "components" so using suitably
named variables to focus the reader on which bits are the "components"
seems wise.

But as I say, we don't need a "len" variable, so I wouldn't bother
renaming "str" to "cmpnt" at this stage.

Thanks,
NeilBrown