Quell false positives in svc_vc_create and svc_vc_create_conn with cd and xprt

[ngie]

Summary

Quell false positives in svc_vc_create and svc_vc_create_conn with cd and xprt

Both cd and xprt will be non-NULL after their respective malloc(9) wrappers are
called (mem_alloc and svc_xprt_alloc, which calls mem_alloc) as mem_alloc
always gets called with M_WAITOK|M_ZERO today. Thus, testing for them being
non-NULL is incorrect -- it misleads Coverity and it misleads the reader.

Remove some unnecessary NULL initializations

MFC after: 2 weeks
Reported by: Coverity
CID: 1007338, 1007339, 1007340
Sponsored by: EMC / Isilon Storage Division

[truckman]

Looks fine to me.

[cem]

I don't dislike the if() { } cleanup pattern on exit, even if it is impossible with today's code.

Does this code ever get compiled in userspace? Do the mem_alloc routines do the same thing there?

[truckman]

In D6572#138990, @cem wrote:

I don't dislike the if() { } cleanup pattern on exit, even if it is impossible with today's code.

Does this code ever get compiled in userspace? Do the mem_alloc routines do the same thing there?

IF mem_alloc() failed, the check at the end would not be helpful because earlier code would have already tried to dereference the NULL pointer and croaked. If you have a check, then it has to be before the first use.

[cem]

In D6572#139010, @truckman wrote:

IF mem_alloc() failed, the check at the end would not be helpful because earlier code would have already tried to dereference the NULL pointer and croaked. If you have a check, then it has to be before the first use.

That's not where a NULL check is useful. Imagine some other error happens earlier, before the memory allocation.

In the current code, all the other errors happen after the memory allocation. But say some new code was added to the top of these functions and goto ...cleanup; on failure. Then the existing if(){} cleanups would still be valid.

