457737 - Convert nsRegisterGREWin to Wide calls

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect)

Description

[Doug Turner (:dougt)]

Attachment: patch v.1

patch from blassey.  converts nsRegisterGREWin.cpp to use wide system calls

Comment 1

[Benjamin Smedberg]

Comment on attachment 340959 [details] [diff] [review]
patch v.1

>diff -r 38501014ee43 xulrunner/app/nsRegisterGREWin.cpp

>-  if (::RegCreateKeyEx(root, keyname, NULL, NULL, 0, KEY_WRITE, NULL,
>+  // wchar_t* keyname = (wchar_t*)(NS_ConvertUTF8toUTF16(keynameAscii).get());
>+  if (::RegCreateKeyExW(root, keyname, NULL, NULL, 0, KEY_WRITE, NULL,
>                        &subkey, &disp) != ERROR_SUCCESS)

Why UTF8 instead of ASCII? We're guaranteed that the keyname is ASCII, right?

>-  failed |= ::RegSetValueEx(subkey, "Version", NULL, REG_SZ,
>+  failed |= ::RegSetValueExW(subkey, L"Version", NULL, REG_SZ,
>                             (BYTE*) aGREMilestone,
>-                            strlen(aGREMilestone)) != ERROR_SUCCESS;
>-  failed |= ::RegSetValueEx(subkey, "GreHome", NULL, REG_SZ,
>+                            wcslen(aGREMilestone)) != ERROR_SUCCESS;

You have replaced strlen with wcslen. I'm almost positive that this is incorrect... this is supposed to be the size of the data in bytes, not the size in characters. The original code was also slightly broken, because it didn't include the extra byte for the null-terminator, which is required, see http://msdn.microsoft.com/en-us/library/ms724923(VS.85).aspx

>+    wchar_t* prop = (wchar_t*)NS_ConvertUTF8toUTF16(aProperties[i].property).get();
>+    wchar_t* val = (wchar_t*)malloc( strlen(aProperties[i].value)*2);

Currently the charset of properties is undefined. You should document that they should be ASCII-only, and then just byte-expand them instead of doing UTF8 conversion.

>-  strcpy(keyName, aGREMilestone);
>+  wcscpy(keyName, aGREMilestone);

This changes the charset of the file from ASCII to UTF16, which I don't think is acceptable. We should be writing the file in ASCII.

Comment 2

[Doug Turner (:dougt)]

brad can you clean up?

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: updated based on bsmedberg's comments

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #1)
> (From update of attachment 340959 [details] [diff] [review])
> >diff -r 38501014ee43 xulrunner/app/nsRegisterGREWin.cpp
> 
> >-  if (::RegCreateKeyEx(root, keyname, NULL, NULL, 0, KEY_WRITE, NULL,
> >+  // wchar_t* keyname = (wchar_t*)(NS_ConvertUTF8toUTF16(keynameAscii).get());
> >+  if (::RegCreateKeyExW(root, keyname, NULL, NULL, 0, KEY_WRITE, NULL,
> >                        &subkey, &disp) != ERROR_SUCCESS)
> 
> Why UTF8 instead of ASCII? We're guaranteed that the keyname is ASCII, right?

The utf8 conversion is commented out, but I removed it in the update patch

> >+    wchar_t* prop = (wchar_t*)NS_ConvertUTF8toUTF16(aProperties[i].property).get();
> >+    wchar_t* val = (wchar_t*)malloc( strlen(aProperties[i].value)*2);
> 
> Currently the charset of properties is undefined. You should document that they
> should be ASCII-only, and then just byte-expand them instead of doing UTF8
> conversion.

I changed the conversion, and added a comment.  Is that what you mean by document?  or put something on mdc?
> 
> >-  strcpy(keyName, aGREMilestone);
> >+  wcscpy(keyName, aGREMilestone);
> 
> This changes the charset of the file from ASCII to UTF16, which I don't think
> is acceptable. We should be writing the file in ASCII.

I put a conversion just before the two PR_Write's

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: updated based on all of bsmedberg's comments

Comment 7

[Benjamin Smedberg]

Comment on attachment 343174 [details] [diff] [review]
updated based on all of bsmedberg's comments

>diff -r 37c122b5400b xulrunner/app/nsRegisterGREWin.cpp

>+    // Properties should be ascii only 
>+    wchar_t* prop = (wchar_t*)NS_ConvertASCIItoUTF16(aProperties[i].property).get();

"prop" will go out of scope immediately. I think you should just substitute this expression directly... see my code below.

>+    wchar_t* val = (wchar_t*)malloc( strlen(aProperties[i].value)*2);
>+    failed |= ::RegSetValueExW(subkey, prop, NULL, REG_SZ,
>+			       (BYTE*) val, sizeof(val)
>+                              ) != ERROR_SUCCESS;
>+    strcpy((char*)NS_ConvertUTF16toASCII(val).get(), aProperties[i].value) ;

This code doesn't make sense... you haven't initialized the "val" buffer before calling RegSetValueExW, and sizeof(val) will be the size of the pointer, not the size of the buffer. I think you want:

NS_ConvertASCIItoUTF16 wval(aProperties[i].value);
failed |= ::RegSetValueExW(subkey, NS_ConvertASCIIToUTF16(aProperties[i].property).get(),
                           NULL, REG_SZ, (BYTE*) wval.get(), (wval.Length() + 1) * sizeof(WCHAR)) != ERROR_SUCCESS;

>+  const wchar_t* aGREMilestone = (const wchar_t*)NS_ConvertASCIItoUTF16(aGREMilestoneAscii).get();

Similar scope issues here.

>+    char* keyNameAscii = NS_ConvertUTF16toASCII(nsDependentString(keyName)).get();

and here

>+      char* keyNameAscii = NS_ConvertUTF16toASCII(nsDependentString(keyName)).get();

and here

Comment 8

[Benjamin Smedberg]

The docs about properties being ASCII-only should be noted here: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/standalone/nsXPCOMGlue.h#86

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fixed scope issues

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: adds note about ascii properties

Comment 11

[Benjamin Smedberg]

Comment on attachment 343950 [details] [diff] [review]
fixed scope issues

>diff -r 24fcf9b34965 xulrunner/app/nsRegisterGREWin.cpp

>   for (PRUint32 i = 0; i < aPropertiesLen; ++i) {
>-    failed |= ::RegSetValueEx(subkey, aProperties[i].property, NULL, REG_SZ,
>-                              (BYTE*) aProperties[i].value,
>-                              strlen(aProperties[i].value)) != ERROR_SUCCESS;
>+    // Properties should be ascii only 
>+    NS_ConvertASCIItoUTF16 prop(aProperties[i].property);
>+    NS_ConvertASCIItoUTF16 val(aProperties[i].value);
>+    failed |= ::RegSetValueExW(subkey, prop.get(), NULL, REG_SZ, 
>+			       (BYTE*) val.get(), 
>+			       sizeof(wchar_t)*(val.Length()+1)
>+			       ) != ERROR_SUCCESS;
>+    strcpy((char*)NS_ConvertUTF16toUTF8(val).get(), aProperties[i].value);
>   }

What is the last strcpy for? It seems superfluous, perhaps leftover from a previous version?

>-  nsCString greHome;
>-  rv = aLocation->GetNativePath(greHome);
>+   NS_ConvertASCIItoUTF16 aGREMilestone(aGREMilestoneAscii);
>+  nsString greHome;
>+  rv = aLocation->GetPath(greHome);
>   if (NS_FAILED(rv))
>     return rv;

Nit, fix whitespace.

>-  if (::RegCreateKeyEx(aRegisterGlobally ? HKEY_LOCAL_MACHINE :
>+  if (::RegCreateKeyExW(aRegisterGlobally ? HKEY_LOCAL_MACHINE :
>                                            HKEY_CURRENT_USER,

nit, fix whitespace

>   HKEY rootKey = NULL;
>-  if (::RegOpenKeyEx(aGlobal ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER,
>+  if (::RegOpenKeyExW(aGlobal ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER,
>                      kRegKeyRoot, 0, KEY_READ, &rootKey) != ERROR_SUCCESS)

nit, whitespace

r=me with nits fixed

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

2fccc2d253e2: Bug 457737 - Convert nsRegisterGREWin to Wide calls r=bsmedberg
diff
browse
Brad Lassey <[e-mail]> - Mon, 20 Oct 2008 16:15:32 -0400 - rev 20681
Bug 457737 - Convert nsRegisterGREWin to Wide calls r=bsmedberg
 
resolving as fixed, but still need review on the documentation patch