Closed
Bug 194439
Opened 22 years ago
Closed 21 years ago
nsOSHelperAppService on windows destroys non-latin1 default description
Categories
(Core Graveyard :: File Handling, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.6beta
People
(Reporter: Biesinger, Assigned: Biesinger)
References
()
Details
Attachments
(1 file)
10.13 KB,
patch
|
emaijala+moz
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
(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•22 years ago
|
||
ccing some people who may have a clue about the Win9x unicode issues.
Assignee | ||
Comment 2•22 years ago
|
||
oh, and as for "destroy" - the actual registry entry is of course not destroyed.
it only gets displayed incorrectly by mozilla.
Assignee | ||
Comment 4•21 years ago
|
||
use RegQueryValueExW on nt-based windows versions where that's supported (which
returns a UTF-16 string), and the ascii version otherwise
Assignee | ||
Updated•21 years ago
|
Attachment #135393 -
Flags: review?(ere)
Assignee | ||
Updated•21 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.6beta
Comment 5•21 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+
Assignee | ||
Comment 6•21 years ago
|
||
Comment on attachment 135393 [details] [diff] [review]
patch
ok, I'll make that change
Attachment #135393 -
Flags: superreview?(bz-vacation)
![]() |
||
Comment 7•21 years ago
|
||
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+
Assignee | ||
Comment 8•21 years ago
|
||
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
Closed: 21 years ago
Resolution: --- → FIXED
![]() |
||
Comment 9•21 years ago
|
||
> 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 → ---
Assignee | ||
Comment 10•21 years ago
|
||
sigh. I'm too tired today :(
thanks for looking at what I did, sorry about these mistakes...
I fixed this now.
Status: REOPENED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
![]() |
||
Comment 11•21 years ago
|
||
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... ;)
Assignee | ||
Comment 12•21 years ago
|
||
hm, yeah, looks like it should. should I just check in that change with
r+sr=bzbarsky?
![]() |
||
Comment 13•21 years ago
|
||
Sure.
Assignee | ||
Comment 14•21 years ago
|
||
ok, done
Updated•9 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•