Re: [PATCH 1/1] nfs-utils/statd: fix a segfault caused by improper usage of RPC interface

[Steve Dickson <SteveD@redhat.com>]

On 09/23/2015 02:24 AM, Shan Hai wrote:
> There is a hack which uses the bottom-level RPC improperly as below
> in the current statd implementation:
> insert a socket in the svc_fdset without a corresponding transport handle
> and passes the socket to the svc_getreqset subroutine, this usage causes
> a segfault of statd on a huge amount of sm-notifications.
> 
> Fix the issue by separating the non-RPC-server socket from RPC dispatcher.
> 
> Below is the segfault:
> Core was generated by `/usr/sbin/rpc.statd --name=master --port 32765 --outgoing-port 32766'.
> 
> Program terminated with signal 11, Segmentation fault.
> 0 __GI_svc_getreq_common (fd=<optimized out>) at svc.c:492
> warning: Source file is more recent than executable.
> 492 if (SVC_RECV (xprt, &msg))
> 
> (gdb) bt
> 0 __GI_svc_getreq_common (fd=<optimized out>) at svc.c:492
> 1 0x0000003af9f13a82 in __GI_svc_getreqset (readfds=<optimized out>) at svc.c:439
> 2 0x0000000000404e64 in my_svc_run () at svc_run.c:129
> 3 0x00000000004035dd in main (argc=6, argv=<optimized out>) at statd.c:472
> 
> (gdb) info frame 1
> Stack frame at 0x7fffdd256a40:
> rip = 0x3af9f13a82 in __GI_svc_getreqset (svc.c:439); saved rip 0x404e64
> called by frame at 0x7fffdd256b00, caller of frame at 0x7fffdd256a00
> 
> source language c.
> 
> Arglist at 0x7fffdd2569f8, args: readfds=<optimized out>
> Locals at 0x7fffdd2569f8, Previous frame's sp is 0x7fffdd256a40
> 
> Signed-off-by: Shan Hai <shan.hai@windriver.com>
Committed... 

steved.

> ---
>  utils/statd/rmtcall.c | 1 -
>  utils/statd/statd.c   | 5 +++--
>  utils/statd/statd.h   | 2 +-
>  utils/statd/svc_run.c | 8 ++++++--
>  4 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/utils/statd/rmtcall.c b/utils/statd/rmtcall.c
> index 45c84f9..c4f6364 100644
> --- a/utils/statd/rmtcall.c
> +++ b/utils/statd/rmtcall.c
> @@ -113,7 +113,6 @@ statd_get_socket(void)
>  	if (sockfd < 0)
>  		return -1;
>  
> -	FD_SET(sockfd, &SVC_FDSET);
>  	return sockfd;
>  }
>  
> diff --git a/utils/statd/statd.c b/utils/statd/statd.c
> index 2b7a167..e5b4c98 100644
> --- a/utils/statd/statd.c
> +++ b/utils/statd/statd.c
> @@ -247,6 +247,7 @@ int main (int argc, char **argv)
>  	int port = 0, out_port = 0;
>  	int nlm_udp = 0, nlm_tcp = 0;
>  	struct rlimit rlim;
> +	int notify_sockfd;
>  
>  	/* Default: daemon mode, no other options */
>  	run_mode = 0;
> @@ -437,7 +438,7 @@ int main (int argc, char **argv)
>  		}
>  
>  	/* Make sure we have a privilege port for calling into the kernel */
> -	if (statd_get_socket() < 0)
> +	if ((notify_sockfd = statd_get_socket()) < 0)
>  		exit(1);
>  
>  	/* If sm-notify didn't take all the state files, load
> @@ -484,7 +485,7 @@ int main (int argc, char **argv)
>  		 * Handle incoming requests:  SM_NOTIFY socket requests, as
>  		 * well as callbacks from lockd.
>  		 */
> -		my_svc_run();	/* I rolled my own, Olaf made it better... */
> +		my_svc_run(notify_sockfd);	/* I rolled my own, Olaf made it better... */
>  
>  		/* Only get here when simulating a crash so we should probably
>  		 * start sm-notify running again.  As we have already dropped
> diff --git a/utils/statd/statd.h b/utils/statd/statd.h
> index a1d8035..231ac7e 100644
> --- a/utils/statd/statd.h
> +++ b/utils/statd/statd.h
> @@ -28,7 +28,7 @@ extern _Bool	statd_present_address(const struct sockaddr *sap, char *buf,
>  __attribute__((__malloc__))
>  extern char *	statd_canonical_name(const char *hostname);
>  
> -extern void	my_svc_run(void);
> +extern void	my_svc_run(int);
>  extern void	notify_hosts(void);
>  extern void	shuffle_dirs(void);
>  extern int	statd_get_socket(void);
> diff --git a/utils/statd/svc_run.c b/utils/statd/svc_run.c
> index d98ecee..28c1ad6 100644
> --- a/utils/statd/svc_run.c
> +++ b/utils/statd/svc_run.c
> @@ -78,7 +78,7 @@ my_svc_exit(void)
>   * The heart of the server.  A crib from libc for the most part...
>   */
>  void
> -my_svc_run(void)
> +my_svc_run(int sockfd)
>  {
>  	FD_SET_TYPE	readfds;
>  	int             selret;
> @@ -96,6 +96,8 @@ my_svc_run(void)
>  		}
>  
>  		readfds = SVC_FDSET;
> +		/* Set notify sockfd for waiting for reply */
> +		FD_SET(sockfd, &readfds);
>  		if (notify) {
>  			struct timeval	tv;
>  
> @@ -125,8 +127,10 @@ my_svc_run(void)
>  
>  		default:
>  			selret -= process_reply(&readfds);
> -			if (selret)
> +			if (selret) {
> +				FD_CLR(sockfd, &readfds);
>  				svc_getreqset(&readfds);
> +			}
>  		}
>  	}
>  }
>