NS_tsnprintf() does not handle %s correctly on Windows

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.

Trac:
Parent Ticket: #30322 (moved)

added TorBrowserTeam201908R component::applications/tor browser ff68-esr owner::gk parent::30322 priority::very high resolution::fixed severity::critical status::closed tbb-9.0-must-alpha tbb-update type::defect labels

This problem prevented us from making much progress today in testing the ESR68-based updater on Windows. Does anyone else have time to work on this bug? Kathy and I are mostly away from keyboard between now and our release date :(

One possible workaround would be to modify the mywcsprintf () function to replace all occurrences of %s with %S in the format string before calling _vsnwprintf(). We should also verify that the same bug exists in 32bit builds or that any workaround we implement does not break the 32bit updater.

See https://gitweb.torproject.org/tor-browser.git/tree/toolkit/mozapps/update/common/updatedefines.h?h=tor-browser-68.1.0esr-9.0-1#n44

-municode-related...

https://hg.mozilla.org/mozilla-central/rev/663d87d66315#l3.12 instead of https://hg.mozilla.org/mozilla-central/rev/7bbedf6937aa#l5.47

Replying to cypherpunks:

https://hg.mozilla.org/mozilla-central/rev/663d87d66315#l3.12 instead of https://hg.mozilla.org/mozilla-central/rev/7bbedf6937aa#l5.47

What do you mean?

Trac:
Cc: tbb-team to tbb-team, tom
Status: assigned to needs_information

Sry? Replacing elif CONFIG['OS_ARCH'] == 'WINNT': with if CONFIG['OS_TARGET'] == 'WINNT' and CONFIG['CC_TYPE'] in ('gcc', 'clang'):. And you?

tldr there's a patch at the end

Did some digging on this tonight just for fun. According to this post on stack overflow ( https://stackoverflow.com/a/18283757 ) libstdc++ is built with the __USE_MINGW_ANSI_STDIO define, so calls to the printf and wprintf function families will use the expected format string behaviour found on non-windows platforms. Apparently libstdc++ depends on the ANSI stdio behavior so it's toggled on.

The following program will output differently depending on which headers you use (C or C++) even if you build with mingw g++

#ifdef USE_STDC
#include <stdio.h>
#include <wchar.h>
#else
#include <cstdio>
#include <cwchar>
##endif

int main()
{
    printf("1: %s\n", "ansi format, ansi string");
    printf("2: %S\n", L"ansi format, wide string");
    wprintf(L"3: %s\n", "wide format, ansi string");
    wprintf(L"4: %S\n", L"wide format, wide string");
    fflush(stdout);
}

---

If you build with the C headers, you get the following output (wine on linux):

1: ansi format, ansi string
2: ansi format, wide string
3: 4: w

This it the output you would expect if you built with vcs given Microsoft's printf format specifiers ( https://docs.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=vs-2019 )

The first two prints work as expected, but the next two fail: 3 is expecting the string to be a wide character string, but it encounters an ansi string. I don't know what this ansi string encodes as wide string, but it would seem wprintf gives up trying to print it. For shits and giggles, if we replace 3's ansi string with "w\0i\0d\0e\0\0" it prints out 'wide' to the console. 4 is expecting an ansi string but gets a wide string, so it prints the ansi 'w', finds a NULL byte and treats it as a terminator, only printing out 'w'.

---

If you build with the C++ headers (or if you build with the C headers and add #define __USE_MINGW_ANSI_STDIO 1 before including stdio.h, you get the following output:

1: ansi format, ansi string
2: ansi format, wide string
3: wide format, ansi string
4: wide format, wide string

Which is as one would expect I think. Undefining __USE_MINGW_ANSI_STDIO or redifining it to 0 does not seem to reverse the effect when using the C++ headers.

---

However, we probably shouldn't go fucking around with this define because who what else depends on this behaviour to work. Instead, we should replace the _vsnwprintf call with StringCchVPrintfW. As StringCchVPrintfW has no expectation of conforming to ANSI C string format specifiers, it can instead ignore the __USE_MINGW_ANSI_STDIO define and always conform the Microsoft's specifiers. The following example builds and runs using the Microsoft formats:

#include <windows.h>
#include <strsafe.h>

#include <iostream>
using namespace std;

int main()
{
    char abuffer[1024];
    wchar_t wbuffer[1024];

    StringCchPrintfA(abuffer, 1024, "1: %s\n", "ansi format, ansi string"); // small S for ansi
    cout << abuffer;
    StringCchPrintfA(abuffer, 1024, "2: %S\n", L"ansi format, wide string"); // big S for wide
    cout << abuffer;

    cout << flush;

    StringCchPrintfW(wbuffer, 1024, L"3 : %S\n", "wide format, ansi string"); // big S for ansi
    wcout << wbuffer;
    StringCchPrintfW(wbuffer, 1024, L"4': %s\n", L"wide format, wide string"); // small S for wide
    wcout << wbuffer;

    wcout << flush;
}

---

This patch replaces the _vsnwprintf with StringCchVPrintfExW from strsafe.h, a windows specific function and as such implements Microsoft's format-string specifiers. The new implementation of mywcsprintf has the same outputs and retval as the old for a handful of test-cases.

tor-browser patch:

https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_31567

EDIT: we may need to add #define STRSAFE_NO_DEPRECATE before the strsafe.h include if the updatedefines.h header is included in any modules that use the deprecated un-safe string methods the StringCch.* methods were meant to replace.

Trac:
Status: needs_information to needs_review
Keywords: TorBrowserTeam201908 deleted, TorBrowserTeam201908R added

Here comes a 64bit testbuild and it's accompanied .mar file. mcs/brade: I know you don't have much time today but it would be helpful if you could give a short feedback whether that worked (assuming any of us has a hard time setting up the infrastructure for such a test today-ish):

https://people.torproject.org/~gk/testbuilds/torbrowser-install-win64-tbb-nightly_en-US.exe https://people.torproject.org/~gk/testbuilds/torbrowser-install-win64-tbb-nightly_en-US.exe.asc

https://people.torproject.org/~gk/testbuilds/tor-browser-win64-tbb-nightly_en-US.mar https://people.torproject.org/~gk/testbuilds/tor-browser-win64-tbb-nightly_en-US.mar.asc

Replying to gk:

Here comes a 64bit testbuild and it's accompanied .mar file. mcs/brade: I know you don't have much time today but it would be helpful if you could give a short feedback whether that worked (assuming any of us has a hard time setting up the infrastructure for such a test today-ish): ...

Thanks for the builds, and Richard thank you so much for hacking on this last night. Unfortunately, the problem is not fixed and I think the same error is occurring (Unicode strings are being interpreted as C/ANSI strings). I do not know why, but maybe our cypherpunks are on the right track in that this may be a general problem with how updater.exe gets built.

I will attach a Windows cmd file that can be used to run the updater "manually" to reproduce the problem. After you run the updater via the cmd file, look under ...\Browser\TorBrowser\UpdateInfo\updates\0 for newly created files. When the updater fails due to this bug, you will see status and log files there that are named sta and log. The GUID suffix is there because the rename (MoveFileExW() call) fails as I described in the bug description. The contents of the log file will be failed setting status to 'applying'.

Trac:
updater-test.cmd

Windows cmd file for running the updater

FWIW: I did not do #define STRSAFE_NO_DEPRECATE before the strsafe.h as I saw the edit after I started my build. Might be a first thing to test.

As for __USE_MINGW_ANSI_STDIO, now when ucrt is used, mingw-w64 should definitely be patched with

-# define __USE_MINGW_ANSI_STDIO 1
+# define __USE_MINGW_ANSI_STDIO 0

in its ugly defaults: https://github.com/mingw-w64/mingw-w64/blob/f8e2c9ac594259fd2a46746943f84bd8766d7054/mingw-w64-headers/crt/_mingw.h.in#L428

mcs: we need to update NS_tvsnprintf's usage of _vsnwprintf as well. Will post a patch shortly.

EDIT: perhaps not actually, NS_tvsnprintf only seems to be used in the tests :/

I think it is also worth noting that ESR60 included this same code, more or less. That points to a toolchain change as the root cause.

Trac:
Keywords: TorBrowserTeam201908R deleted, TorBrowserTeam201908 added
Status: needs_review to needs_revision

Ok, so the reason why the above patch does not work with the clang build is that StringCchVPrintfExW which was meant to replace _vsnwprintf ultimately calls _vsnwprintf itself. This call ulimately goes down to ucrtbase!__stdio_common_vswprintf with the UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION (0x1) flag. Switching this flag to UCRTBASE_PRINTF_LEGACY_WIDE_SPECIFIERS (0x4) results in the behaviour that we want.

Patching the binary at runtime in windbg and letting it run results in these files:

update.log:

PATCH DIRECTORY C:\Users\user\Desktop\GKTest\Browser\TorBrowser\UpdateInfo\updates\0
INSTALLATION DIRECTORY C:\Users\user\Desktop\GKTest
WORKING DIRECTORY C:\Users\user\Desktop\GKTest
failed: 6
calling QuitProgressUI

update.status:

failed: 6

Not sure what the correct behaviour is here with regards to the updater but at least we get this far.

I suspect this is a bug in mingw, but it's unclear to me at the moment what the right behaviour is here

copied from the mozilla bug:

So I'm tentatively going to claim this is a bug in mingw's implementation of _vsnwprintf in ucrt__vsnwprintf.c. I wrote a quick test program and built with msvc 19.21 and verified in windbg that a value of '5' (which corresponds to UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION | UCRTBASE_PRINTF_LEGACY_WIDE_SPECIFIERS ) while mingw's implementation only passes in '1' ( UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION ). Swapping the 5 to a 1 in the debugger gives us the same broken behavior.

Replying to pospeselr:

... Patching the binary at runtime in windbg and letting it run results in these files:

update.log:

{{{ PATCH DIRECTORY C:\Users\user\Desktop\GKTest\Browser\TorBrowser\UpdateInfo\updates\0 INSTALLATION DIRECTORY C:\Users\user\Desktop\GKTest WORKING DIRECTORY C:\Users\user\Desktop\GKTest failed: 6 calling QuitProgressUI }}}

update.status: {{{ failed: 6 }}}

Not sure what the correct behaviour is here with regards to the updater but at least we get this far.

I don't know why Mozilla doesn't add an errorToString() function for the updater error codes so they could log something more intelligible, but so far they have not done so. Anyway, you can find the updater error codes here: https://gitweb.torproject.org/tor-browser.git/tree/toolkit/mozapps/update/common/updatererrors.h?h=tor-browser-68.1.0esr-9.0-1

Error 6 is READ_ERROR and that is probably happening because I forgot to tell you to copy Georg's test mar file to ...\updates\0\update.mar. Sorry!

If the mar file is not signed (and I suspect it may not be), it will fail at some point anyway... but I think you got past the problem we are trying to solve in this ticket.