[PATCH] Initialize IO_STATUS_BLOCK for pread, pwrite

[Corinna Vinschen]

On Nov 28 02:28, Mark Geisert wrote:
> Corinna Vinschen wrote:
> > On Nov 28 00:03, Mark Geisert wrote:
> > > Mark Geisert wrote:
> > > > ---
> > > >  winsup/cygwin/fhandler_disk_file.cc | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/winsup/cygwin/fhandler_disk_file.cc b/winsup/cygwin/fhandler_disk_file.cc
> > > > index 5dfcae4d9..2ead9948c 100644
> > > [...]
> > > 
> > > Oops, I neglected to include an explanatory comment. Issuing simultaneous
> > > pwrite(s) on one file descriptor from multiple threads, as one might do in a
> > > forthcoming POSIX aio implementation, sometimes results in garbage status in
> > > the IO_STATUS_BLOCK on return from NtWriteFile(). Zeroing beforehand made
> > > the issue go away.
> > > 
> > > This is mildly concerning to me because there are many other uses of
> > > IO_STATUS_BLOCK in the Cygwin DLL that haven't seemed to have needed
> > > initialization.
> > > 
> > > Puzzledly,
> > 
> > Ok, let's start with, why did you tweak pread if you only observed
> > a problem in pwrite?
> 
> Optimism? :-)  No, you're correct; I was getting ahead of myself.
> 
> >                       In terms of pread, we already have a very recent
> > patch series:
> > 
> > https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=46702f92ea49
> > https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=c983aa48798d
> > https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=181fe5d2edac
> > 
> > In this case it turned out that the problem was related to hitting EOF.
> > I wonder if we hit a similar problem here.
> > 
> > Two points:
> > 
> > - Did you check the status code returned by NtWriteFile?  Not all non-0
> >   status codes fail the !NT_SUCCESS (status) test.
> 
> I did check the status code but don't recall what it was.  The symptom I was
> seeing was outlandish io.Information values being returned by pwrite().  Far
> larger than the number requested in the call to pwrite() and NtWriteFile().

That doesn't mean it has been returned by NtWriteFile.  Random values
suggest NtWriteFile didn *set* a value in the first place, so what
you see is the random value from the stack position io is in.  And
that in turn suggests the status code should indicate why io wasn't
written by NtWriteFile.  If you're playing with async IO, is it possible
the status code indicates something like STATUS_TIMEOUT or STATUS_PENDING,
both of which are treated as NT_SUCCESS?

> > - Do you have a simple, self-contained testcase?
> 
> That would be difficult.  I can supply an strace excerpt just showing the
> region of these simultaneous pwrite() calls, without this patch.  If it's
> too large I'll put it somewhere and post a link (but I don't think it will
> be).

Alternatively, what you should do is adding debug_printf statements
before and after NtWriteFile, kind of like this...

diff --git a/winsup/cygwin/fhandler_disk_file.cc b/winsup/cygwin/fhandler_disk_file.cc
index 5dfcae4d9eb7..149c80484213 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -1635,8 +1635,10 @@ fhandler_disk_file::pwrite (void *buf, size_t count, off_t offset)
 
       if (!prw_handle && prw_open (true))
 	goto non_atomic;
+      debug_printf ("Before NtWriteFile, io %Y", io.Information);
       status = NtWriteFile (prw_handle, NULL, NULL, NULL, &io, buf, count,
 			    &off, NULL);
+      debug_printf ("After NtWriteFile, io %Y, status %y", io.Information);
       if (!NT_SUCCESS (status))
 	{
 	  __seterrno_from_nt_status (status);

...and report your findings.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-patches/attachments/20171128/6046022e/attachment.sig>