597630 - Fix __snprintf / snwprintf usage in updater.cpp and various x64 compiler warnings

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

patch coming up

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 476479 [details] [diff] [review]
patch rev1

If any of you have a chance to look over this I'd appreciate it! Thanks

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

fixes a silly mistake in the macro.

I'm not going to set up the feedback requests again but feel free to do a drive by review. Thanks

Comment 4

[:Mook]

Comment on attachment 476488 [details] [diff] [review]
patch rev2

Just driving by; feel free to fish the stuff I threw into the pot back out.
Err.. okay, mixing metaphors doesn't work very well.

>+++ b/toolkit/mozapps/update/updater/progressui_win.cpp

> InitDialog(HWND hDlg)

>+  if (GetStringsFile(filename) && ReadStrings(filename, &uiStrings) == OK) {
>+    int len = MultiByteToWideChar(CP_UTF8, 0, uiStrings.title, -1, NULL, 0);
>+    if (len != 0) {
>+      len = MultiByteToWideChar(CP_UTF8, 0, uiStrings.title, -1, szwTitle, len);
>+      // Use a hardcoded string as a last ditch effort.
>+      if (len == 0)
>+        wcscpy(szwTitle, TITLE_FALLBACK);
>+    }
so the first call to MultiByteToWideChar is to catch failed conversion
(garbled encoding of the ini file), and to not set any text in that case?
wouldn't it be more useful to just try to use the string, and if it fails for
any reason, use the fallback text?
if (0 == MultiByteToWideChar(CP_UTF8, 0, uiStrings.title, -1, szwTitle, NS_ARRAY_LENGTH(szwTitle))) {
  // Use a hardcoded string as a last ditch effort.
  wcscpy(szwTitle, TITLE_FALLBACK);
}

>+  } else {
>+    // Use hardcoded strings as a last ditch effort.
>+    wcscpy(szwTitle, TITLE_FALLBACK);
>+    wcscpy(szwInfo, INFO_FALLBACK);
would it be useful to PR_STATIC_ASSERT that sizeof(TITLE_FALLBACK) is no more than sizeof(szwTitle)?

>+++ b/toolkit/mozapps/update/updater/updater.cpp

>+// On Windows _snprintf does not guarantee NULL termination so do it ourselves.
>+# define snprintf(dest, count, fmt, ...) \
don't you want the equivalent of PR_BEGIN_MACRO / PR_END_MACRO here?
>+  { \
>+    size_t len = _snprintf(dest, count - 1, fmt, ##__VA_ARGS__); \
>+    if (len >= 0) \
>+      dest[len] = L'\0'; \
if I'm reading http://msdn.microsoft.com/en-us/library/2ts7cx93.aspx right, in
the case that we're trying to fix, the result is actually less than zero. it
would probably be easier (and possibly faster) to just do an unconditional
dest[count] = '\0' here - it's fine to have more than one null terminator in a
string buffer. (also: _snprintf isn't wchar_t, no L)
P.S. you're awesome for managing to do this in a way that, if I read things
right, should be mingw-compatible :)

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 476488 [details] [diff] [review]
patch rev2

(In reply to comment #4)
> Comment on attachment 476488 [details] [diff] [review]
> patch rev2
> 
> Just driving by; feel free to fish the stuff I threw into the pot back out.
> Err.. okay, mixing metaphors doesn't work very well.
Thanks and always feel free to do a drive by on my patches. mingw compatibility was definitely intended. I'm probably going to clean up the MultiByteToWideChar usage a bit more. I'll implement the remaining comments in some fashion or another. Thanks again and keep mixing those metaphors! :)

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: patch rev3

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 476511 [details] [diff] [review]
patch rev3

I ran this through the try server and it failed in the test_0110_general.js, test_0111_general.js, and test_0112_general.js xpcshell tests though they had been passing locally :(

I'll try to figure out what's going on

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: patch rev4

Comment 9

[Mook (songbird dead account)]

Comment on attachment 476717 [details] [diff] [review]
patch rev4

looks good to me

>+++ b/toolkit/mozapps/update/updater/updater.cpp

> WriteStatusFile(int status)

>   char buf[32];
>   if (status == OK) {
>     text = "succeeded\n";
>   } else {
>-    snprintf(buf, sizeof(buf), "failed: %d\n", status);
>+    snprintf(buf, sizeof(buf)/sizeof(buf[0]), "failed: %d\n", status);
This doesn't check anything (not that it needs to).
Especially since all this *actually* does is
 if (status == OK) fprintf(file, "succeeded\n");
 else fprintf(file, "failed: %d\n", status);
and there was no need to even have an intermediate buffer... was there?
>     text = buf;
>   }
>   fwrite(text, strlen(text), 1, file);
> }

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 476717 [details] [diff] [review]
patch rev4

>+# define snprintf(dest, count, fmt, ...) \
>+  PR_BEGIN_MACRO \
>+    _snprintf(dest, count - 1, fmt, ##__VA_ARGS__); \
>+    dest[count - 1] = '\0'; \
>+  PR_END_MACRO
>+# define NS_tsnprintf(dest, count, fmt, ...) \
>+  PR_BEGIN_MACRO \
>+    _snwprintf(dest, count - 1, fmt, ##__VA_ARGS__); \
>+    dest[count - 1] = L'\0'; \
>+  PR_END_MACRO

These reference |count| twice, which would result in it being evaluated twice. Seems really unlikely someone would use it this way, though. Probably worth fixing by assigning to a temporary in the macro?

EG: snprintf(dst, len++, "wtf");

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: patch rev5

Comment 12

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/f7c52846caa3

These are all correctness fixes and there really isn't much we could do to test that would be of any value.

Comment 13

[Robert Strong (they/them - no direct email)]

(In reply to comment #9)
> Comment on attachment 476717 [details] [diff] [review]
> patch rev4
> 
> looks good to me
> 
> >+++ b/toolkit/mozapps/update/updater/updater.cpp
> 
> > WriteStatusFile(int status)
> 
> >   char buf[32];
> >   if (status == OK) {
> >     text = "succeeded\n";
> >   } else {
> >-    snprintf(buf, sizeof(buf), "failed: %d\n", status);
> >+    snprintf(buf, sizeof(buf)/sizeof(buf[0]), "failed: %d\n", status);
> This doesn't check anything (not that it needs to).
> Especially since all this *actually* does is
>  if (status == OK) fprintf(file, "succeeded\n");
>  else fprintf(file, "failed: %d\n", status);
> and there was no need to even have an intermediate buffer... was there?
> >     text = buf;
> >   }
> >   fwrite(text, strlen(text), 1, file);
> > }
Sorry about not getting this in at the same time... other work was knocking (more like pounding) on my door.

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: 1.9.2 patch

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 517685 [details] [diff] [review]
1.9.2 patch

Since will likely be taking several updater changes on 1.9.2 I'd like to get this as well so the code is consistent with Firefox 4 / trunk

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 517685 [details] [diff] [review]
1.9.2 patch

Approved for 1.9.2.16, a=dveditz for release-drivers

Comment 17

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/942c74f2da4b