[PATCH v2 1/1] ntdll: Fix reading stale Information from IOSB.

[Jin-oh Kang]

On Tue, May 31, 2022, 7:52 AM Zebediah Figura wrote:

> On 5/30/22 14:20, Jinoh Kang wrote:
> > +
> > +    /* GetOverlappedResultEx() skips waiting for the event/file handle
> if the
> > +     * Status field indicates completion, and returns the result from
> the
> > +     * Information field.
> > +     *
> > +     * Hence, we have to ensure that writes to the Information field
> are seen
> > +     * from the other threads *before* writes to the Status field.
> > +     *
> > +     * Otherwise, a race condition results: if writing the Status field
> has
> > +     * been completed but the subsequent update to the Information
> field has
> > +     * not, the application may end up reading stale value from it.
> > +     *
> > +     * The race condition can be especially problematic with
> applications that
> > +     * use the HasOverlappedIoCompleted() macro in a tight loop to
> busy-wait
> > +     * for completion of overlapped I/O.
> > +     */
> > +    MemoryBarrier();  /* TODO: consider replacing with a store-release.
> */
> > +    *(volatile NTSTATUS *)status_ptr = status;
> >   }
> >
> >   static inline client_ptr_t iosb_client_ptr( IO_STATUS_BLOCK *io )
>
> Unpaired MemoryBarrier() is pretty much always wrong. (In fact, I think
> unpaired anything would be, here.)
>

By pairing, do you mean that another MemoryBarrier() should follow the
status write?


> It also doesn't seem clear to me that it's actually better than a
> store-release, as you point out. Ideally what we want is ReadAcquire()
> and WriteRelease(), but even setting that aside, a pair of interlocked
> operations will probably be an improvement by themselves.
>

As in, convert both status and information writes to interlocked
operations, or replace the barrier with a dummy atomic op?

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20220531/195a43e6/attachment.htm>