Generally speaking, if your function fails, you should return a failure code


A customer requested assistance with their shell namespace extension, and the request worked its way to me for resolution.

Unhandled exception at 0x76fab89c (shell32.dll) in explorer.exe: 0xC0000005:
Access violation reading location 0x00000000.

shell32.dll!CShellItem::_GetPropertyStoreWorker()  + 0x44 bytes
shell32.dll!CShellItem::GetPropertyStoreForKeys()  + 0x38 bytes
thumbcache.dll!CThumbnailCache::_GetMonikerDataFromShellItem()  + 0x8b bytes
thumbcache.dll!CThumbnailCache::GetThumbnail()  + 0x11c bytes
shell32.dll!CSetOperationCallback::_LookupThumbnail()  + 0x8d bytes
shell32.dll!CSetOperationCallback::_PrefetchCachedThumbnails()  + 0xb6 bytes
shell32.dll!CSetOperationCallback::OnNextBatch()  + 0x4f bytes
shell32.dll!CEnumTask::_PushBatchToView()  + 0x68 bytes
shell32.dll!CEnumTask::_IncrFillEnumToView()  + 0x2ca5 bytes
shell32.dll!CEnumTask::_IncrEnumFolder()  + 0x8da5a bytes
shell32.dll!CEnumTask::InternalResumeRT()  + 0xa6 bytes
shell32.dll!CRunnableTask::Run()  + 0x92 bytes
browseui.dll!CShellTask::TT_Run()  + 0x2d bytes
browseui.dll!CShellTaskThread::ThreadProc()  + 0x87 bytes
browseui.dll!CShellTaskThread::s_ThreadProc()  + 0x21 bytes
shlwapi.dll!_ExecuteWorkItemThreadProc@4()  + 0xe bytes
ntdll.dll!_RtlpTpWorkCallback@8()  + 0xaa bytes
ntdll.dll!_TppWorkerThread@4()  + 0x274 bytes
kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes
ntdll.dll!__RtlUserThreadStart@8()  + 0x27 bytes

The customer was at a loss because the customer's code was nowhere on the stack. What is wrong?

The customer didn't provide a dump file or any other information beyond the stack trace. (Hint: When reporting a problem with a shell namespace extension, at least mention the last few method calls your namespace extension received before the crash.) I was forced to use my psychic powers to solve the problem. But you can, too. All the information you need is right there in front of you.

The shell faulted on a null pointer in the function CShellItem::_GetPropertyStoreWorker, which from its name is clearly a worker function which obtains the property store from a shell item.

At this point, you put on your thinking cap. Why is the shell taking a null pointer fault trying to retrieve the property store from a shell item? Remember that the problem is tied to a custom namespace extension.

My psychic powers tell me that the namespace extension returned S_OK from GetUIObjectOf(IPropertyStoreFactory) but set the output pointer to NULL.

(It turns out my psychic powers were weak without coffee, because the initial psychic diagnosis was GetUIObjecttOf(IPropertyStore) instead of IPropertyStoreFactory.)

As a general rule, if your function fails, then you should return a failure code, not a success code. There are exceptions to this rule, particular when OLE automation is involved, but it's a good rule to start with.

The customer reported that fixing their IShellFolder::BindToObject to return an error code when it failed fixed the problem. The customer then followed up with another crash, again providing startling little information.

Unhandled exception at 0x763cf7e7 (shell32.dll) in explorer.exe: 0xC0000005: 
Access violation reading location 0x000a0d70.

Call Stack:

shell32.dll!CInfotipTask::InternalResumeRT() + 0x2e bytes 
shell32.dll!CRunnableTask::Run() + 0x92 bytes 
browseui.dll!CShellTask::TT_Run() + 0x2d bytes 
browseui.dll!CShellTaskThread::ThreadProc() + 0x87 bytes 
browseui.dll!CShellTaskThread::s_ThreadProc() + 0x21 bytes 
shlwapi.dll!_ExecuteWorkItemThreadProc@4() + 0xe bytes 
ntdll.dll!_RtlpTpWorkCallback@8() + 0xaa bytes 
ntdll.dll!_TppWorkerThread@4() + 0x274 bytes 
kernel32.dll!@BaseThreadInitThunk@12() + 0x12 bytes 
ntdll.dll!__RtlUserThreadStart@8() + 0x27 bytes