(@ngie, it's the same pattern as one we use in Isilon C style code.)

[ngie]

In D6572#138990, @cem wrote:

I don't dislike the if() { } cleanup pattern on exit, even if it is impossible with today's code.

Does this code ever get compiled in userspace? Do the mem_alloc routines do the same thing there?

mem_alloc is #define'd to calloc(..) in userspace, and malloc(..., M_WAITOK|M_ZERO) in kernel space:

63 #ifdef _KERNEL
64 #ifdef _SYS_MALLOC_H_
65 MALLOC_DECLARE(M_RPC);
66 #endif
67 #define mem_alloc(bsize)        malloc(bsize, M_RPC,  M_WAITOK|M_ZERO)
68 #define mem_free(ptr, bsize)    free(ptr, M_RPC)
69 #else
70 #define mem_alloc(bsize)        calloc(1, bsize)
71 #define mem_free(ptr, bsize)    free(ptr)
72 #endif

As for the question "does this code ever get compiled in userspace?", the answer is no. There's a parallel implementation in lib/libc/rpc however (*sigh*); both lineages probably date back to the original Sun RPC port:

$ egrep -Ir 'svc_vc_create|svc_vc_create_conn' . | grep -v svn
./lib/libc/rpc/Symbol.map:      svc_vc_create;
./lib/libc/rpc/Makefile.inc:            rpc_svc_create.3 svc_vc_create.3 \
./lib/libc/rpc/svc_vc.c: *      xprt = svc_vc_create(sock, send_buf_size, recv_buf_size);
./lib/libc/rpc/svc_vc.c:svc_vc_create(int fd, u_int sendsize, u_int recvsize)
./lib/libc/rpc/svc_vc.c:                warnx("svc_vc_create: out of memory");
./lib/libc/rpc/svc_vc.c:                goto cleanup_svc_vc_create;
./lib/libc/rpc/svc_vc.c:                warnx("svc_vc_create: out of memory");
./lib/libc/rpc/svc_vc.c:                goto cleanup_svc_vc_create;
./lib/libc/rpc/svc_vc.c:                warnx("svc_vc_create: could not retrieve local addr");
./lib/libc/rpc/svc_vc.c:                goto cleanup_svc_vc_create;
./lib/libc/rpc/svc_vc.c:                warnx("svc_vc_create: no mem for local addr");
./lib/libc/rpc/svc_vc.c:                goto cleanup_svc_vc_create;
./lib/libc/rpc/svc_vc.c:cleanup_svc_vc_create:
./lib/libc/rpc/svc_vc.c:                warnx("svc_vc_create: could not retrieve local addr");
./lib/libc/rpc/svc_vc.c:                        warnx("svc_vc_create: no mem for local addr");
./lib/libc/rpc/rpc.3:.It Fn svc_vc_create
./lib/libc/rpc/rpc_soc.c: * Obsoleted by svc_vc_create().
./lib/libc/rpc/rpc_svc_create.3:.Nm svc_vc_create
./lib/libc/rpc/rpc_svc_create.3:.Fn svc_vc_create "const int fildes" "const u_int sendsz" "const u_int recvsz"
./lib/libc/rpc/rpc_svc_create.3:.It Fn svc_vc_create
./lib/libc/rpc/svc_generic.c:                           xprt = svc_vc_create(fd, sendsz, recvsz);
./lib/libc/rpc/svc_generic.c:            * svc_vc_create(), svc_fd_create() and svc_dg_create().
./include/rpc/svc.h:extern SVCXPRT *svc_vc_create(const int, const u_int, const u_int);
./include/rpc/svc.h: * Added for compatibility to old rpc 4.0. Obsoleted by svc_vc_create().
./contrib/netbsd-tests/fs/nfs/nfsservice/mountd.c:              tcptransp = svc_vc_create(tcpsock, RPC_MAXDATASIZE,
./contrib/gcc/sys-protos.h:extern SVCXPRT *              svc_vc_create(/* ??? */);
./usr.sbin/mountd/mountd.c:                     transp = svc_vc_create(fd, RPC_MAXDATASIZE,
./usr.sbin/rpc.lockd/lockd.c:                   xprt = svc_vc_create(fd, RPC_MAXDATASIZE, RPC_MAXDATASIZE);
./usr.sbin/gssd/gssd.c: xprt = svc_vc_create(fd, RPC_MAXDATASIZE, RPC_MAXDATASIZE);
./usr.sbin/ypserv/yp_main.c:                    transp = svc_vc_create(slep->sle_sock, RPC_MAXDATASIZE,
./sys/rpc/svc_vc.c:static SVCXPRT *svc_vc_create_conn(SVCPOOL *pool, struct socket *so,
./sys/rpc/svc_vc.c: *   xprt = svc_vc_create(sock, send_buf_size, recv_buf_size);
./sys/rpc/svc_vc.c:svc_vc_create(SVCPOOL *pool, struct socket *so, size_t sendsize,
./sys/rpc/svc_vc.c:             xprt = svc_vc_create_conn(pool, so, sa);
./sys/rpc/svc_vc.c:             goto cleanup_svc_vc_create;
./sys/rpc/svc_vc.c:cleanup_svc_vc_create:
./sys/rpc/svc_vc.c:svc_vc_create_conn(SVCPOOL *pool, struct socket *so, struct sockaddr *raddr)
./sys/rpc/svc_vc.c:             goto cleanup_svc_vc_create;
./sys/rpc/svc_vc.c:cleanup_svc_vc_create:
./sys/rpc/svc_vc.c:svc_vc_create_backchannel(SVCPOOL *pool)
./sys/rpc/svc_vc.c:      * svc_vc_create_conn will call xprt_register - we don't need
./sys/rpc/svc_vc.c:     new_xprt = svc_vc_create_conn(xprt->xp_pool, so, sa);
./sys/rpc/svc_generic.c:                                xprt = svc_vc_create(pool, so, sendsz, recvsz);
./sys/rpc/svc_generic.c:                 * svc_vc_create(), svc_fd_create() and svc_dg_create().
./sys/rpc/svc.h:extern SVCXPRT *svc_vc_create(SVCPOOL *, struct socket *,
./sys/rpc/svc.h:extern SVCXPRT *svc_vc_create_backchannel(SVCPOOL *);
./sys/rpc/svc.h:extern SVCXPRT *svc_vc_create(const int, const u_int, const u_int);
./sys/rpc/svc.h: * Added for compatibility to old rpc 4.0. Obsoleted by svc_vc_create().
./sys/fs/nfsserver/nfs_nfsdkrpc.c:              xprt = svc_vc_create(nfsrvd_pool, so, 0, 0);
./sys/fs/nfsclient/nfs_clkrpc.c:                xprt = svc_vc_create(nfscbd_pool, so, 0, 0);
./sys/fs/nfs/nfs_commonkrpc.c:                          xprt = svc_vc_create_backchannel(nfscbd_pool);

[markj]

I don't really like the notion of changing already-correct code to fix coverity false positives. You can mark coverity issues as false positives, why not just do that?

But this change makes the code a bit more straightforward than before, so I'm for it. These subroutines are pretty short, so I don't feel strongly about keeping the defensive checks. I do think those checks make it easier to reason about cleanup logic in longer functions.