Page MenuHomeFreeBSD
Differential D6572

Quell false positives in svc_vc_create and svc_vc_create_conn with cd and xprt
ClosedPublic
Actions

Authored by ngie on May 26 2016, 6:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 9, 12:23 AM
Unknown Object (File)
Mon, Dec 2, 2:42 AM
Unknown Object (File)
Mon, Nov 25, 7:35 AM
Unknown Object (File)
Oct 29 2024, 11:38 AM
Unknown Object (File)
Oct 28 2024, 5:18 AM
Unknown Object (File)
Oct 20 2024, 1:54 AM
Unknown Object (File)
Oct 19 2024, 5:51 AM
Unknown Object (File)
Oct 2 2024, 5:54 AM
View All 45 Files
Subscribers
cem
imp
markj
pfg
truckman

Details

Reviewers
markj
truckman
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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3962
Build 4005: arc lint + arc unit

Event Timeline

ngie updated this revision to Diff 16894.May 26 2016, 6:58 AM
ngie retitled this revision from to Quell false positives in svc_vc_create and svc_vc_create_conn with cd and xprt.
ngie updated this object.
ngie edited the test plan for this revision. (Show Details)
ngie added subscribers: cem, markj, pfg.
Herald added a subscriber: imp. · View Herald TranscriptMay 26 2016, 6:58 AM
ngie added a subscriber: truckman.May 26 2016, 6:58 AM
truckman accepted this revision.May 26 2016, 12:23 PM
truckman added a reviewer: truckman.

Looks fine to me.

This revision is now accepted and ready to land.May 26 2016, 12:23 PM
cem added a comment.May 26 2016, 3:55 PM

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 added a comment.May 26 2016, 4:23 PM
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 added a comment.May 26 2016, 4:35 PM
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 added a comment.May 26 2016, 5:26 PM
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 accepted this revision.May 26 2016, 8:47 PM
markj added a reviewer: 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.

ngie closed this revision.May 27 2016, 8:51 AM

Revision Contents
Changeset List

PathSize
sys/
rpc/
24 lines

Diff 16894

View Options

sys/rpc/svc_vc.c

Loading...