[tbb-bugs] #31567 [Applications/Tor Browser]: NS_tsnprintf() does not handle %s correctly on Windows

[Tor Bug Tracker & Wiki]

#31567: NS_tsnprintf() does not handle %s correctly on Windows
-------------------------------------+-------------------------------------
     Reporter:  mcs                  |      Owner:  gk
         Type:  defect               |     Status:  assigned
     Priority:  Very High            |  Milestone:
    Component:  Applications/Tor     |    Version:
  Browser                            |   Keywords:  ff68-esr, tbb-9.0-must-
     Severity:  Critical             |  alpha, TorBrowserTeam201908
Actual Points:                       |  Parent ID:
       Points:                       |   Reviewer:
      Sponsor:                       |
-------------------------------------+-------------------------------------
 While testing the ESR68-based updater on Windows, Kathy and I found a
 Windows toolchain problem. We tested using our own 64-bit build (rbm
 nightly build process) on a Windows 10 system.

 We discovered that file paths generated using format strings are
 corrupted. Specifically, calls to `NS_tsnprintf()` do not work correctly;
 apparently, because that is a "wide" function a `%s` when used in the
 format string is supposed to mean "expect the arg to be of type WCHAR *".
 Instead, the args are processed as C-style strings which means they get
 truncated after the first character (at least when using characters that
 fit within the first 256 Unicode codepoints).

 `NS_tsnprintf()` is a macro that is defined in
 toolkit/mozapps/update/common/updatedefines.h (all of the code that uses
 it is related to the updater). We first noticed that problem when we saw a
 failure inside updater.cpp's `WriteToFile()` function. The following code
 from that function fails because it tries to move `filename` to a bad
 path.
 {{{
 #if defined(XP_WIN)
   NS_tchar dstfilename[MAXPATHLEN] = {NS_T('\0')};
   NS_tsnprintf(dstfilename, sizeof(dstfilename) / sizeof(dstfilename[0]),
                NS_T("%s\\%s"), gPatchDirPath, aFilename);
   if (MoveFileExW(filename, dstfilename, MOVEFILE_REPLACE_EXISTING) == 0)
 {
     return false;
   }
 #endif
 }}}
 The computed path is `C\u` (the first character from `gPatchDirPath`
 followed by the \ and then the first character of `aFilename`).

 On Windows, `NS_tsnprintf()` is defined as `mywcsprintf` which is an
 inline function that uses `_vsnwprintf()`. We have not traced this bug
 deeper than that point, but we did verify that the problems disappear if
 we replace all occurrences of `%s` with `%S` in the `NS_tsnprintf()`
 format strings. That is a very ugly fix though, and it would be wrong on
 macOS and Linux.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/31567>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online