Fixes for the NFSERR_RETERR commit.

- Make sure to set the mbuf pointers and the dpos pointer correctly
in nfs_request() before returning an error.

- Fix potential mbufs leaks in nfs_{read,write}rpc for v2. The reply
mbuf was not being freed before the jump to nfsmout.
- Reorder error handling to prevent an error case being treated as a
non-error case.

Fixes issues noticed by sthen@ and ajacoutot@. Tested by both of them.
Reviewd by oga@
OK blambert@

2 files changed, 60 insertions, 64 deletions

diff --git a/sys/nfs/nfs_socket.c b/sys/nfs/nfs_socket.c
index d11135914a4..420187d7903 100644
--- a/sys/nfs/nfs_socket.c
+++ b/sys/nfs/nfs_socket.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: nfs_socket.c,v 1.86 2009/07/13 15:39:55 thib Exp $	*/
+/*	$OpenBSD: nfs_socket.c,v 1.87 2009/07/18 14:40:31 thib Exp $	*/
 /*	$NetBSD: nfs_socket.c,v 1.27 1996/04/15 20:20:00 thorpej Exp $	*/
 
 /*
@@ -994,11 +994,8 @@ tryagain:
 	mrep = rep->r_mrep;
 	md = rep->r_md;
 	dpos = rep->r_dpos;
-	if (error) {
-		m_freem(rep->r_mreq);
-		pool_put(&nfsreqpl, rep);
-		return (error);
-	}
+	if (error)
+		goto nfsmout;
 
 	/*
 	 * break down the rpc header and check if ok
@@ -1008,11 +1005,8 @@ tryagain:
 		if (*tl == rpc_mismatch)
 			error = EOPNOTSUPP;
 		else
-			error = EACCES;
-		m_freem(mrep);
-		m_freem(rep->r_mreq);
-		pool_put(&nfsreqpl, rep);
-		return (error);
+			error = EACCES;	/* Should be EAUTH. */
+		goto nfsmout;
 	}
 
 	/*
@@ -1053,17 +1047,15 @@ tryagain:
 			if (error == ESTALE)
 				cache_purge(rep->r_vp);
 		}
-
-		*mrp = mrep;
-		*mdp = md;
-		*dposp = dpos;
-
 		goto nfsmout;
 	}
 
 	error = EPROTONOSUPPORT;
 
 nfsmout:
+	*mrp = mrep;
+	*mdp = md;
+	*dposp = dpos;
 	m_freem(rep->r_mreq);
 	pool_put(&nfsreqpl, rep);
 	return (error);
diff --git a/sys/nfs/nfs_vnops.c b/sys/nfs/nfs_vnops.c
index 91efaec692d..56e8641cf5d 100644
--- a/sys/nfs/nfs_vnops.c
+++ b/sys/nfs/nfs_vnops.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: nfs_vnops.c,v 1.117 2009/07/13 15:39:55 thib Exp $	*/
+/*	$OpenBSD: nfs_vnops.c,v 1.118 2009/07/18 14:40:31 thib Exp $	*/
 /*	$NetBSD: nfs_vnops.c,v 1.62.4.1 1996/07/08 20:26:52 jtc Exp $	*/
 
 /*
@@ -1034,17 +1034,19 @@ nfs_readrpc(vp, uiop)
 
 		error = nfs_request(vp, mreq, NFSPROC_READ, uiop->uio_procp,
 		    VTONFS(vp)->n_rcred, &mrep, &md, &dpos);
+		if (v3)
+			nfsm_postop_attr(vp, attrflag);
+		if (error) {
+			m_freem(mrep);
+			goto nfsmout;
+		}
 
 		if (v3) {
-			nfsm_postop_attr(vp, attrflag);
-			if (error) {
-				m_freem(mrep);
-				goto nfsmout;
-			}
 			nfsm_dissect(tl, u_int32_t *, 2 * NFSX_UNSIGNED);
 			eof = fxdr_unsigned(int, *(tl + 1));
-		} else if (error == 0)
+		} else {
 			nfsm_loadattr(vp, NULL);
+		}
 
 		nfsm_strsiz(retlen, nmp->nm_rsize);
 		nfsm_mtouio(uiop, retlen);
@@ -1115,56 +1117,58 @@ nfs_writerpc(vp, uiop, iomode, must_commit)
 
 		error = nfs_request(vp, mreq, NFSPROC_WRITE, uiop->uio_procp,
 		    VTONFS(vp)->n_wcred, &mrep, &md, &dpos);
+		if (v3) {
+			wccflag = NFSV3_WCCCHK;
+			nfsm_wcc_data(vp, wccflag);
+		}
 
-		if (error && !v3)
+		if (error) {
+			m_freem(mrep);
 			goto nfsmout;
+		}
 
 		if (v3) {
 			wccflag = NFSV3_WCCCHK;
-			nfsm_wcc_data(vp, wccflag);
-			if (!error) {
-				nfsm_dissect(tl, u_int32_t *, 2 * NFSX_UNSIGNED
-					+ NFSX_V3WRITEVERF);
-				rlen = fxdr_unsigned(int, *tl++);
-				if (rlen == 0) {
-					error = NFSERR_IO;
-					break;
-				} else if (rlen < len) {
-					backup = len - rlen;
-					uiop->uio_iov->iov_base =
-					    (char *)uiop->uio_iov->iov_base -
-					    backup;
-					uiop->uio_iov->iov_len += backup;
-					uiop->uio_offset -= backup;
-					uiop->uio_resid += backup;
-					len = rlen;
-				}
-				commit = fxdr_unsigned(int, *tl++);
-
-				/*
-				 * Return the lowest committment level
-				 * obtained by any of the RPCs.
-				 */
-				if (committed == NFSV3WRITE_FILESYNC)
-					committed = commit;
-				else if (committed == NFSV3WRITE_DATASYNC &&
-					commit == NFSV3WRITE_UNSTABLE)
-					committed = commit;
-				if ((nmp->nm_flag & NFSMNT_HASWRITEVERF) == 0) {
-				    bcopy((caddr_t)tl, (caddr_t)nmp->nm_verf,
-					NFSX_V3WRITEVERF);
-				    nmp->nm_flag |= NFSMNT_HASWRITEVERF;
-				} else if (bcmp((caddr_t)tl,
-				    (caddr_t)nmp->nm_verf, NFSX_V3WRITEVERF)) {
-				    *must_commit = 1;
-				    bcopy((caddr_t)tl, (caddr_t)nmp->nm_verf,
-					NFSX_V3WRITEVERF);
-				}
+			nfsm_dissect(tl, u_int32_t *, 2 * NFSX_UNSIGNED
+				+ NFSX_V3WRITEVERF);
+			rlen = fxdr_unsigned(int, *tl++);
+			if (rlen == 0) {
+				error = NFSERR_IO;
+				break;
+			} else if (rlen < len) {
+				backup = len - rlen;
+				uiop->uio_iov->iov_base =
+				    (char *)uiop->uio_iov->iov_base -
+				    backup;
+				uiop->uio_iov->iov_len += backup;
+				uiop->uio_offset -= backup;
+				uiop->uio_resid += backup;
+				len = rlen;
+			}
+			commit = fxdr_unsigned(int, *tl++);
+
+			/*
+			 * Return the lowest committment level
+			 * obtained by any of the RPCs.
+			 */
+			if (committed == NFSV3WRITE_FILESYNC)
+				committed = commit;
+			else if (committed == NFSV3WRITE_DATASYNC &&
+				commit == NFSV3WRITE_UNSTABLE)
+				committed = commit;
+			if ((nmp->nm_flag & NFSMNT_HASWRITEVERF) == 0) {
+			    bcopy((caddr_t)tl, (caddr_t)nmp->nm_verf,
+				NFSX_V3WRITEVERF);
+			    nmp->nm_flag |= NFSMNT_HASWRITEVERF;
+			} else if (bcmp((caddr_t)tl,
+			    (caddr_t)nmp->nm_verf, NFSX_V3WRITEVERF)) {
+			    *must_commit = 1;
+			    bcopy((caddr_t)tl, (caddr_t)nmp->nm_verf,
+				NFSX_V3WRITEVERF);
 			}
 		} else {
 			nfsm_loadattr(vp, NULL);
 		}
-
 		if (wccflag)
 		    VTONFS(vp)->n_mtime = VTONFS(vp)->n_vattr.va_mtime;
 		m_freem(mrep);