[PATCH] rpc.statd: Avoid passing unregistered socket to svc_getreqset.

[PATCH] rpc.statd: Avoid passing unregistered socket to svc_getreqset.

[Carlos O'Donell]

rpc.statd may crash if it receives both a notification reply and a client
connection at the same time. It crashes because it adds sockfd to SVC_FDSET
and that violates the API contract. The SVC_FDSET is to be considered read-only
and must not be modified by user code. The daemon modifies it for expediency
to avoid having to maintain two distinct fd lists and select on each one.
It is a practical choice that makes sense.

Thus, if a notification reply arrives by itself everything works, or if a
client connection arrives by itself everything works. Both must arrive at
the same time for sockfd to be set in SVC_FDSET and to be processed by
svc_getreqset because more than one of readfds is ready.

It is the processing by svc_getreqset that will crash when it finds an
unregistered fd in the list that doesn't correlate to any of the internal
book keeping done by the library. At present the glibc SunRPC library will
crash, but TIRPC does not (it is robust against invalid API usage in this
case). However, future RPC libraries may be implemented differently, and
the questionable API usage should be fixed.

The simplest fix is for process_reply to *clear* sockfd from the
ready-to-read fds, since it was never registered with xprt_register.
This works because the code always calls process_reply before handing the 
fd set to the RPC layer for processing.

Compile-tested on x86_64 against master.

Signed-off-by: Carlos O'Donell <carlos@redhat.com>
---
 rmtcall.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/utils/statd/rmtcall.c b/utils/statd/rmtcall.c
index fd576d9..66a6eeb 100644
--- a/utils/statd/rmtcall.c
+++ b/utils/statd/rmtcall.c
@@ -221,6 +221,9 @@ process_reply(FD_SET_TYPE *rfds)
 	if (sockfd == -1 || !FD_ISSET(sockfd, rfds))
 		return 0;
 
+	/* Should not be processed again. */
+	FD_CLR (sockfd, rfds);
+
 	if (!(lp = recv_rply(&port)))
 		return 1;
 

Re: [PATCH] rpc.statd: Avoid passing unregistered socket to svc_getreqset.

[Steve Dickson]

On 02/19/2015 11:16 AM, Carlos O'Donell wrote:
> rpc.statd may crash if it receives both a notification reply and a client
> connection at the same time. It crashes because it adds sockfd to SVC_FDSET
> and that violates the API contract. The SVC_FDSET is to be considered read-only
> and must not be modified by user code. The daemon modifies it for expediency
> to avoid having to maintain two distinct fd lists and select on each one.
> It is a practical choice that makes sense.
> 
> Thus, if a notification reply arrives by itself everything works, or if a
> client connection arrives by itself everything works. Both must arrive at
> the same time for sockfd to be set in SVC_FDSET and to be processed by
> svc_getreqset because more than one of readfds is ready.
> 
> It is the processing by svc_getreqset that will crash when it finds an
> unregistered fd in the list that doesn't correlate to any of the internal
> book keeping done by the library. At present the glibc SunRPC library will
> crash, but TIRPC does not (it is robust against invalid API usage in this
> case). However, future RPC libraries may be implemented differently, and
> the questionable API usage should be fixed.
> 
> The simplest fix is for process_reply to *clear* sockfd from the
> ready-to-read fds, since it was never registered with xprt_register.
> This works because the code always calls process_reply before handing the 
> fd set to the RPC layer for processing.
> 
> Compile-tested on x86_64 against master.
> 
> Signed-off-by: Carlos O'Donell <carlos@redhat.com>
Committed!

steved.

> ---
>  rmtcall.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/utils/statd/rmtcall.c b/utils/statd/rmtcall.c
> index fd576d9..66a6eeb 100644
> --- a/utils/statd/rmtcall.c
> +++ b/utils/statd/rmtcall.c
> @@ -221,6 +221,9 @@ process_reply(FD_SET_TYPE *rfds)
>  	if (sockfd == -1 || !FD_ISSET(sockfd, rfds))
>  		return 0;
>  
> +	/* Should not be processed again. */
> +	FD_CLR (sockfd, rfds);
> +
>  	if (!(lp = recv_rply(&port)))
>  		return 1;
>  
>