IoFreeWorkItem frees already freed memory!

I have the simple problem that an IoFreeWorkItem() call tries to free
memory that was already freed.
But: this memory (the WorkerItem) should never have been freed. I must
have screwed up the memory totally terrible and ugly before.
But I don’t know how!

Strange: If I use the same code without the WorkingItem it works
flawless. (But can only execute, of course, if my
procedure gets entered at IRQL = PASSIVE_LEVEL.)

Any idea what’s wrong with this code? :


typedef struct _WORKER_CONTEXT {

// Wished indicator state.
KEYBOARD_INDICATOR_PARAMETERS kbIndicators;

// Pointer to device extension.
PDEVICE_EXTENSION pDevExt;

// Pointer to work item to, free it in called procedure.
PIO_WORKITEM pWorkItem;
} WORKER_CONTEXT, *PWORKER_CONTEXT;

VOID CallBackCalledProcedure(…)
{

// We must use a workerItem. Because sometimes we are at irql
PASSIVE_LEVEL,
// which is ok, and sometimes we are at DISPATCH_LEVEL, which is way
to high!
// And never, never do KeLowerIrql() on an irql which you haven’t raised
// yourself by KeRaiseIrql().
WORKER_CONTEXT workerContext;
PIO_WORKITEM pWorkItem;

USHORT IndicatorParameters;

KeQuerySystemTime(&pGlobalDeviceExtension->SystemTime);

pWorkItem = IoAllocateWorkItem(pGlobalDeviceExtension->Self);
//// We can’t start a waiting process at this IRQL (DISPATCH_LEVEL)
//// So we start a work queue which runs at PASSIVE_LEVEL.
workerContext.kbIndicators.UnitId = 0; // For tests: Switch
LED always on
workerContext.kbIndicators.LedFlags = KEYBOARD_SCROLL_LOCK_ON;
//IndicatorParameters;
workerContext.pDevExt = pGlobalDeviceExtension;
workerContext.pWorkItem = pWorkItem;
IoQueueWorkItem(pWorkItem, &ProcessKbQueueCall,
DelayedWorkQueue, &workerContext);

}

//
// This routine is structured as a work-item routine
//
VOID
ProcessKbQueueCall(
IN PDEVICE_OBJECT Fdo,
IN PWORKER_CONTEXT Context
)
{
PIRP irp;
IO_STATUS_BLOCK ioStatus;
NTSTATUS status;
KEVENT event;

ASSERT(Fdo!=NULL);
ASSERT(Context!=NULL);
if (KeGetCurrentIrql() != PASSIVE_LEVEL)
{
DbgPrint(“Irql: %i\n”, KeGetCurrentIrql());
return;
}

//
// Create notification event object to be used to signal the
// request completion.
//
KeInitializeEvent(&event, NotificationEvent, FALSE);

irp = IoBuildDeviceIoControlRequest(
IOCTL_KEYBOARD_SET_INDICATORS,
Context->pDevExt->TopOfStack,

&Context->kbIndicators, // Input Buffer
sizeof(KEYBOARD_INDICATOR_PARAMETERS), // Input Buffer size
&Context->kbIndicators, // Output Buffer
sizeof(KEYBOARD_INDICATOR_PARAMETERS), // Output Buffer
size

TRUE,
&event, // Receiving event
&ioStatus // Receiving status
);

if (irp)
{
//
// Call the actual driver to perform the operation. If the
returned status
// is PENDING, wait for the request to complete.
//
status = IoCallDriver(Context->pDevExt->TopOfStack, irp);

if (status == STATUS_DEVICE_NOT_READY)
DbgPrint(“Failure: Keyboard wasn’t ready!\n”);
else
DbgPrint(“Status Code: %i”, status);

if (status == STATUS_PENDING)
{
(VOID) KeWaitForSingleObject(
&event,
Executive,
KernelMode,
FALSE,
NULL
);

// –> Exception! irp freed after this call
// Why was this code written in official MS Source-Code?
// Wrong example? Source code bugfixed before brought into
kernel?
//
Another Question:
Why does this fail?
As said: this code works if I call it in the process which creates the
work Item.
Except the following line. It complains that “irp” was already freed.
What?!
It’s official sourceCode I just copied over. I’m confused!
//
//status = irp->IoStatus.Status;
}
else
{
//
// Ensure that the proper status value gets picked up.
//
ioStatus.Status = status;
}
}
else
{
ioStatus.Status = STATUS_INSUFFICIENT_RESOURCES;
}

Here comes the exception. “pWorkItem” was already freed. WTF? By whom?
Windows doesn’t do it.
It’s just removed from the execution queue. And it’s absolut normal to
free the workItem in the procedure
which was called with the item.

IoFreeWorkItem(Context->pWorkItem);
}

