[PATCH] vfs_default: Fix passing of errno from async calls

[Christof Schmitt]

On Wed, Aug 23, 2017 at 04:01:57PM -0700, Jeremy Allison wrote:
> On Wed, Aug 23, 2017 at 03:58:36PM -0700, Christof Schmitt wrote:
> > On Wed, Aug 23, 2017 at 03:54:58PM -0700, Jeremy Allison wrote:
> > > On Wed, Aug 23, 2017 at 03:12:26PM -0700, Christof Schmitt via samba-technical wrote:
> > > > I found this while testing with a system where the actual write() system
> > > > call returned an error. This led to Samba hanging and never continuing
> > > > with the result of the write. The attached patch fixes the problem. From
> > > > what i understand, errno was never passed correctly back to the callers.
> > > 
> > > Wow - good catch !!!!
> > > 
> > > Can we change the assignment:
> > > 
> > >  -     state->err = errno;
> > >  +     state->vfs_aio_state.error = errno;
> > > 
> > > to:
> > > 
> > >  -     state->err = errno;
> > >  +	if (state->ret == -1) {
> > >  +	     state->vfs_aio_state.error = errno;
> > >  +	}
> > > 
> > > on each of the hunks, as we need to make
> > > sure that the thread only assigns non-zero
> > > to state->vfs_aio_state.error in the fail
> > > case. It's (theoretically) possible that
> > > the first go around returns -1, errno = EINTR and
> > > the second go around returns 0, but
> > > doesn't set errno (leaving it as the
> > > previous EINTR value). We should only
> > > set state->vfs_aio_state.error on a
> > > *known* error return IMHO.
> > > 
> > > If you're happy with that RB+.
> > 
> > Yes, that makes sense. Let me test with the change and then send the
> > updated patch.
> 
> Bug ID for back-ports:
> 
> https://bugzilla.samba.org/show_bug.cgi?id=12983

Tested again with the failing pwrite() call. The modified patch
(attached) catches that case as expected. I also added the bugzilla link
to the commit message.

Christof
-------------- next part --------------
From d73a513a0e33e234f2f54aa1c443e31501e5ee07 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 23 Aug 2017 14:37:28 -0700
Subject: [PATCH] vfs_default: Fix passing of errno from async calls

Current code assigns errno from async pthreadpool calls to the
vfs_default internal vfswrap_*_state.  The callers of the vfs_*_recv
functions expect the value from errno in vfs_aio_state.error.

Correctly assign errno to vfs_aio_state.error and remove the unused
internal err variable.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12983

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 source3/modules/vfs_default.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c
index fbfe654..0a56e45 100644
--- a/source3/modules/vfs_default.c
+++ b/source3/modules/vfs_default.c
@@ -738,7 +738,6 @@ static int vfswrap_init_pool(struct smbd_server_connection *conn)
 
 struct vfswrap_pread_state {
 	ssize_t ret;
-	int err;
 	int fd;
 	void *buf;
 	size_t count;
@@ -812,7 +811,9 @@ static void vfs_pread_do(void *private_data)
 				   state->offset);
 	} while ((state->ret == -1) && (errno == EINTR));
 
-	state->err = errno;
+	if (state->ret == -1) {
+		state->vfs_aio_state.error = errno;
+	}
 
 	PROFILE_TIMESTAMP(&end_time);
 
@@ -861,7 +862,6 @@ static ssize_t vfswrap_pread_recv(struct tevent_req *req,
 
 struct vfswrap_pwrite_state {
 	ssize_t ret;
-	int err;
 	int fd;
 	const void *buf;
 	size_t count;
@@ -935,7 +935,9 @@ static void vfs_pwrite_do(void *private_data)
 				   state->offset);
 	} while ((state->ret == -1) && (errno == EINTR));
 
-	state->err = errno;
+	if (state->ret == -1) {
+		state->vfs_aio_state.error = errno;
+	}
 
 	PROFILE_TIMESTAMP(&end_time);
 
@@ -984,7 +986,6 @@ static ssize_t vfswrap_pwrite_recv(struct tevent_req *req,
 
 struct vfswrap_fsync_state {
 	ssize_t ret;
-	int err;
 	int fd;
 
 	struct vfs_aio_state vfs_aio_state;
@@ -1045,7 +1046,9 @@ static void vfs_fsync_do(void *private_data)
 		state->ret = fsync(state->fd);
 	} while ((state->ret == -1) && (errno == EINTR));
 
-	state->err = errno;
+	if (state->ret == -1) {
+		state->vfs_aio_state.error = errno;
+	}
 
 	PROFILE_TIMESTAMP(&end_time);
 
-- 
1.8.3.1