nsOSHelperAppService on windows destroys non-latin1 default description

RESOLVED FIXED in mozilla1.6beta

Status

Core Graveyard
File Handling
RESOLVED FIXED
16 years ago
2 years ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

Trunk
mozilla1.6beta
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(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)
ccing some people who may have a clue about the Win9x unicode issues.
oh, and as for "destroy" - the actual registry entry is of course not destroyed.
it only gets displayed incorrectly by mozilla.
taking...
Assignee: law → cbiesinger
Created attachment 135393 [details] [diff] [review]
patch

use RegQueryValueExW on nt-based windows versions where that's supported (which
returns a UTF-16 string), and the ascii version otherwise
Attachment #135393 - Flags: review?(ere)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.6beta

Comment 5

15 years ago
Comment on attachment 135393 [details] [diff] [review]
patch

I suggest doing an early return from GetMIMEInfoFromRegistry() if rc !=
ERROR_SUCCESS. Anyway, r=ere
Attachment #135393 - Flags: review?(ere) → review+
Comment on attachment 135393 [details] [diff] [review]
patch

ok, I'll make that change
Attachment #135393 - Flags: superreview?(bz-vacation)
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.
Attachment #135393 - Flags: superreview?(bz-vacation) → superreview+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
> 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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
sigh. I'm too tired today :(
thanks for looking at what I did, sorry about these mistakes...
I fixed this now.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED
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... ;)

hm, yeah, looks like it should. should I just check in that change with
r+sr=bzbarsky?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.