214173 - Crash in typeFromExtEquals

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

missing null check in typeFromExtEquals:
strcmp() line 77
typeFromExtEquals(const char * 0x0012f73c, const char * 0x0012f964) line 319 +
13 bytes
nsOSHelperAppService::GetMIMEInfoFromOS(const char * 0x0012f964, const char *
0x0012f73c) line 441 + 29 bytes
nsExternalHelperAppService::GetFromTypeAndExtension(nsExternalHelperAppService *
const 0x00fa5324, const char * 0x0012f964, const char * 0x0012f73c, nsIMIMEInfo
* * 0x0012f724) line 2103 + 27 bytes
nsExternalHelperAppService::DoContent(nsExternalHelperAppService * const
0x00fa5318, const char * 0x0012f964, nsIRequest * 0x03e4cff8, nsISupports *
0x03b847c8, nsIStreamListener * * 0x0012f92c) line 337 + 57 bytes
nsDocumentOpenInfo::DispatchContent(nsIRequest * 0x03e4cff8, nsISupports *
0x00000000) line 437 + 97 bytes
nsDocumentOpenInfo::OnStartRequest(nsDocumentOpenInfo * const 0x03c9a9a0,
nsIRequest * 0x03e4cff8, nsISupports * 0x00000000) line 227 + 16 bytes
nsHttpChannel::CallOnStartRequest() line 620 + 60 bytes
nsHttpChannel::ProcessNormal() line 747 + 8 bytes
nsHttpChannel::ProcessResponse() line 647 + 8 bytes
nsHttpChannel::OnStartRequest(nsHttpChannel * const 0x03e4d000, nsIRequest *
0x03c885a0, nsISupports * 0x00000000) line 3169 + 11 bytes
nsInputStreamPump::OnStateStart() line 362 + 42 bytes
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x03c885a4,
nsIAsyncInputStream * 0x03e03f7c) line 318 + 11 bytes
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x03e94b54) line 117
PL_HandleEvent(PLEvent * 0x03e94b54) line 671 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00ed8348) line 606 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x00030144, unsigned int 0x0000c0d9, unsigned int
0x00000000, long 0x00ed8348) line 1412 + 9 bytes
USER32! GetSysColor + 1092 bytes
USER32! GetSysColor + 1377 bytes
USER32! DispatchMessageW + 11 bytes
nsAppShellService::Run(nsAppShellService * const 0x0158c800) line 478
main1(int 0x00000003, char * * 0x00262638, nsISupports * 0x00f5add0) line 1290 +
32 bytes
main(int 0x00000003, char * * 0x00262638) line 1669 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! DosDateTimeToFileTime + 3172 bytes

     LPBYTE pBytes = GetValueBytes( hKey, "Content Type");
     PRBool eq = strcmp((const char *)pBytes, aType) == 0; <-- this crashes if
pBytes is null

might be related to bug 213985

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 128700 [details] [diff] [review]
patch

this adds a missing null-check

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 214172 has been marked as a duplicate of this bug. ***

Comment 4

[Darin Fisher]

Comment on attachment 128700 [details] [diff] [review]
patch

>Index: nsOSHelperAppService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.cpp,v
>retrieving revision 1.39
>diff -u -8 -p -r1.39 nsOSHelperAppService.cpp
>--- nsOSHelperAppService.cpp	25 Jul 2003 16:47:55 -0000	1.39
>+++ nsOSHelperAppService.cpp	28 Jul 2003 14:53:37 -0000
>@@ -311,16 +311,18 @@ static PRBool typeFromExtEquals(const ch
> 
>   fileExtToUse.Append(aExt);
> 
>   HKEY hKey;
>   LONG err = ::RegOpenKeyEx( HKEY_CLASSES_ROOT, fileExtToUse.get(), 0, KEY_QUERY_VALUE, &hKey);
>   if (err == ERROR_SUCCESS)
>   {
>      LPBYTE pBytes = GetValueBytes( hKey, "Content Type");
>+     if (!pBytes)
>+       return PR_FALSE;
>      PRBool eq = strcmp((const char *)pBytes, aType) == 0;
>      delete[] pBytes;
>      return eq;
>   }
>   return PR_FALSE;
> }

isn't this function leaking hKey?   doesn't it need to call RegCloseKey?

  PRBool eq = PR_FALSE;
  LONG err = ::RegOpenKeyEx( HKEY_CLASSES_ROOT, fileExtToUse.get(), 0,
KEY_QUERY_VALUE, &hKey);
  if (err == ERROR_SUCCESS)
  {
     LPBYTE pBytes = GetValueBytes( hKey, "Content Type");
     if (pBytes)
     {
	eq = strcmp((const char *)pBytes, aType) == 0;
	delete[] pBytes;
     }
  }
  ::RegCloseKey(hKey);
  return PR_FALSE;

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 128700 [details] [diff] [review]
patch

hm, indeed
going to make a new patch

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 128707 [details] [diff] [review]
patch v2

You don't want to close a key that never got opened.  In any case, the key
issue is handled by the patch in bug 213985 -- either merge in that patch in
full or leave it out completely or something.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 128707 [details] [diff] [review]
patch v2

ok then. leaving the key closing to the other bug

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 128711 [details] [diff] [review]
patch

>Index: win/nsOSHelperAppService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.cpp,v
>retrieving revision 1.39
>diff -u -8 -p -r1.39 nsOSHelperAppService.cpp
>--- win/nsOSHelperAppService.cpp	25 Jul 2003 16:47:55 -0000	1.39
>+++ win/nsOSHelperAppService.cpp	28 Jul 2003 16:10:54 -0000
>@@ -307,25 +307,27 @@ static PRBool typeFromExtEquals(const ch
>     return PR_FALSE;
>   nsCAutoString fileExtToUse;
>   if (aExt[0] != '.')
>     fileExtToUse = '.';
> 
>   fileExtToUse.Append(aExt);
> 
>   HKEY hKey;
>+  PRBool eq = PR_FALSE;
>   LONG err = ::RegOpenKeyEx( HKEY_CLASSES_ROOT, fileExtToUse.get(), 0, KEY_QUERY_VALUE, &hKey);
>   if (err == ERROR_SUCCESS)
>   {
>      LPBYTE pBytes = GetValueBytes( hKey, "Content Type");
>-     PRBool eq = strcmp((const char *)pBytes, aType) == 0;
>-     delete[] pBytes;
>-     return eq;
>+     if (pBytes) {
>+       eq = strcmp((const char *)pBytes, aType) == 0;
>+       delete[] pBytes;
>+     }
>   }
>-  return PR_FALSE;
>+  return eq;
> }
> 
> already_AddRefed<nsIMIMEInfo> nsOSHelperAppService::GetByExtension(const char *aFileExt, const char *aTypeHint)
> {
>   if (!aFileExt || !*aFileExt)
>     return nsnull;
> 
>   // windows registry assumes your file extension is going to include the '.'.

Comment 11

[Darin Fisher]

Comment on attachment 128711 [details] [diff] [review]
patch

sr=darin (writing code snipets before the morning coffee is never a good
idea... sorry for the snafus :-/)

nit: keep brace style consistent.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in, with the brace style fixed

Checking in nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.cpp,v  <-- 
nsOSHelperAppService.cpp
new revision: 1.40; previous revision: 1.39
done