| Message ID | 54597868.3060408@redhat.com |
|---|---|
| State | New |
| Headers | show |
Brad Hubbard <bhubbard@redhat.com> writes: > Following is the valgrind warning. > > ==26802== Conditional jump or move depends on uninitialised value(s) > ==26802== at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so) > ==26802== by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so) Why was svc_getreqset called with file descriptors that were never seen by xprt_register? Andreas.
On Wed, Nov 05, 2014 at 10:03:46AM +0100, Andreas Schwab wrote: > Brad Hubbard <bhubbard@redhat.com> writes: > > > Following is the valgrind warning. > > > > ==26802== Conditional jump or move depends on uninitialised value(s) > > ==26802== at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so) > > > ==26802== by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so) > > Why was svc_getreqset called with file descriptors that were never seen > by xprt_register? That is likely an application bug, but it might not be a bad idea to include the patch anyway. Failing the NULL check and returning seems better than allowing to dereference arbitrary pointer values. Siddhesh
On 11/05/2014 07:03 PM, Andreas Schwab wrote: > Brad Hubbard <bhubbard@redhat.com> writes: > >> Following is the valgrind warning. >> >> ==26802== Conditional jump or move depends on uninitialised value(s) >> ==26802== at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so) > >> ==26802== by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so) > > Why was svc_getreqset called with file descriptors that were never seen > by xprt_register? Yes, I see your point of course but I still think the patch has merit. My initial analysis was flawed.
Siddhesh Poyarekar <siddhesh@redhat.com> writes: > On Wed, Nov 05, 2014 at 10:03:46AM +0100, Andreas Schwab wrote: >> Brad Hubbard <bhubbard@redhat.com> writes: >> >> > Following is the valgrind warning. >> > >> > ==26802== Conditional jump or move depends on uninitialised value(s) >> > ==26802== at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so) >> >> > ==26802== by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so) >> >> Why was svc_getreqset called with file descriptors that were never seen >> by xprt_register? > > That is likely an application bug, but it might not be a bad idea to > include the patch anyway. Failing the NULL check and returning seems > better than allowing to dereference arbitrary pointer values. But what does this have to do with "sock > _rpc_dtablesize()"? Andreas.
On Wed, Nov 05, 2014 at 10:31:31AM +0100, Andreas Schwab wrote: > > That is likely an application bug, but it might not be a bad idea to > > include the patch anyway. Failing the NULL check and returning seems > > better than allowing to dereference arbitrary pointer values. > > But what does this have to do with "sock > _rpc_dtablesize()"? Nothing. That analysis is incorrect AFAIK and it seems to be a failure in calling xprt_register as you pointed out, which should be an application bug. Siddhesh
On Wed, Nov 05, 2014 at 02:44:34PM +0530, Siddhesh Poyarekar wrote: > On Wed, Nov 05, 2014 at 10:03:46AM +0100, Andreas Schwab wrote: > > Brad Hubbard <bhubbard@redhat.com> writes: > > > > > Following is the valgrind warning. > > > > > > ==26802== Conditional jump or move depends on uninitialised value(s) > > > ==26802== at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so) > > > > > ==26802== by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so) > > > > Why was svc_getreqset called with file descriptors that were never seen > > by xprt_register? > > That is likely an application bug, but it might not be a bad idea to > include the patch anyway. Failing the NULL check and returning seems > better than allowing to dereference arbitrary pointer values. > As its better to always fail than only sometimes I also think its good to include it.
On 12/10/2014 08:33 AM, Ondřej Bílka wrote: > On Wed, Nov 05, 2014 at 02:44:34PM +0530, Siddhesh Poyarekar wrote: >> On Wed, Nov 05, 2014 at 10:03:46AM +0100, Andreas Schwab wrote: >>> Brad Hubbard <bhubbard@redhat.com> writes: >>> >>>> Following is the valgrind warning. >>>> >>>> ==26802== Conditional jump or move depends on uninitialised value(s) >>>> ==26802== at 0x5343A25: svc_getreq_common (in /lib64/libc-2.5.so) >>> >>>> ==26802== by 0x534357B: svc_getreqset (in /lib64/libc-2.5.so) >>> >>> Why was svc_getreqset called with file descriptors that were never seen >>> by xprt_register? >> >> That is likely an application bug, but it might not be a bad idea to >> include the patch anyway. Failing the NULL check and returning seems >> better than allowing to dereference arbitrary pointer values. >> > As its better to always fail than only sometimes I also think its good > to include it. > Does anyone want me to follow up on the upstream bug? Any advice on how best to do so? I assume a "ping" in the Bugzilla and the mailing list but what format should they have (definite noob here but I want to learn). Any advice is greatly appreciated.
On Wed, Dec 10, 2014 at 08:48:20AM +1000, Brad Hubbard wrote: > >As its better to always fail than only sometimes I also think its good > >to include it. > > Andreas, do you object to me committing this patch? Siddhesh
Siddhesh Poyarekar <siddhesh@redhat.com> writes: > On Wed, Dec 10, 2014 at 08:48:20AM +1000, Brad Hubbard wrote: >> >As its better to always fail than only sometimes I also think its good >> >to include it. >> > > > Andreas, do you object to me committing this patch? We should first understand what is the real bug. Andreas.
On Wed, Dec 10, 2014 at 09:26:26AM +0100, Andreas Schwab wrote:
> We should first understand what is the real bug.
The real bug is in the calling application, which calls svc_getreq on
a file descriptor that it did not register. We're patching this in
sunrpc so that the check in svc_getreq_common below that was supposed
to catch such conditions actually works.
...
xprt = xports[fd];
/* Do we control fd? */
if (xprt == NULL)
return;
...
Siddhesh
On Wed, Dec 10, 2014 at 02:06:45PM +0530, Siddhesh Poyarekar wrote: > On Wed, Dec 10, 2014 at 09:26:26AM +0100, Andreas Schwab wrote: > > We should first understand what is the real bug. > > The real bug is in the calling application, which calls svc_getreq on > a file descriptor that it did not register. We're patching this in > sunrpc so that the check in svc_getreq_common below that was supposed > to catch such conditions actually works. > > ... > xprt = xports[fd]; > /* Do we control fd? */ > if (xprt == NULL) > return; > ... Andreas, is this OK or are you still not convinced about adding this patch in? Siddhesh
Siddhesh Poyarekar <siddhesh@redhat.com> writes: > Andreas, is this OK or are you still not convinced about adding this > patch in? How about fixing the real bug? Andreas.
On Mon, Mar 02, 2015 at 10:29:31AM +0100, Andreas Schwab wrote: > Siddhesh Poyarekar <siddhesh@redhat.com> writes: > > > Andreas, is this OK or are you still not convinced about adding this > > patch in? > > How about fixing the real bug? Carlos has posted a fix for nfs-utils[1]. The intent of including this patch is not to work around that specific bug, but to make the code robust against such bugs in future. Siddhesh [1] http://article.gmane.org/gmane.linux.nfs/69437
On 03/02/2015 04:34 AM, Siddhesh Poyarekar wrote: > On Mon, Mar 02, 2015 at 10:29:31AM +0100, Andreas Schwab wrote: >> Siddhesh Poyarekar <siddhesh@redhat.com> writes: >> >>> Andreas, is this OK or are you still not convinced about adding this >>> patch in? >> >> How about fixing the real bug? > > Carlos has posted a fix for nfs-utils[1]. The intent of including > this patch is not to work around that specific bug, but to make the > code robust against such bugs in future. > > Siddhesh > > [1] http://article.gmane.org/gmane.linux.nfs/69437 > This should be committed to glibc. This is now committed to nfs-utils. I'd say we should make SunRPC robust also, just as a matter of fact. We still have old applications using the compatibility symbols that can benefit from having unregistered fds be ignored. commit 810423415dd1a2b7275b3abf294e6a69951614a1 Author: Carlos O'Donell <carlos@redhat.com> Date: Thu Feb 26 14:13:26 2015 -0500 rpc.statd: Avoid passing unregistered socket to svc_getreqset 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> Signed-off-by: Steve Dickson <steved@redhat.com> Cheers, Carlos.
On Thu, Mar 05, 2015 at 03:05:20PM -0500, Carlos O'Donell wrote: > On 03/02/2015 04:34 AM, Siddhesh Poyarekar wrote: > > On Mon, Mar 02, 2015 at 10:29:31AM +0100, Andreas Schwab wrote: > >> Siddhesh Poyarekar <siddhesh@redhat.com> writes: > >> > >>> Andreas, is this OK or are you still not convinced about adding this > >>> patch in? > >> > >> How about fixing the real bug? > > > > Carlos has posted a fix for nfs-utils[1]. The intent of including > > this patch is not to work around that specific bug, but to make the > > code robust against such bugs in future. > > > > Siddhesh > > > > [1] http://article.gmane.org/gmane.linux.nfs/69437 > > > > This should be committed to glibc. Since Andreas' point of fixing the original bug has also been taken care off, I'll commit this by the end of the week if there are no more objections. Siddhesh
On Wed, Mar 11, 2015 at 09:14:34PM +0530, Siddhesh Poyarekar wrote: > Since Andreas' point of fixing the original bug has also been taken > care off, I'll commit this by the end of the week if there are no more > objections. I have pushed this now. Siddhesh
On 03/18/2015 05:28 AM, Siddhesh Poyarekar wrote: > On Wed, Mar 11, 2015 at 09:14:34PM +0530, Siddhesh Poyarekar wrote: >> Since Andreas' point of fixing the original bug has also been taken >> care off, I'll commit this by the end of the week if there are no more >> objections. > > I have pushed this now. Thanks. c.
diff --git a/sunrpc/svc.c b/sunrpc/svc.c index ccf0902..30c3a93 100644 --- a/sunrpc/svc.c +++ b/sunrpc/svc.c @@ -97,8 +97,8 @@ xprt_register (SVCXPRT *xprt) if (xports == NULL) { - xports = (SVCXPRT **) malloc (_rpc_dtablesize () * sizeof (SVCXPRT *)); - if (xports == NULL) /* Don´t add handle */ + xports = (SVCXPRT **) calloc (_rpc_dtablesize (), sizeof (SVCXPRT *)); + if (xports == NULL) /* Don't add handle */ return; }