> Hi !
I have tried to implement the inverted call and it’s not working so well…
I’m not using WDF but with the article i think I have understand the way
inverted calls work (or maybe not…)
here is how I implemented it :
I have a linked list of pending Irp with functions to push and pop
pendings IRPs :
// add an irp to the list
NTSTATUS pushIrp(PIRP Irp)
{
PPENDING_IRP_QUEUE new_entry;
new_entry = (PPENDING_IRP_QUEUE)ExAllocatePoolWithTag(NonPagedPool,
sizeof(PENDING_IRP_QUEUE), ‘tesT’);
if(new_entry == NULL)
return -1;
-1 is not a defined NTSTATUS value. Anything you return will be a
symbolic name defined in nterr.h, usually beginning as STATUS_ . In this
case, STATUS_INSUFFICIENT_RESOURCES. You can’t just whimsically decide
that “-1” is a sensible value; it is not.
new_entry->irp = Irp;
new_entry->flink = NULL;
And where is the spin lock that protects the queue? Why are you not using
ExInterlockedInsertTailList and ExInterlockedRemoveHeadList? You have
gone to a great deal of effort to create an incorrect and unnecessarily
complex solution to a problem best handled by existing mechanisms. If you
are using KMDF, use the supported queue models. The first thing to do is
throw away this code and replace it with something far, far simpler. If
you are not using KMDF, the field you want is Irp->Tail.Overlay.ListEntry.
Also make sure you understand the CONTAINING_RECORD macro.
currentIrp->flink = new_entry;
currentIrp = new_entry;
There is already a field in the IRP, Tail.Overlay.ListEntry, which you are
permitted to use for purposes of maintaining your own queue, and
consequently there is no need to allocate some kind of queue-structure of
your own. Also, linked lists are two-way, and therefore flink cannot be
NULL if you use a LIST_ENTRY structure such as the one already in the IRP.
return STATUS_SUCCESS;
}
// returns first IRP of the list
PIRP popIrp()
{
PPENDING_IRP_QUEUE entry_to_remove, next_entry;
PIRP ret_irp;
And where is the spin lock that protects this queue? You must be very
fond of the color blue, because you pretty much guarantee you will be
seeing a lot of it.
entry_to_remove = pending_irp_queue->flink;
// if queue is empty
if(entry_to_remove == NULL)
return NULL;
next_entry = entry_to_remove->flink;
// if next_entry doesn’t exist
if(next_entry == NULL)
{
DbgPrint(“next_entry == NULL\n”);
ret_irp = entry_to_remove->irp;
pending_irp_queue->flink = NULL;
ExFreePool(entry_to_remove);
currentIrp = pending_irp_queue;
return ret_irp;
Note that if you were to keep this horrid structure, the correct approach
is to lock the data structure, do your thing, unlock it, and only THEN
call the potentially time-consuming ExFreePool. But since you don’t need
to allocate or free anything, because Tail.Overlay.ListEntry or KMDF
queues are already there for you, you can ignore this performance
enhancement.
Note also that you have not indicated how you handle cancellation of the
entries in this queue, which will be critical. KMDF should handle this
for you if you use a KMDF queue. Or at least that is my understanding.
You would actually have to move to cancel-safe queues if you are not using
KMDF.
}
else
{
DbgPrint(“next_entry exist\n”);
ret_irp = entry_to_remove->irp;
pending_irp_queue->flink = next_entry;
ExFreePool(entry_to_remove);
currentIrp = next_entry;
return ret_irp;
}
Note that potentially expensive operations such as DbgPrint need to be
done either before the lock is acquired or after the lock is released
(depending on when the information being printed is valid). Do not use
DbgPrint; use KdPrint. KdPrints disappear from the release build,
DbgPrints exist forever.
}
when I receive an ioctl from the userland, I just add it to my linked
list
that way :
case IOCTL_LOG:
DbgPrint(“IOCTL_LOG received !\n”);
if(pIoStackIrp->Parameters.DeviceIoControl.OutputBufferLength >=
bufSize)
{
// queue the IRP
if(NT_SUCCESS(pushIrp(Irp)))
DbgPrint(“pending Irp added to the list\n”);
// marks the irp as pending
You can’t do this. As soon as you do pushIrp, you have lost control of
the IRP, and therefore the address is immediately meaningless. There is
no possible way you can validly use this address after pushIrp.
IoMarkIrpPending(Irp);
}
break;
when I have logs to send, I pop an IRP from the linked list and fill its
buffer with the logs and complete it :
I just don’t get why you are using a stack instead of a queue.
// get IRP from the queue
irp_to_complete = popIrp();
if(irp_to_complete == NULL)
{
DbgPrint(“Failed to retrieve IRP\n”);
return STATUS_FAILURE;
The ultimate cop-out. This is a bit like having a compiler that prints,
at the end of your compilation, “Your input file contained syntax errors.
Fix them and try again” as the entire error output. STATUS_FAILURE makes
for a tech support nightmare.
}
else
DbgPrint(“IRP correctly retrieved\n”);
// fill the buffer
outputBuffer = (PCHAR)irp_to_complete->AssociatedIrp.SystemBuffer;
RtlCopyMemory(outputBuffer, buf, sizeBuf);
And where do you check to make sure that sizebuf is less than or equal to
the actual buffer length in the IRP? What if it isn’t? Will you properly
NUL-terminate the string? What will the app do if you truncate the string
and don’t put a terminating NUL character at the end? Why are you
popping IRPs instead of dequeuing the oldest? Or is a terminal NUL part
of the specified interface? If it isn’t, it becomes the application’s
problem to deal with it. But you have to be very clear on the behavior.
This code is deadly, and cannot be trusted. A long string copied to a
short buffer will soon turn your screen blue.
DbgPrint(“to send : %s\n”, outputBuffer);
// complete the irp
irp_to_complete->IoStatus.Information = sizeBuf;
Does this include a terminal NUL, or is a terminal NUL part of the
specification? Note that if the buffer is too small, you should determine
this before dequeing the message from its queue, and the incoming IRP
should be completed with something like STATUS_BUFFER_TOO_SMALL (there are
two such error codes, which have slightly different meanings, and you need
to choose the correct one)
irp_to_complete->IoStatus.Status = STATUS_SUCCESS;
IoCompleteRequest(irp_to_complete, IO_NO_INCREMENT);
from my user app, I have implemented the same way than the exemple from
the last issue of NT Insider with few changes to fit with my needs :
hDevice = CreateFile(“\\.\myDriver”, GENERIC_READ|GENERIC_WRITE,
0, NULL, OPEN_EXISTING, FILE_FLAG_OVERLAPPED, NULL);
if(hDevice == INVALID_HANDLE_VALUE)
{
fprintf(stderr, “Cannot access device driver : %x\n”, GetLastError());
exit(0);
Note: program vertically. It makes no sense to put the exit(0) on the
same line as the fprintf. AND it is a lousy idea to ever put an exit()
call or any of its numerous synonyms into most apps. Use of this is like
strcpy, it should be declared a felony.
}
// creates an I/O completion port
hCompletionPort = CreateIoCompletionPort(hDevice, NULL, 0, 0);
if(!hCompletionPort)
{
fprintf(stderr, “Cannot create IO Completion Port : %x\n”,
GetLastError());
It is best practice to always make this the first line of a failure branch:
DWORD err = GetLastError();
there are any number of lines you could put in front of the fprintf which
will guarantee that GetLastError will be invalid, and so the FIRST thing
you do, before ANYTHING else, is retrieve it and save it in a safe place.
exit(0);
}
// creates a thread
hThread = CreateThread(NULL, 0, CompletionPortThread, hCompletionPort,
0,
&threadId);
CreateThread is a VERY, VERY, VERY bad idea. Since the thread uses the
CRT, you MUST create the thread with _beginthread or _beginthreadex (the
latter is my preference). You must also close the thread handle when you
are done with it. Note carefully the specifications of the function
prototypes for _beginthread and _beginthreadex, which are /not/ WINAPI.
Also note that if the symbol _beginthread[ex] is undefined, it is a
miracle if this code ever worked, because it means you are running with
the single-threaded CRT, which works rather poorly in multithreaded
environments. Let’s say “rather poorly” translates to “bizarre crashes
caused by strange behavior which is timing-sensitive”, but if you just use
CreateThread, and your thread uses the CRT, there is no guarantee that the
critical CRT data structures are properly initialized.
Generally, you need the thread handle, so you can wait for the thread to
complete after you shut it down, so the main thread blocks until the
subthread completes
if(!hThread)
{
fprintf(stderr, “Cannot create thread : %x\n”, GetLastError());
exit(0);
}
while(1)
{
memset(&msg, 0, sizeof(KERNEL_MESSAGE));
Why are you zeroing this out? You’re going to overwrite it. This
zeroing-out can actually mask serious bugs; for example, that a string
which is exactly as long as the buffer will not be NUL-terminated, so
you’ve left a landmine for someone to step on.
Note that every DeviceIoControl you send must have a UNIQUE
PKERNEL_MESSAGE structure; every address must be different. Therefore,
msg cannot be allocated on the stack (generally, this won’t work anyway
because, except in this very restricted case, the function could exit
before the KERNEL_MESSAGE is used, rendering the address nonsensical. You
should allocate a new KERNEL_MESSAGE object for each call to
DeviceIoControl, and free it in the dequeue handler.
The OVERLAPPED structure is effectively the ID of the transaction, and you
cannot have two pending transactions with the same ID, or you will get
rubbish. Imagine, for example, you dequeue an IRP, and before you can see
its buffer contents, they are overwritten by the NEXT IRP in the queue.
One I/O request, one unique OVERLAPPED structure. Not negotiable; if you
have the same address for two pending I/O operations, your behavior will
be undefined (and probably wrong. No, it won’t bluescreen, but you’ll
just see improper results, and not be able to figure out why it works some
times and not others).
// send IOCTL to grab log
DeviceIoControl(hDevice, IOCTL_LOG, NULL, 0, msg.message,
sizeof(msg.message), NULL, &msg.overlapped);
It is hard to tell the validity of this code, since we do not see the msg
structure. Note also that the buffer length you pass in is for the entire
msg structure, but in the driver, you blindly copy the message into the
buffer referenced, not just into the msg component, and you do so without
checking that the msg component is long enough to hold the string. This
means that your app can be getting the wrong values, in the brief window
before the BSOD pops up because to clobber the user structures, you have
to clobber critical kernel structures, but in a free build I’ve seen the
system run for as long as five minutes of interactive usage before the
BSOD happens.
Note that when you show code examples. EVERY variable and its declaration
site must be shown, and for user-defined structures, like KERNEL_MESSAGE,
you must show the structure so we can make sense of it. The above call is
almost certainly wrong, but I can’t say why without seeing the structure.
Note that for proprietary data structures, you only need show the parts
that are relevant to the discussion, and can elide anything remotely
sensitive that might hint at current or future product offerings. But you
must show them, e.g.,
typedef struct {
OVERLAPPED ovl;
//…lots of proprietary stuff here
LIST_ENTRY msgqueue;
} KERNEL_MESSAGE, *PKERNEL_MESSAGE;
or whatever your struct looks like.
Sleep(100);
}
Even though the indentation is wrong, I suspect that the above } closes
off the “while(1)” loop (use while(TRUE)). At this point, you would use
PostQueuedCompletionStatus to send a command to the thread to kill itself
(see later comments), then wait on the handle until the thread completes.
DWORD WINAPI CompletionPortThread(LPVOID PortHandle)
{
PKERNEL_MESSAGE msg = NULL;
ULONG_PTR compKey = 0;
OVERLAPPED *overlapped = NULL;
DWORD bytesTransferred = 0;
No reason to initialize any of these variables, since they will all be
overwritten when the message is received.
DWORD i;
while(1)
{
overlapped = NULL;
// wait for IRP completion
GetQueuedCompletionStatus(PortHandle, &bytesTransferred, &compKey,
&overlapped, INFINITE);
How did this compile at all? GetQueuedCompletionStatus wants a HANDLE
value, and you have given it a PVOID. My own preference is to write the
declarations
…CompletionPortThread(LPVOID p)
{
HANDLE PortHandle = (HANDLE)p;
And you check the return value of GetQueuedCompletionStatus where? And
use GetLastError to report what happened, where? How is it that you do
something this complex and feel you can just blaze on, pretending it
worked and that the variables now have meaning?
Also, you have no way to determine if the loop should exit, which means
you have no good way to shut this off. I typically use the compKey to
determine if the message is a queue value to process (compKey == 0) or
some metacommand (compKey != 0) and depending on how many metacommands, I
may further use a switch statement to decode the metacommands, but most
commonly it is just one case, so I can write it as an ‘if’:
if(compKey == MY_COMMAND_QUIT_THREAD)
break;
if(bytesTransferred == 0)
continue;
if(overlapped == NULL)
continue;
msg = (PKERNEL_MESSAGE)overlapped;
// print the received log
printf(“msg : %s\n”, msg->message);
And where do you free the msg structure? Or are you just going to leak
these like crazy.
}
Could you tell me what is wrong with my implementation? If you any
advise,
i’ll be really gratefull !
Well, I pretty much have. Lots and lots and lots of problems. None
insurmountable, or requiring a total rewrite, so you have
sort-of-the-right-idea, but at the detail level, both your driver and your
app have fatal flaws. Mostly you just have to add code to guarantee
correctness. (Unlike some examples I’ve seen, where my response is “There
is nothing wrong with this code that cannot be fixed by a complete rewrite
to a sensible design”. To repeat a tag line used elsewhere in this NG:
trust, but verify. There is no conceivable way that failing to check the
buffer length in the driver, for example, can be justified. So you have
to check EVERYTHING. Make a list, check it twice, and if something looks
naughty, make it nice. (Or, don’t make a list; use the existing KMDF
queues or use a real queue, but I digress…).
Your code is far too trusting, and that is potentially fatal.
joe
Thanks guys !
NTDEV is sponsored by OSR
Visit the list at: http://www.osronline.com/showlists.cfm?list=ntdev
OSR is HIRING!! See http://www.osr.com/careers
For our schedule of WDF, WDM, debugging and other seminars visit:
http://www.osr.com/seminars
To unsubscribe, visit the List Server section of OSR Online at
http://www.osronline.com/page.cfm?name=ListServer