> WORKER_CONTEXT workerContext;
A guess: this puppy is on stack, so if you are [un]lucky, it is still alive
when the work item is executed; if you are unlucky, that is,
if CallBackCalledProcedure exits before work item is executed,
it is not.
How about putting workerContext on the heap (and not forgetting to
free it in the work item…)

-------------- Original message --------------
From: Jan Diederich

> I have the simple problem that an IoFreeWorkItem() call tries to free
> memory that was already freed.
> But: this memory (the WorkerItem) should never have been freed. I must
> have screwed up the memory totally terrible and ugly before.
> But I don’t know how!
>
> Strange: If I use the same code without the WorkingItem it works
> flawless. (But can only execute, of course, if my
> procedure gets entered at IRQL = PASSIVE_LEVEL.)
>
> Any idea what’s wrong with this code? :
>
> -------------
> typedef struct _WORKER_CONTEXT {
>
> // Wished indicator state.
> KEYBOARD_INDICATOR_PARAMETERS kbIndicators;
>
> // Pointer to device extension.
> PDEVICE_EXTENSION pDevExt;
>
> // Pointer to work item to, free it in called procedure.
> PIO_WORKITEM pWorkItem;
> } WORKER_CONTEXT, *PWORKER_CONTEXT;
>
>
> VOID CallBackCalledProcedure(…)
> {
> …
> // We must use a workerItem. Because sometimes we are at irql
> PASSIVE_LEVEL,
> // which is ok, and sometimes we are at DISPATCH_LEVEL, which is way
> to high!
> // And never, never do KeLowerIrql() on an irql which you haven’t raised
> // yourself by KeRaiseIrql().
> WORKER_CONTEXT workerContext;
> PIO_WORKITEM pWorkItem;
>
> USHORT IndicatorParameters;
> …
> KeQuerySystemTime(&pGlobalDeviceExtension->SystemTime);
>
> pWorkItem = IoAllocateWorkItem(pGlobalDeviceExtension->Self);
> //// We can’t start a waiting process at this IRQL (DISPATCH_LEVEL)
> //// So we start a work queue which runs at PASSIVE_LEVEL.
> workerContext.kbIndicators.UnitId = 0; // For tests: Switch
> LED always on
> workerContext.kbIndicators.LedFlags = KEYBOARD_SCROLL_LOCK_ON;
> //IndicatorParameters;
> workerContext.pDevExt = pGlobalDeviceExtension;
> workerContext.pWorkItem = pWorkItem;
> IoQueueWorkItem(pWorkItem, &ProcessKbQueueCall,
> DelayedWorkQueue, &workerContext);
> …
> }
>
> //
> // This routine is structured as a work-item routine
> //
> VOID
> ProcessKbQueueCall(
> IN PDEVICE_OBJECT Fdo,
> IN PWORKER_CONTEXT Context
> )
> {
> PIRP irp;
> IO_STATUS_BLOCK ioStatus;
> NTSTATUS status;
> KEVENT event;
>
> ASSERT(Fdo!=NULL);
> ASSERT(Context!=NULL);
> if (KeGetCurrentIrql() != PASSIVE_LEVEL)
> {
> DbgPrint(“Irql: %i\n”, KeGetCurrentIrql());
> return;
> }
>
> //
> // Create notification event object to be used to signal the
> // request completion.
> //
> KeInitializeEvent(&event, NotificationEvent, FALSE);
>
> irp = IoBuildDeviceIoControlRequest(
> IOCTL_KEYBOARD_SET_INDICATORS,
> Context->pDevExt->TopOfStack,
>
> &Context->kbIndicators, // Input Buffer
> sizeof(KEYBOARD_INDICATOR_PARAMETERS), // Input Buffer size
> &Context->kbIndicators, // Output Buffer
> sizeof(KEYBOARD_INDICATOR_PARAMETERS), // Output Buffer
> size
>
> TRUE,
> &event, // Receiving event
> &ioStatus // Receiving status
> );
>
> if (irp)
> {
> //
> // Call the actual driver to perform the operation. If the
> returned status
> // is PENDING, wait for the request to complete.
> //
> status = IoCallDriver(Context->pDevExt->TopOfStack, irp);
>
> if (status == STATUS_DEVICE_NOT_READY)
> DbgPrint(“Failure: Keyboard wasn’t ready!\n”);
> else
> DbgPrint(“Status Code: %i”, status);
>
> if (status == STATUS_PENDING)
> {
> (VOID) KeWaitForSingleObject(
> &event,
> Executive,
> KernelMode,
> FALSE,
> NULL
> );
>
> // –> Exception! irp freed after this call
> // Why was this code written in official MS Source-Code?
> // Wrong example? Source code bugfixed before brought into
> kernel?
> //
> Another Question:
> Why does this fail?
> As said: this code works if I call it in the process which creates the
> work Item.
> Except the following line. It complains that “irp” was already freed.
> What?!
> It’s official sourceCode I just copied over. I’m confused!
> //
> //status = irp->IoStatus.Status;
> }
> else
> {
> //
> // Ensure that the proper status value gets picked up.
> //
> ioStatus.Status = status;
> }
> }
> else
> {
> ioStatus.Status = STATUS_INSUFFICIENT_RESOURCES;
> }
>
> Here comes the exception. “pWorkItem” was already freed. WTF? By whom?
> Windows doesn’t do it.
> It’s just removed from the execution queue. And it’s absolut normal to
> free the workItem in the procedure
> which was called with the item.
>
> IoFreeWorkItem(Context->pWorkItem);
> }
> -------------
>
> —
> Questions? First check the Kernel Driver FAQ at
> http://www.osronline.com/article.cfm?id=256
>
> To unsubscribe, visit the List Server section of OSR Online at
> http://www.osronline.com/page.cfm?name=ListServer