The customer reported that IQueryInfo::SetInfoTip is getting called. The customer liaison added, "Raymond, I'm looking forward to your psychic powers again."

Apparently, some people don't understand that psychic powers are not something you ask for. It's my way of scolding you for not providing enough information to make a quality diagnosis possible. You don't come back saying, "Hey, thanks for answering my question even though I did a crappy job of asking it. Here's another crappy question!"

I reported back that my psychic powers were growing weary from overuse, and that the customer might want to expend a little more time investigating the problem themselves. Especially since it has the same root cause as their previous problem.

[Raymond is currently away; this message was pre-recorded.]

Comments (22)
  1. Troll says:

    Couldn't you fix the bugs in explorer.exe instead of complaining?

  2. Mike Caron says:

    Yeah, Explorer should know to fill in the customer's pointer before trying to dereference it!

  3. Karellen says:

    Shouldn't this post be tagged "socialskillsofathermonucleardevice"? :-)

  4. Gabe says:

    How could this be a bug in explorer.exe? I don't see it (or any EXE) anywhere in the stack trace.

  5. Bran says:

    "Apparently, some people don't understand that psychic powers are not something you ask for. It's my way of scolding you for not providing enough information to make a quality diagnosis possible. You don't come back saying, "Hey, thanks for answering my question even though I did a crappy job of asking it. Here's another crappy question!""

    Social skills of a thermonuclear device indeed.

  6. Ken Hagan says:

    "Yeah, Explorer should know to fill in the customer's pointer before trying to dereference it!"

    Alternatively, Explorer should test the pointer as well as the HRESULT. I know it shouldn't have too, but surely the shell team know by now that third-party shell extensions are, erm, of variable quality. Indeed, Raymond's psychic powers could probably have predicted this particular class of error within minutes of seeing the prototype for the interface method in question.

  7. Gabe says:

    Ken: Perhaps you are unfamiliar with the "fail fast" philosophy? If you cover up the programmer's errors, the programmer has no way to debug his problem. In this case you are assuming that the programmer meant to return an error code rather than success, hence the bad pointer could be ignored. But what if the programmer really did intend to return success and forgot to set the output pointer properly? Or maybe they set an invalid output pointer. Regardless, either scenario is just as likely.

    Of course if they "fixed Explorer" so that it swallowed exceptions like this, Raymond would just have a different question to post: "How come the output pointer from my function is getting ignored?" — only there would be no stack trace. And then people would start posting things like "You should crash when the programmer returns garbage so that you can debug it"!

  8. John Fringe says:

    Why should Explorer test the pointer? If the application is not working is because it is buggy. And if Explorer has no specific patch for this buggy behavior is because there's not a lot of applications with this same bug (there can not be: they wouldn't work).

    So, after yelling at Raymond for years for patching windows so buggy applications can run (i.e., for caring for its customers), you are know asking him to relax windows rules pre-patching windows so developers can create new buggy applications?

    I think you do not understand the philosophy behind compatibility patches. You relax rules so existing applications can work. You don't relax rules so that new applications can do new bad things.

  9. John says:

    "You relax rules so existing applications can work. You don't relax rules so that new applications can do new bad things."

    But it's a bit of a chicken and egg problem.  You wouldn't need shims if applications were written properly, but if applications were written properly then you wouldn't need shims!  If doing something the wrong way continues to work, then don't complain when people continue to do it the wrong way.

  10. ton says:

    "The customer reported that IQueryInfo::SetInfoTip is getting called. The customer liaison added, "Raymond, I'm looking forward to your psychic powers again."

    I used to wonder why Raymond always seemed to be so grumpy, angry, in a general bad mood and "boasted" of having the social skills of a thermonuclear device. After seeing the idiocy from this customer's inquiries, know I now.

  11. ton says:

    Oops meant "know I now". tee hee…

  12. ton says:

    Meant "now I know". There my brain is finally active.

  13. James Schend says:

    Hey guys, the account name is "troll". You think maybe he's trolling you?

    Stop feeding the trolls. Especially when they flat-out admit they're trolls. Sheesh.

  14. CGomez says:

    Hilarious to watch a few bits of sarcasm in a few comments turn into actual arguing and debate.  Lol

  15. Joshua says:

    never: return ((String)"Could not open file: " + (String)filename).c_str();

  16. Antonio Rodríguez says:

    @Chris Gomez: this is The Old New Thing, the only blog in the world that needs a nitpickers' corner, because we love to read Raymond and then start to talk about a completely unrelated topic (like I'm doing right now ;-) ). No wonder this is the only blog I read *both* the articles and the comments of.

  17. 640k says:

    Instead: return "stack allocated char* holding the following message: could not open file, which this program refuse to give the name of";

  18. MattG says:

    The address 0x000a0d70 is awfully suspicious. If you treat it as four bytes (little-endian), you get 'p','r','n',''. In other words, it looks like a C-style string.

    So: somehow, the developer returned the last part of his string as a pointer? That takes courage.

  19. Cesar says:

    @MattG: my turn trying to imitate Raymond's psychic debugging skills. Since this is an "uninitialized variable" kind of problem, it is probably stack junk, in this case leftover from a C string allocated on the stack.

    Some function at some time used the stack to work with a C string which ended in "prn" (like 'char asdf[] = "….prn";'), and later (perhaps much later) the buggy function was called, and failed to either return an error or clear the returned pointer (I am guessing it was one of these HRESULT-returning functions with an "out" pointer-to-pointer parameter). Since the caller also did not clear the out parameter (if it is an "out" parameter, the caller is not supposed to have to clear it), nobody cleared it and it had stack junk on it – in this case leftover bits from a C string.

    So, it is not the developer returning the last part of a string as a pointer – it is the developer returning "undefined".

  20. Cherry says:

    Correction: It must be:

    void* randomLocation = (rand() + (rand() << 31)) % mbi.RegionSize + mbi.AllocationBase;

  21. Cherry says:

    Looks like my brain is in power-saving mode today. "<< 15" of course (according to the "rand" definition, the highest possible random number is at least 32767).

  22. Cherry says:

    I'd suggest this way (note that I don't know how shell namespace extensions work):

    // ….. some code …..

    SomeThing* outputPointer = (SomeThing*)0xDEADBEEF;

    // Let's call the customer's function and see if they are too dumb for returning a failure code on failure!

    if(TheCustomersFunction(&outputPointer) == S_OK) {

     if(outputPointer == 0xDEADBEEF) {

       // I hate you, guys.

       HMODULE customersLibBase;

       MEMORY_BASIC_INFORMATION mbi;

       GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT | GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, &TheCustomersFunction, &customersLibBase))

       VirtualQuery(customersLibBase + 0x1000, &mbi, sizeof(mbi)); // We assume here that the code section is at RVA 0x1000. In some odd cases, this assumption might be wrong, but further checks (extracting information from section table in PE header) would be too complex for now.

       // You are DOOMED. Good luck with debugging.

       for(int i = 0; i < 100; i++) {

         void* randomLocation = (rand() * rand()) % mbi.RegionSize + mbi.AllocationBase;

         unsigned char randomGarbage = rand() % 0xFF;

         WriteProcessMemory(GetCurrentProcess(), randomLocation, &randomGarbage, 1, NULL); // Using WriteProcessMemory, we circumvent memory page write-protection as well as memory breakpoints (if I remember correctly).

       }

       // Now behave as if the function had returned a failure code and everything was fine… *mad laughter*

     }

    }

    // ….. some code …..

    This should busy such developers for a while. ;-)

Comments are closed.