Re: [ros-svn] [hbirr] 14964: Do always set the UserIosb of an irp in IoSecondStageCompletion.

[Alex Ionescu]

[e-mail] wrote:
> Do always set the UserIosb of an irp in IoSecondStageCompletion.
>
> Updated files:
> trunk/reactos/ntoskrnl/io/irp.c

This is incorrect.

1) The IOSB should not always be set. Create a driver and fail an 
operation that you send to yourself by an IRP. Make that IRP not 
SYNCH_API, or better yet, make sure you don't have a File Object.
You will notice that the Status Block is not touched.

2) The IOSB is not checked if it exists, it should ALWAYS be there. IRPs 
without a IOSB are invalid. To verify this, set the IOSB of your IRP to 
0 and run Windows with a Debugger. You will see that it will break in 
many places, because Windows has simply placed SEH to make sure that the 
write is valid. So the correct thing to do is wrap the write in SEH, 
which protects both against invalid pointers and zero ones, but that 
still doesn't mean they are"valid"  and should be checked that way.

Best regards,
Alex Ionescu

[Hartmut Birr]

Alex Ionescu wrote:
> [e-mail] wrote:
> > Do always set the UserIosb of an irp in IoSecondStageCompletion.
> >
> > Updated files:
> > trunk/reactos/ntoskrnl/io/irp.c
>
> This is incorrect.
> The IOSB should not always be set. Create a driver and fail an
>
> operation that you send to yourself by an IRP. Make that IRP not
> SYNCH_API, or better yet, make sure you don't have a File Object.
> You will notice that the Status Block is not touched.
> The IOSB is not checked if it exists, it should ALWAYS be there.
>
> IRPs without a IOSB are invalid. To verify this, set the IOSB of your
> IRP to 0 and run Windows with a Debugger. You will see that it will
> break in many places, because Windows has simply placed SEH to make
> sure that the write is valid. So the correct thing to do is wrap the
> write in SEH, which protects both against invalid pointers and zero
> ones, but that still doesn't mean they are"valid"  and should be
> checked that way.
>
> Best regards,
> Alex Ionescu
> _______________________________________________
> Ros-dev mailing list
> [e-mail]
> http://reactos.com:8080/mailman/listinfo/ros-dev

Please look at bug #609.

- Hartmut

[Alex Ionescu]

Hartmut Birr wrote:
> Alex Ionescu wrote:
> > [e-mail] wrote:
> > > Do always set the UserIosb of an irp in IoSecondStageCompletion.
> > >
> > > Updated files:
> > > trunk/reactos/ntoskrnl/io/irp.c
> >
> > This is incorrect.
> > The IOSB should not always be set. Create a driver and fail an
> >
> > operation that you send to yourself by an IRP. Make that IRP not
> > SYNCH_API, or better yet, make sure you don't have a File Object.
> > You will notice that the Status Block is not touched.
> > The IOSB is not checked if it exists, it should ALWAYS be there.
> >
> > IRPs without a IOSB are invalid. To verify this, set the IOSB of your
> > IRP to 0 and run Windows with a Debugger. You will see that it will
> > break in many places, because Windows has simply placed SEH to make
> > sure that the write is valid. So the correct thing to do is wrap the
> > write in SEH, which protects both against invalid pointers and zero
> > ones, but that still doesn't mean they are"valid"  and should be
> > checked that way.
> >
> > Best regards,
> > Alex Ionescu
> > _______________________________________________
> > Ros-dev mailing list
> > [e-mail]
> > http://reactos.com:8080/mailman/listinfo/ros-dev
>
> Please look at bug #609.
> Hartmut

Ok, well that indicates that our VFAT driver is broken. It should be 
fixed instead of breaking correct code.

Best regards,
Alex Ionescu

[WaxDragon]

This change breaks FTP for me.  FTP was already broken after Alex's
IRP changes, and I had it running again with a patch from arty and a
one-liner I added.  Now, networking may be broken in a similar way to
the vfat driver, but I would like this change to be confirmed.

WD
-- 
"<tinus_> and also win32k should be fixed up a bit"

[Hartmut Birr]

Alex Ionescu wrote:
> Ok, well that indicates that our VFAT driver is broken. It should be
> fixed instead of breaking correct code.

That isn't correct. The bug is in IoCreateFile or/and
SecondStageCompletion. It is possible that a FS driver returns
STATUS_PENDING and does later complete the irp with an error.
IoCreateFile does wait on the FileObject event  but in some cases
IoSecontCompletion doesn't set the result in Irp->UserIosb. This is the
reason for bug #609. There exists more bugs. All Directory/File function
may always provide an user event handle. In some conditions they must
provide an user event handle. IoSecondStageCompletion does sometimes
dereference the user event and sometimes not. An other problem is the
copying of  the results to Irp->UserIosb. The old code has used
MmSafeCopyToUser. It must use this function or guard the copy operation
by an exception frame and use MmProbeForWrite. If the operation is
asynchronous, the calling thread may be buggy and does dereference the
status block. In this case ros does crash. I've the feeling, you should
start a second rewrite of the  io completion code.

- Hartmut

[Alex Ionescu]

Hartmut Birr wrote:
> Alex Ionescu wrote:
> > Ok, well that indicates that our VFAT driver is broken. It should be
> > fixed instead of breaking correct code.
>
> That isn't correct. The bug is in IoCreateFile or/and
> SecondStageCompletion. It is possible that a FS driver returns
> STATUS_PENDING and does later complete the irp with an error.

Ok, that's normal.
> IoCreateFile does wait on the FileObject event but in some cases
> IoSecontCompletion doesn't set the result in Irp->UserIosb. This is the
> reason for bug #609.

I would say the bug is in IoCreateFile, but I haven't had much time to 
look at it.
> There exists more bugs. All Directory/File function
> may always provide an user event handle. In some conditions they must
> provide an user event handle.

They must only provide the handle if the operation will need one, 
depending if the file object is opened with SYNCH_IO or not. See the 
recent fixes I made.
> IoSecondStageCompletion does sometimes
> dereference the user event and sometimes not.

Yes, and that is correct behavior. It should not get derefefenced if 
it's a KEVENT. We know it's a KEVENT if the operation doesn't have 
IRP_SYNCH_API.
> An other problem is the
> copying of  the results to Irp->UserIosb. The old code has used
> MmSafeCopyToUser. It must use this function or guard the copy operation
> by an exception frame and use MmProbeForWrite. If the operation is
> asynchronous, the calling thread may be buggy and does dereference the
> status block. In this case ros does crash.

This is my fault, you are right that the copy should be SEHed.
> I've the feeling, you should
> start a second rewrite of the  io completion code.

I have some additional changes locally but really no time to commit them 
until around May 15th when I finish school.
Thanks for your help and valuable comments.

There are so many problems because so many things depend on the old 
code. But I can guarantee you that my changes are correct.
> Hartmut

Best regards,
Alex Ionescu