Re: [ros-dev] [ros-diffs] [aandrejevic] 68607: [FASTFAT] Irp->UserBuffer being NULL doesn't indicate any error. It could be that the caller really wants the result stored at address NULL (which can be valid, and is valid by...

[Pierre Schweitzer]

This patch, as it, is really dangerous. MS allows null pointer in its
Read/Write routines, but they are wrapped inside a SEH block, to prevent
nasty things to happen in case of a null pointer dereference.

Here we don't, meaning that a null pointer here will cause major damages.

Furthermore, your commit regressed ntdll:file:
https://reactos.org/sites/all/modules/reactos/testman/compare.php?ids=40042,...

Regards,

On 08/07/2015 05:30 AM, [e-mail] wrote:
> Author: aandrejevic
> Date: Fri Aug  7 03:30:05 2015
> New Revision: 68607
>
> URL: http://svn.reactos.org/svn/reactos?rev=68607&view=rev
> Log:
> [FASTFAT]
> Irp->UserBuffer being NULL doesn't indicate any error. It could be that the
> caller really wants the result stored at address NULL (which can be valid,
> and is valid by default for programs like NTVDM).
>
> Modified:
>     trunk/reactos/drivers/filesystems/fastfat/rw.c
>
> Modified: trunk/reactos/drivers/filesystems/fastfat/rw.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/fastfat...
> ==============================================================================
> --- trunk/reactos/drivers/filesystems/fastfat/rw.c	[iso-8859-1] (original)
> +++ trunk/reactos/drivers/filesystems/fastfat/rw.c	[iso-8859-1] Fri Aug  7 03:30:05 2015
> @@ -656,7 +656,7 @@
>      }
>  Buffer = VfatGetUserBuffer(IrpContext->Irp, BooleanFlagOn(IrpContext->Irp->Flags, IRP_PAGING_IO));
> if (!Buffer)
> if (!Buffer && IrpContext->Irp->MdlAddress)
> {
>     Status = STATUS_INVALID_USER_BUFFER;
>     goto ByeBye;
>
> @@ -927,7 +927,7 @@
>      OldFileSize = Fcb->RFCB.FileSize;
>  Buffer = VfatGetUserBuffer(IrpContext->Irp, BooleanFlagOn(IrpContext->Irp->Flags, IRP_PAGING_IO));
> if (!Buffer)
> if (!Buffer && IrpContext->Irp->MdlAddress)
> {
>     Status = STATUS_INVALID_USER_BUFFER;
>     goto ByeBye;
-- 
Pierre Schweitzer [e-mail]
System & Network Administrator
Senior Kernel Developer
ReactOS Deutschland e.V.

[Alexander Andrejevic]

Then I guess someone should wrap that up in a SEH block. (I can do it.)
Who ever thought doing "neither direct nor buffered" I/O without SEH
is a good idea anyway?

Regards,
Alex

On Fri, Aug 07, 2015 at 10:31:58AM +0200, Pierre Schweitzer wrote:
> This patch, as it, is really dangerous. MS allows null pointer in its
> Read/Write routines, but they are wrapped inside a SEH block, to prevent
> nasty things to happen in case of a null pointer dereference.
>
> Here we don't, meaning that a null pointer here will cause major damages.
>
> Furthermore, your commit regressed ntdll:file:
> https://reactos.org/sites/all/modules/reactos/testman/compare.php?ids=40042,...
>
> Regards,
>
> On 08/07/2015 05:30 AM, [e-mail] wrote:
> > Author: aandrejevic
> > Date: Fri Aug  7 03:30:05 2015
> > New Revision: 68607
> >
> > URL: http://svn.reactos.org/svn/reactos?rev=68607&view=rev
> > Log:
> > [FASTFAT]
> > Irp->UserBuffer being NULL doesn't indicate any error. It could be that the
> > caller really wants the result stored at address NULL (which can be valid,
> > and is valid by default for programs like NTVDM).
> >
> > Modified:
> >     trunk/reactos/drivers/filesystems/fastfat/rw.c
> >
> > Modified: trunk/reactos/drivers/filesystems/fastfat/rw.c
> > URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/fastfat...
> > ==============================================================================
> > --- trunk/reactos/drivers/filesystems/fastfat/rw.c	[iso-8859-1] (original)
> > +++ trunk/reactos/drivers/filesystems/fastfat/rw.c	[iso-8859-1] Fri Aug  7 03:30:05 2015
> > @@ -656,7 +656,7 @@
> >      }
> >  Buffer = VfatGetUserBuffer(IrpContext->Irp, BooleanFlagOn(IrpContext->Irp->Flags, IRP_PAGING_IO));
> > if (!Buffer)
> > if (!Buffer && IrpContext->Irp->MdlAddress)
> > {
> >     Status = STATUS_INVALID_USER_BUFFER;
> >     goto ByeBye;
> >
> > @@ -927,7 +927,7 @@
> >      OldFileSize = Fcb->RFCB.FileSize;
> >  Buffer = VfatGetUserBuffer(IrpContext->Irp, BooleanFlagOn(IrpContext->Irp->Flags, IRP_PAGING_IO));
> > if (!Buffer)
> > if (!Buffer && IrpContext->Irp->MdlAddress)
> > {
> >     Status = STATUS_INVALID_USER_BUFFER;
> >     goto ByeBye;
>
> -- 
> Pierre Schweitzer [e-mail]
> System & Network Administrator
> Senior Kernel Developer
> ReactOS Deutschland e.V.
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev
-- 
Alexander Andrejevic [e-mail]
SDF Public Access UNIX System - http://sdf.lonestar.org