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

RESOLVED FIXED in mozilla2.0b7

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
mozilla2.0b7
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(status1.9.2 .17-fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Posted patch patch rev1 (obsolete) — Splinter Review
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #476479 - Flags: feedback?(mook)
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
Attachment #476479 - Flags: feedback?(reed)
Posted patch patch rev2 (obsolete) — Splinter Review
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
Attachment #476479 - Attachment is obsolete: true
Attachment #476488 - Flags: review?(dolske)
Attachment #476479 - Flags: review?(dolske)
Attachment #476479 - Flags: feedback?(reed)
Attachment #476479 - Flags: feedback?(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 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! :)
Attachment #476488 - Attachment is obsolete: true
Attachment #476488 - Flags: review?(dolske)
Posted patch patch rev3 (obsolete) — Splinter Review
Attachment #476511 - Flags: review?(dolske)
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
Attachment #476511 - Flags: review?(dolske)
Posted patch patch rev4 (obsolete) — Splinter Review
Attachment #476511 - Attachment is obsolete: true
Attachment #476717 - Flags: review?(dolske)
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 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");
Attachment #476717 - Flags: review?(dolske) → review+
Posted patch patch rev5Splinter Review
Attachment #476717 - Attachment is obsolete: true
Attachment #477022 - Flags: review+
Attachment #477022 - Flags: approval2.0?
Attachment #477022 - Flags: approval2.0? → approval2.0+
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
(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 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
Attachment #517685 - Flags: review+
Attachment #517685 - Flags: approval1.9.2.16?
Comment on attachment 517685 [details] [diff] [review]
1.9.2 patch

Approved for 1.9.2.16, a=dveditz for release-drivers
Attachment #517685 - Flags: approval1.9.2.16? → approval1.9.2.16+
Blocks: 458729
Duplicate of this bug: 458729
You need to log in before you can comment on or make changes to this bug.