Ahh, damnit!
Thanks!
I already thought about that. But: I saw many other procedures using
local variables on local procedures stacks for similar things. Similar
to this code, not identical.
A queued work item is definitely another story.
Not it works like a charm. Thanks.

xxxxx@comcast.net schrieb:

> WORKER_CONTEXT workerContext;
A guess: this puppy is on stack, so if you are [un]lucky, it is still alive
when the work item is executed; if you are unlucky, that is,
if CallBackCalledProcedure exits before work item is executed,
it is not.
How about putting workerContext on the heap (and not forgetting to
free it in the work item…)

-------------- Original message --------------
From: Jan Diederich
>
> > I have the simple problem that an IoFreeWorkItem() call tries to
> free
> > memory that was already freed.
> > But: this memory (the WorkerItem) should never have been freed. I
> must
> > have screwed up the memory totally terrible and ugly before.
> > But I don’t know how!
> >
> > Strange: If I use the same code without the WorkingItem it works
> > flawless. (But can only execute, of course, if my
> > procedure gets entered at IRQL = PASSIVE_LEVEL.)
> >
> > Any idea what’s wrong with this code? :
> >
> > -------------
> > typedef struct _WORKER_CONTEXT {
> >
> > // Wished indicator state.
> > KEYBOARD_INDICATOR_PARAMETERS kbIndicators;
> >
> > // Pointer to device extension.
> > PDEVICE_EXTEN SION pDevExt;
> >
> > // Pointer to work item to, free it in called procedure.
> > PIO_WORKITEM pWorkItem;
> > } WORKER_CONTEXT, *PWORKER_CONTEXT;
> >
> >
> > VOID CallBackCalledProcedure(…)
> > {
> > …
> > // We must use a workerItem. Because sometimes we are at irql
> > PASSIVE_LEVEL,
> > // which is ok, and sometimes we are at DISPATCH_LEVEL, which is way
> > to high!
> > // And never, never do KeLowerIrql() on an irql which you haven’t
> raised
> > // yourself by KeRaiseIrql().
> > WORKER_CONTEXT workerContext;
> > PIO_WORKITEM pWorkItem;
> >
> > USHORT IndicatorParameters;
> > …
> > KeQuerySystemTime(&pGlobalDeviceExtension->SystemTime);
> >
> > pWorkItem = IoAllocateWorkItem(pGlobalDeviceExtension->Self);
> > //// We can’t start a waiting process at this IRQL (DISPATCH_LEVEL)
> > //// So we start a work queue which runs at PASSIVE_LEVEL. < BR>>
> workerContext.kbIndicators.UnitId = 0; // For tests: Switch
> > LED always on
> > workerContext.kbIndicators.LedFlags = KEYBOARD_SCROLL_LOCK_ON;
> > //IndicatorParameters;
> > workerContext.pDevExt = pGlobalDeviceExtension;
> > workerContext.pWorkItem = pWorkItem;
> > IoQueueWorkItem(pWorkItem, &ProcessKbQueueCall,
> > DelayedWorkQueue, &workerContext);
> > …
> > }
> >
> > //
> > // This routine is structured as a work-item routine
> > //
> > VOID
> > ProcessKbQueueCall(
> > IN PDEVICE_OBJECT Fdo,
> > IN PWORKER_CONTEXT Context
> > )
> > {
> > PIRP irp;
> > IO_STATUS_BLOCK ioStatus;
> > NTSTATUS status;
> > KEVENT event;
> >
> > ASSERT(Fdo!=NULL);
> > ASSERT(Context!=NULL);
> > if (KeGetCurrentIrql() != PASSIVE_LEVEL)
> > {
> > DbgPrint(“Irql: %i\n”, KeGetCurrentIrql());
> > return;
> > }
> >
> > // *> // Create notification event object to be used to signal the
> > // request completion.
> > //
> > KeInitializeEvent(&event, NotificationEvent, FALSE);
> >
> > irp = IoBuildDeviceIoControlRequest(
> > IOCTL_KEYBOARD_SET_INDICATORS,
> > Context->pDevExt->TopOfStack,
> >
> > &Context->kbIndicators, // Input Buffer
> > sizeof(KEYBOARD_INDICATOR_PARAMETERS), // Input Buffer size
> > &Context->kbIndicators, // Output Buffer
> > sizeof(KEYBOARD_INDICATOR_PARAMETERS), // Output Buffer
> > size
> >
> > TRUE,
> > &event, // Receiving event
> > &ioStatus // Receiving status
> > );
> >
> > if (irp)
> > {
> > //
> > // Call the actual driver to perform the operation. If the
> > returned status
> > // is PENDING, wait for the request to complete.
> > //
> > status = IoCallDriver(Context->pDevExt->TopOfStack, irp);
> >
> > if (status == STATUS_DEVICE_NOT_READY)
> > DbgPrint(“Failure: Keyboard wasn’t ready!\n”);
> > else
> > DbgPrint(“Status Code: %i”, status);
> >
> > if (status == STATUS_PENDING)
> > {
> > (VOID) KeWaitForSingleObject(
> > &event,
> > Executive,
> > KernelMode,
> > FALSE,
> > NULL
> > );
> >
> > // –> Exception! irp freed after this call
> > // Why was this code written in official MS Source-Code?
> > // Wrong example? Source code bugfixed before brought into
> > kernel?
> > //
> > Another Question:
> > Why does this fail?
> > As said: this code works if I call it in the process which
> creates the
> > work Item.
> > Except the following line. It complains that “irp” was already
> freed.
> > What?!
> > It’s official sourceCode I just copied over. I’m confused!
> > //
> > //status = irp->IoStatus.Status;
> > }
> > else
> > {
> > //
> > // Ensure that the proper status value gets picked up.
> > //
> > ioStatus.Status = status;
> > }
> > }
> > else
> > {
> > ioStatus.Status = STATUS_INSUFFICIENT_RESOURCES;
> > }
> >
> > Here comes the exception. “pWorkItem” was already freed. WTF? By
> whom?
> > Windows doesn’t do it.
> > It’s just removed from the execution queue. And it’s absolut
> normal to
> > free the workItem in the procedure
> > which was called with the item.
> >
> > IoFreeWorkItem(Context->pWorkItem);
> > }
> > -------------
> >
> > —
> > Questions? First check the Kernel Driver FAQ at
> > http://www.osronline.com/article.cfm?id=256
> >
> > To unsubscribe, visit the List Server section of OSR Online at
> > http://www.osronline.com/page.cfm?name=ListServer *

BTW, it is not a very good idea to wait in workitem routine, especially the way you do, i.e. with infinite timeout - workitem routine runs in context of a system thread, and the number of these threads is limited. Probably, it makes sense to create your own dedicated system thread if you intend to wait…

Anton Bassov