216388 - obsolete RegOpenKey API used

Status: RESOLVED WORKSFORME

(SeaMonkey :: UI Design, defect)

Description

[Brant Gurganus]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030808 Mozilla Firebird/0.6.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030808 Mozilla Firebird/0.6.1+

The RegOpenKey API is being used.  It is obsolete and RegOpenKeyEx should be
used in its place.

Reproducible: Always

Steps to Reproduce:
1. Search for RegOpenKey with lxr.
2. One of the files is PalmSyncInstall.cpp
Actual Results:  
This file should not use RegOpenKey.

Expected Results:  
It should us RegOpenKeyEx instead.

I made a best guess at what access permission is necessary but most limited in
my patch.

Comment 1

[Brant Gurganus]

Attachment: Patch that changes RegOpenKey to RegOpenKeyEx

Comment 2

[Brant Gurganus]

Comment on attachment 129917 [details] [diff] [review]
Patch that changes RegOpenKey to RegOpenKeyEx

Since I can't compile Mozilla (no MSVC and Mingw that hasn't worked yet),
bsmedberg said for me to request a review from him to test the patch before
actual reviews requested.

Comment 3

[Benjamin Smedberg]

Comment on attachment 129917 [details] [diff] [review]
Patch that changes RegOpenKey to RegOpenKeyEx

looks good to me... who owns this code so you can get owner-approval?

Comment 4

[David Hallowell]

Comment on attachment 129917 [details] [diff] [review]
Patch that changes RegOpenKey to RegOpenKeyEx

Noticed this bug when looking at bug 237754 for Firefox. This patch is for Palm
Sync. I'll put together one for winhooks. It's been stagnant for a while so
would be good to get this checked in.

Comment 5

[David Hallowell]

Attachment: Patch for winhooks and MAPI RegOpenKey -> RegOpenKeyEx

Supplemental patch to the other patch, this one covers winhooks and mapi.

I think that's all the instances in Seamonkey where RegOpenKey is incorrectly
called

Comment 6

[David Hallowell]

Not sure of the correct component but it definitely has nothing to do with the
installer so moving to XP Apps.

If anyone has any ideas who could review my new patch then please add the
appropriate flag.

Should be easy enough to verify and possibly make it into 1.8a.

MSDN on RegOpenKey:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/regopenkey.asp

Comment 7

[Brant Gurganus]

Comment on attachment 146228 [details] [diff] [review]
Patch for winhooks and MAPI RegOpenKey -> RegOpenKeyEx

For the second portion of your patch (nsWindowsHookUtil.cpp), you are giving
KEY_ALL_ACCESS permissions to the operation, however, I don't think it needs
that much permission.  I think those permissions are are bit masks, so or'ing
them for the specific tasks needed will be better than all access.  I might be
wrong on this though, I'm not a C/C++ programmer yet.

Comment 8

[:deanis74]

KEY_READ = KEY_QUERY_VALUE | KEY_ENUMERATE_SUB_KEYS | KEY_NOTIFY

KEY_WRITE = KEY_SET_VALUE | KEY_CREATE_SUB_KEY

KEY_ALL_ACCESS = KEY_QUERY_VALUE | KEY_ENUMERATE_SUB_KEYS | KEY_NOTIFY |
                 KEY_CREATE_SUB_KEY | KEY_CREATE_LINK | KEY_SET_VALUE

Therefore KEY_ALL_ACCESS = KEY_READ | KEY_WRITE | KEY_CREATE_LINK.  That's not
that big of a deal, but maybe KEY_ALL_ACCESS could be changed to just KEY_READ |
KEY_WRITE?

Comment 9

[Bernard Alleysson]

The real test to do here is to test the code with a non-privileged account (no
administrator rights). KEY_ALL_ACCESS might only work with administrator
account. Any other flags that do the job for a non-privileged user is OK.

Comment 10

[David :Bienvenu]

I'll look at driving this home.

Comment 11

[David :Bienvenu]

I don't see any uses of RegOpenKey in comm-central.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 129917 [details] [diff] [review]
Patch that changes RegOpenKey to RegOpenKeyEx

Per comment 11, RegOpenKey is no longer present in the source. Therefore clearing obsolete review request.