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, aandrejevic(a)svn.reactos.org 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/fastfa…
>  ==============================================================================
>  --- 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 <pierre(a)reactos.org>
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, aandrejevic(a)svn.reactos.org 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/fastfa…
> >  ==============================================================================
> >  --- 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 <pierre(a)reactos.org>
>  System & Network Administrator
>  Senior Kernel Developer
>  ReactOS Deutschland e.V.
>   
>   _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev 
--
Alexander Andrejevic <theflash(a)sdf.lonestar.org>
SDF Public Access UNIX System - http://sdf.lonestar.org