Re: [PATCH 1/7] SUNRPC: Address a few compiler warnings in the RPC client

Re: [PATCH 1/7] SUNRPC: Address a few compiler warnings in the RPC client

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]

>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index c796e2f..0ad3042 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -127,7 +127,7 @@ static struct rpc_clnt * rpc_new_client(struct rpc_xprt *xprt, char *servname, s
>>  	struct rpc_clnt		*clnt = NULL;
>>  	struct rpc_auth		*auth;
>>  	int err;
>> -	int len;
>> +	size_t len;
> 
> What if 'len' overflows when we do 'strlen() + 1'?

strlen() returns size_t, which is unsigned.  Thus the overflow would be 
zero.  Seems like that's a better outcome than what would happen today.

>>  	dprintk("RPC:       creating %s client for %s (xprt %p)\n",
>>  			program->name, servname, xprt);
>> @@ -436,7 +436,7 @@ rpc_release_client(struct rpc_clnt *clnt)
>>   */
>>  struct rpc_clnt *rpc_bind_new_program(struct rpc_clnt *old,
>>  				      struct rpc_program *program,
>> -				      int vers)
>> +				      unsigned int vers)
>>  {
>>  	struct rpc_clnt *clnt;
>>  	struct rpc_version *version;
>> @@ -1508,6 +1508,7 @@ void rpc_show_tasks(void)
>>  {
>>  	struct rpc_clnt *clnt;
>>  	struct rpc_task *t;
>> +	int proc = -1;
>>  
>>  	spin_lock(&rpc_client_lock);
>>  	if (list_empty(&all_clients))
>> @@ -1524,9 +1525,11 @@ void rpc_show_tasks(void)
>>  			if (RPC_IS_QUEUED(t))
>>  				rpc_waitq = rpc_qname(t->u.tk_wait.rpc_waitq);
>>  
>> +			if (t->tk_msg.rpc_proc)
>> +				proc = t->tk_msg.rpc_proc->p_proc;
>> +
>>  			printk("%5u %04d %04x %6d %8p %6d %8p %8ld %8s %8p %8p\n",
>> -				t->tk_pid,
>> -				(t->tk_msg.rpc_proc ? t->tk_msg.rpc_proc->p_proc : -1),
>> +				t->tk_pid, proc,
>>  				t->tk_flags, t->tk_status,
>>  				t->tk_client,
>>  				(t->tk_client ? t->tk_client->cl_prog : 0),
> 
> NACK. Look again at your change to rpc_show_tasks. What you are doing
> with your variable 'proc' is _not_ equivalent to the existing code: you
> need to reset proc to '-1' each time you go through the loop.

Noted and fixed.

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 302 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
url:http://oss.oracle.com/~cel
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH 1/7] SUNRPC: Address a few compiler warnings in the RPC client

[Trond Myklebust]

On Mon, 2007-08-27 at 15:15 -0400, Chuck Lever wrote:
> >> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> >> index c796e2f..0ad3042 100644
> >> --- a/net/sunrpc/clnt.c
> >> +++ b/net/sunrpc/clnt.c
> >> @@ -127,7 +127,7 @@ static struct rpc_clnt * rpc_new_client(struct rpc_xprt *xprt, char *servname, s
> >>  	struct rpc_clnt		*clnt = NULL;
> >>  	struct rpc_auth		*auth;
> >>  	int err;
> >> -	int len;
> >> +	size_t len;
> > 
> > What if 'len' overflows when we do 'strlen() + 1'?
> 
> strlen() returns size_t, which is unsigned.  Thus the overflow would be 
> zero.  Seems like that's a better outcome than what would happen today.

Yes, but there has to be a reason for fixing these signed comparisons
beyond just silencing the compiler. If we're worried that len may
overflow the signed integer space, then we really should be setting a
hard limit on the string length. I personally don't think that a server
name of 2^31 characters or more makes any sense whatsoever.

Do we know if the callers are checking the size of the server name prior
to calling rpc_new_client?

Trond


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs