Re: Alloc Error Handling in lib/libc/rpc/svc.c

[Konstantin Belousov]

On Fri, Apr 27, 2012 at 05:48:06PM -0400, Matt Miller wrote:
> In an OOM condition, we noticed a couple of mem_alloc handling bugs in
> this file.  Please let me know if a PR should be opened for these.
> 
> - No NULL checks after mem_alloc()'s:
> 
> SVCXPRT *
> svc_xprt_alloc()
> {
>       SVCXPRT *xprt;
>       SVCXPRT_EXT *ext;
> 
>       xprt = mem_alloc(sizeof(SVCXPRT));
>       memset(xprt, 0, sizeof(SVCXPRT));
>       ext = mem_alloc(sizeof(SVCXPRT_EXT));
>       memset(ext, 0, sizeof(SVCXPRT_EXT));
>       xprt->xp_p3 = ext;
>       ext->xp_auth.svc_ah_ops = &svc_auth_null_ops;
> 
>       return (xprt);
> }
> 
> - No lock release if mem_alloc() returns NULL:
> 
> void
> xprt_register(xprt)
>       SVCXPRT *xprt;
> {
>       int sock;
> 
>       assert(xprt != NULL);
> 
>       sock = xprt->xp_fd;
> 
>       rwlock_wrlock(&svc_fd_lock);
>       if (__svc_xports == NULL) {
>               __svc_xports = (SVCXPRT **)
>                       mem_alloc(FD_SETSIZE * sizeof(SVCXPRT *));
>               if (__svc_xports == NULL)
>                       return;
>               memset(__svc_xports, '\0', FD_SETSIZE * sizeof(SVCXPRT *));
>       }
>       if (sock < FD_SETSIZE) {
>               __svc_xports[sock] = xprt;
>               FD_SET(sock, &svc_fdset);
>               svc_maxfd = max(svc_maxfd, sock);
>       }
>       rwlock_unlock(&svc_fd_lock);
> }

Thank you for the report.
Does the patch below look fine to you ?

diff --git a/lib/libc/rpc/svc.c b/lib/libc/rpc/svc.c
index 282c2be..78a8ae1 100644
--- a/lib/libc/rpc/svc.c
+++ b/lib/libc/rpc/svc.c
@@ -108,8 +108,10 @@ xprt_register(xprt)
        if (__svc_xports == NULL) {
                __svc_xports = (SVCXPRT **)
                        mem_alloc(FD_SETSIZE * sizeof(SVCXPRT *));
-               if (__svc_xports == NULL)
+               if (__svc_xports == NULL) {
+                       rwlock_unlock(&svc_fd_lock);
                        return;
+               }
                memset(__svc_xports, '\0', FD_SETSIZE * sizeof(SVCXPRT *));
        }
        if (sock < FD_SETSIZE) {
@@ -565,8 +567,14 @@ svc_xprt_alloc()
        SVCXPRT_EXT *ext;
 
        xprt = mem_alloc(sizeof(SVCXPRT));
+       if (xprt == NULL)
+               return (NULL);
        memset(xprt, 0, sizeof(SVCXPRT));
        ext = mem_alloc(sizeof(SVCXPRT_EXT));
+       if (ext == NULL) {
+               mem_free(xprt, sizeof(SVCXPRT));
+               return (NULL);
+       }
        memset(ext, 0, sizeof(SVCXPRT_EXT));
        xprt->xp_p3 = ext;
        ext->xp_auth.svc_ah_ops = &svc_auth_null_ops;
diff --git a/lib/libc/rpc/svc_raw.c b/lib/libc/rpc/svc_raw.c
index 67bcba1..de95152 100644
--- a/lib/libc/rpc/svc_raw.c
+++ b/lib/libc/rpc/svc_raw.c
@@ -96,10 +96,22 @@ svc_raw_create()
                        mutex_unlock(&svcraw_lock);
                        return (NULL);
                }
-               if (__rpc_rawcombuf == NULL)
+               if (__rpc_rawcombuf == NULL) {
                        __rpc_rawcombuf = calloc(UDPMSGSIZE, sizeof (char));
+                       if (__rpc_rawcombuf == NULL) {
+                               free(srp);
+                               mutex_unlock(&svcraw_lock);
+                               return (NULL);
+                       }
+               }
                srp->raw_buf = __rpc_rawcombuf; /* Share it with the client */
                srp->server = svc_xprt_alloc();
+               if (srp->server == NULL) {
+                       free(__rpc_rawcombuf);
+                       free(srp);
+                       mutex_unlock(&svcraw_lock);
+                       return (NULL);
+               }
                svc_raw_private = srp;
        }
        srp->server->xp_fd = FD_SETSIZE;

pgpme1qB0yumP.pgp
Description: PGP signature