Re: [PATCH v6 3/5] ntoskrnl.exe: Implement volume information queries for device files.

[Zebediah Figura (she/her)]

On 2/11/21 12:17 PM, Erich E. Hoover wrote:
> On Wed, Feb 10, 2021 at 9:33 PM Zebediah Figura (she/her)
>  wrote:
> > Taking a closer look since the last time I looked at this patch...
> > > From: "Erich E. Hoover" 
> > > Subject: [PATCH v6 3/5] ntoskrnl.exe: Implement volume information queries for device files.
> > > Message-Id: 
> > > Date: Sat, 6 Feb 2021 11:25:22 -0700
> > > ...
> > > - if (out_size)
> > > - {
> > > -
> > >    if (out_size > context->in_size)
> > > -
> > >    {
> > > -
> > >        if (!(out_buff = HeapAlloc( GetProcessHeap(), 0, out_size ))) return STATUS_NO_MEMORY;
> > > -
> > >        memcpy( out_buff, context->in_buff, context->in_size );
> > > -
> > >        to_free = context->in_buff;
> > > -
> > >        context->in_buff = out_buff;
> > > -
> > >    }
> > > -
> > >    else
> > > -
> > >        out_buff = context->in_buff;
> > > - }
> > > -
> >
> > This looks wrong. No data is ever passed in to this IRP, so it should
> > probably just always allocate an output buffer, like dispatch_read().
>
> This was copied from dispatch_ioctl under the assumption that some
> future usage might need it, but you are right - according to the
> documentation this IRP has no need for this.  I'll go ahead and match
> the behavior in dispatch_read.
> > [There's probably an argument to reusing context.in_buff already
> > allocated in dispatch_read() and here, but we'd need to store the
> > capacity of the buffer somewhere. This patch will never reuse it, as
> > in_size should always be 0.]
>
> It looks to me like dispatch_read doesn't use context->in_buff, am I
> missing something here?  (Looks to me like it always does a heap
> allocation.)

Sorry, that was probably confusing phrasing, I mean it was already
allocated in __wine_ntoskrnl_main_loop(), and we could potentially use
it in dispatch_read() and here.
> > > ...
> > > -
> > >    FILE_FS_VOLUME_INFORMATION *info = irp->AssociatedIrp.SystemBuffer;
> > > -
> > >    static WCHAR label[] = L"WineTestDriver";
> >
> > Missing "const"?
>
> Yup, sorry about that.
> > > -
> > >    ULONG serial = 0xdeadbeef;
> > > -
> > > -
> > >    if (length < sizeof(FILE_FS_VOLUME_INFORMATION))
> > > -
> > >    {
> > > -
> > >        ret = STATUS_INFO_LENGTH_MISMATCH;
> > > -
> > >        break;
> > > -
> > >    }
> > > -
> > > -
> > >    info->VolumeCreationTime.QuadPart = 0; /* FIXME */
> >
> > "FIXME" seems a bit odd in a test like this, was that a copy/paste error?
>
> Yes, that's a copy/paste error from patch 4.
> > > ...