194439 - nsOSHelperAppService on windows destroys non-latin1 default description

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

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

(see above url as well as line 335)

When I give a file (say a winzip file) a non-latin1 description (say, korean
characters from http://www.joins.com), mozilla just displays these characters as
?. Tested with 2003021408, but latest trunk still has the same code.

best thing to do is, I think, use a W version of RegQueryValueEx near line 165.
(Though, I don't know how that interacts with win 9x)

Comment 1

[Boris Zbarsky [:bzbarsky]]

ccing some people who may have a clue about the Win9x unicode issues.

Comment 2

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

oh, and as for "destroy" - the actual registry entry is of course not destroyed.
it only gets displayed incorrectly by mozilla.

Comment 3

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

taking...

Comment 4

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

Attachment: patch

use RegQueryValueExW on nt-based windows versions where that's supported (which
returns a UTF-16 string), and the ascii version otherwise

Comment 5

[Ere Maijala (slow)]

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

I suggest doing an early return from GetMIMEInfoFromRegistry() if rc !=
ERROR_SUCCESS. Anyway, r=ere

Comment 6

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

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

ok, I'll make that change

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

>Index: win/nsOSHelperAppService.cpp
>+  LONG err = ::RegQueryValueExW( hKey, pValueName, NULL, NULL, NULL, &bufSz); 
>+  if (err == ERROR_SUCCESS) {
>+    PRUnichar* pBytes = new PRUnichar[bufSz];
...
>+    if (err != ERROR_SUCCESS) {
>+      delete [] pBytes;
>+      return PR_FALSE;
>+    } else {
>+      result.Assign(pBytes);
>+      return PR_TRUE;
>+    }

That leaks pBytes in the non-error case, no?  You need to delete it after you
do that assignment...

sr=bzbarsky with that change.

Comment 8

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

thanks for spotting that, bz - I realized that GetValueBytes has the same leak
so I fixed it there too.

Checking in win/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.cpp,v  <-- 
nsOSHelperAppService.cpp
new revision: 1.45; previous revision: 1.44
done
Checking in win/nsOSHelperAppService.h;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.h,v  <-- 
nsOSHelperAppService.h
new revision: 1.14; previous revision: 1.13
done

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I realized that GetValueBytes has the same leak so I fixed it there too.

Except that's not a leak, since that function _returns_ the pBytes and callers
clean it up after looking at it!  With the change you made I expect us to be
crashing a lot as callers access deleted memory...

The difference between GetValueBytes and the GetValueString you added is that
GetValueString _copies_ the bytes into the string, so should clean them up
itself while GetValueBytes returns the raw bytes (and should probably document
this, btw).

As far as that goes, your fix for GetValueString is not cool:

222                     err = ::RegQueryValueExW( hKey, pValueName, NULL, NULL,
(BYTE*)pBytes, &bufSz);
223 cbiesinger 1.45     delete [] pBytes;
224                     if (err != ERROR_SUCCESS) {
225                       return PR_FALSE;
226                     } else {
227                       result.Assign(pBytes);
228                       return PR_TRUE;
229                     }

You delete pBytes, then you pass it to Assign()?  Crash waiting to happen... 
NOte I said you need to delete _AFTER_ you do the assignment.

Reopening to make sure this stays on your radar...

Comment 10

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

sigh. I'm too tired today :(
thanks for looking at what I did, sorry about these mistakes...
I fixed this now.

Comment 11

[Boris Zbarsky [:bzbarsky]]

As far as that goes... Looking at GetExtensionFromWindowsMimeDatabase, shouldn't
that use "operator delete[]" instead of "operator delete" on pBytes?  Not
related to this patch, but I decided to be suspicious of this whole file... ;)

Comment 12

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

hm, yeah, looks like it should. should I just check in that change with
r+sr=bzbarsky?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Sure.

Comment 14

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

ok, done