Re: [ros-diffs] [navaraf] 17604: Check for Request == NULL.

[Hartmut Birr]

[e-mail] wrote:
> Check for Request == NULL.
>
> Modified: trunk/reactos/ntoskrnl/lpc/reply.c
>
> *Modified: trunk/reactos/ntoskrnl/lpc/reply.c*
>
> --- trunk/reactos/ntoskrnl/lpc/reply.c	2005-09-02 11:29:40 UTC (rev 17603)
> +++ trunk/reactos/ntoskrnl/lpc/reply.c	2005-09-02 13:12:44 UTC (rev 17604)
> @@ -263,6 +263,12 @@
>
> Request = EiDequeueMessagePort(Port);
>    KeReleaseSpinLock(&Port->Lock, oldIrql);
>
> if (Request == NULL)
> {
>   ObDereferenceObject(Port);
>   return STATUS_UNSUCCESSFUL;
> }
>
> if (Request->Message.u2.s2.Type == LPC_CONNECTION_REQUEST)
>    {
>      PORT_MESSAGE Header;

Hi,

I think that this fix doesn't solve the real problem. The caller waits
on a semaphore and the semaphore is only signaled if a message is in the
queue. This means, the caller can't get a NULL pointer from
EiDequeueMessagePort(). The real problem is anywhere in
KeWaitForSingleObject, which does return STATUS_USER_APC for a non
alertable wait. That is wrong.

- Hartmut

[Filip Navara]

Hartmut Birr wrote:
> [e-mail] wrote:
> > Check for Request == NULL.
> >
> > Modified: trunk/reactos/ntoskrnl/lpc/reply.c
> >
> > *Modified: trunk/reactos/ntoskrnl/lpc/reply.c*
> >
> > --- trunk/reactos/ntoskrnl/lpc/reply.c	2005-09-02 11:29:40 UTC (rev 17603)
> > +++ trunk/reactos/ntoskrnl/lpc/reply.c	2005-09-02 13:12:44 UTC (rev 17604)
> > @@ -263,6 +263,12 @@
> >
> > Request = EiDequeueMessagePort(Port);
> >   KeReleaseSpinLock(&Port->Lock, oldIrql);
> >
> > if (Request == NULL)
> > {
> >   ObDereferenceObject(Port);
> >   return STATUS_UNSUCCESSFUL;
> > }
> >
> > if (Request->Message.u2.s2.Type == LPC_CONNECTION_REQUEST)
> >   {
> >     PORT_MESSAGE Header;
>
> Hi,
>
> I think that this fix doesn't solve the real problem. The caller waits
> on a semaphore and the semaphore is only signaled if a message is in the
> queue. This means, the caller can't get a NULL pointer from
> EiDequeueMessagePort(). The real problem is anywhere in
> KeWaitForSingleObject, which does return STATUS_USER_APC for a non
> alertable wait. That is wrong.

There's only one case where KeWaitForSingleObject can return 
STATUS_USER_APC for non-alertable waits and that's when the thread is 
about to be terminated. It is correct behaviour of the routine. I 
believe we're hitting just this case here.

- Filip