Opened 7 weeks ago

Closed 7 weeks ago

Last modified 5 weeks ago

#31567 closed defect (fixed)

NS_tsnprintf() does not handle %s correctly on Windows

Reported by: mcs Owned by: gk
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Critical Keywords: ff68-esr, tbb-9.0-must-alpha, TorBrowserTeam201908R, tbb-update
Cc: tbb-team, tom Actual Points:
Parent ID: #30322 Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Attachments (1)

updater-test.cmd (294 bytes) - added by mcs 7 weeks ago.
Windows cmd file for running the updater

Download all attachments as: .zip

Change History (39)

comment:1 Changed 7 weeks ago by mcs

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

comment:2 Changed 7 weeks ago by cypherpunks

-municode-related...

comment:4 in reply to:  3 Changed 7 weeks ago by gk

Cc: tom added
Status: assignedneeds_information

comment:5 Changed 7 weeks ago by cypherpunks

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

comment:6 Changed 7 weeks ago by pospeselr

Keywords: TorBrowserTeam201908R added; TorBrowserTeam201908 removed
Status: needs_informationneeds_review

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.

Last edited 7 weeks ago by pospeselr (previous) (diff)

comment:7 Changed 7 weeks ago by 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):

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

comment:8 in reply to:  7 Changed 7 weeks ago by mcs

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<GUID> and log<GUID>. 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'.

Changed 7 weeks ago by mcs

Attachment: updater-test.cmd added

Windows cmd file for running the updater

comment:9 Changed 7 weeks ago by gk

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.

comment:10 Changed 7 weeks ago by cypherpunks

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

comment:11 Changed 7 weeks ago by pospeselr

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 :/

Last edited 7 weeks ago by pospeselr (previous) (diff)

comment:12 Changed 7 weeks ago by mcs

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.

comment:13 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201908 added; TorBrowserTeam201908R removed
Status: needs_reviewneeds_revision

comment:14 Changed 7 weeks ago by pospeselr

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

comment:15 in reply to:  14 Changed 7 weeks ago by pospeselr

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.

Last edited 7 weeks ago by pospeselr (previous) (diff)

comment:16 in reply to:  14 ; Changed 7 weeks ago by mcs

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.

comment:17 Changed 7 weeks ago by gk

Okay, I built a new .exe with the following patch:

diff --git a/mingw-w64-crt/stdio/ucrt__vsnwprintf.c b/mingw-w64-crt/stdio/ucrt__vsnwprintf.c
index bf9f4de2..a13286e5 100644
--- a/mingw-w64-crt/stdio/ucrt__vsnwprintf.c
+++ b/mingw-w64-crt/stdio/ucrt__vsnwprintf.c
@@ -10,6 +10,6 @@
 
 int __cdecl _vsnwprintf(wchar_t * __restrict__ _Dest,size_t _Count,const wchar_t * __restrict__ _Format,va_list _Args) __MINGW_ATTRIB_DEPRECATED_SEC_WARN
 {
-  return __stdio_common_vswprintf(UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION, _Dest, _Count, _Format, NULL, _Args);
+  return __stdio_common_vswprintf(UCRTBASE_PRINTF_LEGACY_VSPRINTF_NULL_TERMINATION | UCRTBASE_PRINTF_LEGACY_WIDE_SPECIFIERS, _Dest, _Count, _Format, NULL, _Args);
 }
 int __cdecl (*__MINGW_IMP_SYMBOL(_vsnwprintf))(wchar_t *__restrict__, size_t, const wchar_t *__restrict__, va_list) = _vsnwprintf;
-- 
2.23.0.rc1

https://people.torproject.org/~gk/testbuilds/31567.exe
https://people.torproject.org/~gk/testbuilds/31567.exe.asc

and took a .mar file from our recent nightly builds. That seems to get me past the problem we have, resulting in output similar to pospeselr's in comment:14.

failed: 6 seems to mean #define READ_ERROR 6. Not sure why this .mar file should not be readable, though. I'd have more expected a certificate related error as it is not signed...

comment:18 in reply to:  16 ; Changed 7 weeks ago by gk

Replying to mcs:

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!

Good point, let me test that.

comment:19 in reply to:  18 Changed 7 weeks ago by gk

Replying to gk:

Replying to mcs:

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!

Good point, let me test that.

Yeah, that's been the problem. Now I get the error from #define CERT_VERIFY_ERROR 19. So, let me sign the .mar file next and try again.

comment:20 Changed 7 weeks ago by gk

Okay, I got past the cert error just to hit the UPDATE_SETTINGS_FILE_CHANNEL one. Might be due to my testing setup, hard to say from here. What I did was copying the release certs over so that they can be used for nightlies as well and then I just did a make nightly-windows-x86_64.

comment:21 in reply to:  20 ; Changed 7 weeks ago by mcs

Replying to gk:

Okay, I got past the cert error just to hit the UPDATE_SETTINGS_FILE_CHANNEL one. Might be due to my testing setup, hard to say from here. What I did was copying the release certs over so that they can be used for nightlies as well and then I just did a make nightly-windows-x86_64.

I cannot think of a reason why what you did would lead to a failure. The UPDATE_SETTINGS_FILE_CHANNEL error is generated when the updater cannot read and/or parse the file Browser\update-settings.ini. The contents of that file should look similar on all platforms and for all "flavors" of our builds... something like:

[Settings]
ACCEPTED_MAR_CHANNEL_IDS=torbrowser-torproject-release

Unfortunately, Kathy and I are traveling at the moment and cannot do any testing. I will try to do some late tonight or tomorrow.

comment:22 Changed 7 weeks ago by mcs

gk -- hopefully the UPDATE_SETTINGS_FILE_CHANNEL error is caused by your test setup. However, in case Kathy and I need to do some debugging tomorrow, can you tell me how to apply your patch from comment:17 during the build? Do I just need to place your patch in projects/mingw-w64-clang/31567.patch? And will rbm then automatically rebuild the mingw-w64-clang project?

comment:23 in reply to:  22 Changed 7 weeks ago by gk

Replying to mcs:

gk -- hopefully the UPDATE_SETTINGS_FILE_CHANNEL error is caused by your test setup. However, in case Kathy and I need to do some debugging tomorrow, can you tell me how to apply your patch from comment:17 during the build? Do I just need to place your patch in projects/mingw-w64-clang/31567.patch? And will rbm then automatically rebuild the mingw-w64-clang project?

I backported the potential patches Martin provided and am currently testing the build in my bug_31567. You could use that one if needed.

I try to look closer into the error I got later today but still hope that this is due to us not having proper support for updating nightly builds yet.

comment:24 in reply to:  21 ; Changed 7 weeks ago by gk

Replying to mcs:

Replying to gk:

Okay, I got past the cert error just to hit the UPDATE_SETTINGS_FILE_CHANNEL one. Might be due to my testing setup, hard to say from here. What I did was copying the release certs over so that they can be used for nightlies as well and then I just did a make nightly-windows-x86_64.

I cannot think of a reason why what you did would lead to a failure. The UPDATE_SETTINGS_FILE_CHANNEL error is generated when the updater cannot read and/or parse the file Browser\update-settings.ini. The contents of that file should look similar on all platforms and for all "flavors" of our builds... something like:

[Settings]
ACCEPTED_MAR_CHANNEL_IDS=torbrowser-torproject-release

Yes, that's what the file in the nightly build contains.

comment:25 Changed 7 weeks ago by cypherpunks

Martin has confirmed that this is not a bug, but a correct C99-conformant behavior of mingw-w64, which automatically defaults to __USE_MINGW_ANSI_STDIO on POSIX crosses: https://bugzilla.mozilla.org/show_bug.cgi?id=1577872#c7

Last edited 7 weeks ago by cypherpunks (previous) (diff)

comment:26 in reply to:  24 Changed 7 weeks ago by gk

Replying to gk:

Replying to mcs:

Replying to gk:

Okay, I got past the cert error just to hit the UPDATE_SETTINGS_FILE_CHANNEL one. Might be due to my testing setup, hard to say from here. What I did was copying the release certs over so that they can be used for nightlies as well and then I just did a make nightly-windows-x86_64.

I cannot think of a reason why what you did would lead to a failure. The UPDATE_SETTINGS_FILE_CHANNEL error is generated when the updater cannot read and/or parse the file Browser\update-settings.ini. The contents of that file should look similar on all platforms and for all "flavors" of our builds... something like:

[Settings]
ACCEPTED_MAR_CHANNEL_IDS=torbrowser-torproject-release

Yes, that's what the file in the nightly build contains.

I looked at the mar executable to check what ID actually gets embedded during the build process and it shows Default Channel ID: torbrowser-torproject-release. So, this should be fine. I guess the next step is finding out what the updater thinks it got for a channel ID.

Last edited 7 weeks ago by gk (previous) (diff)

comment:27 Changed 7 weeks ago by gk

   int result =
      ReadStrings(path, kUpdaterKeys, kNumStrings, updater_strings, "Settings");

causes result to be 6 which means READ_ERROR and I get garbage back for the channel ID. Hrm. Let me try finding out where exactly the reading fails. I wonder whether that's another instance of mingw-w64 bugs we need to get patched. :/

comment:28 Changed 7 weeks ago by gk

mcs/brade: Oh, and don't let my above comment stop you from debugging if you have the time and energy for that. :) Or let me know about some shortcuts I could take to figure out what is going on.

comment:29 in reply to:  28 ; Changed 7 weeks ago by mcs

Replying to gk:

mcs/brade: Oh, and don't let my above comment stop you from debugging if you have the time and energy for that. :) Or let me know about some shortcuts I could take to figure out what is going on.

OK. The thing that does not make sense to me is that on 2019-08-29 Kathy and I were able to successfully run an update with our own Windows build after we replaced all of the %s format specifiers with %S. That test was interactive (using our own mar file and update server, and with tor-browser just patched for our own signing certificate and to use a different app.update.url). If there was another mingw-w64 bug we should have encountered it during that test.

comment:30 in reply to:  29 ; Changed 7 weeks ago by gk

Replying to mcs:

Replying to gk:

mcs/brade: Oh, and don't let my above comment stop you from debugging if you have the time and energy for that. :) Or let me know about some shortcuts I could take to figure out what is going on.

OK. The thing that does not make sense to me is that on 2019-08-29 Kathy and I were able to successfully run an update with our own Windows build after we replaced all of the %s format specifiers with %S. That test was interactive (using our own mar file and update server, and with tor-browser just patched for our own signing certificate and to use a different app.update.url). If there was another mingw-w64 bug we should have encountered it during that test.

I see. Well, it could be that Martin's patch opened a new hole while closing the previous bug. Or it could still be an issue with my setup. Or it could be related to me not using a full update setup but rather just your script. However, I somehow doubt the latter at least as using the updater executable from 9.0a4 + the newly created .mar file works. Additionally, it could be that you replacing the format specifiers killed that second, different bug as well. And there are probably some more explanations I forgot right now. :) Anyway, I've uploaded both my .exe and the signed .mar file I used, so folks can look at that independently from me and building own bundles.

https://people.torproject.org/~gk/testbuilds/31567_3.exe
https://people.torproject.org/~gk/testbuilds/31567_3.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

comment:31 in reply to:  30 Changed 7 weeks ago by mcs

Replying to gk:

I see. Well, it could be that Martin's patch opened a new hole while closing the previous bug. Or it could still be an issue with my setup. Or it could be related to me not using a full update setup but rather just your script. However, I somehow doubt the latter at least as using the updater executable from 9.0a4 + the newly created .mar file works. Additionally, it could be that you replacing the format specifiers killed that second, different bug as well. And there are probably some more explanations I forgot right now. :) Anyway, I've uploaded both my .exe and the signed .mar file I used, so folks can look at that independently from me and building own bundles.

https://people.torproject.org/~gk/testbuilds/31567_3.exe
https://people.torproject.org/~gk/testbuilds/31567_3.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

Here is what I have learned so far:
1) Using my own build (with Martin's patch + my own signing certificate + my own mar file + my own update server) worked when I did the update interactively within Firefox. That's good news.

2) When I tried again using my updater-test.cmd script with my same build and mar file I saw the UPDATE_SETTINGS_FILE_CHANNEL error. Next I added some logging and saw that the path for update-settings.ini was missing the /Browser path component at the end. When I fixed my test script by appending /Browser toINSTALLDIR (and to correctly copy the mar file to update.mar) it worked! My script now looks similar to the following:

set MARFILE=tor-browser-win64-tbb-nightly_en-US.mar
set INSTALLDIR=C:\Users\USER\Desktop\tbtest\Browser
set UPDATEDIR=%INSTALLDIR%\TorBrowser\UpdateInfo\updates\0

mkdir %UPDATEDIR%
copy %MARFILE% %UPDATEDIR%\update.mar
pushd %INSTALLDIR%
updater %UPDATEDIR% %INSTALLDIR% %INSTALLDIR%
popd

3) The same script worked using the exe and mar file from comment:30.

My conclusion is that Martin's patches fix this bug.

And please accept my apology for wasting people's time by not getting the args right the first time for the manual/command line updater test!

comment:32 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201908R added; TorBrowserTeam201908 removed
Status: needs_revisionneeds_review

Good stuff, bug in my setup then. ;) I have a patch for review up on my bug_31567 branch (https://gitweb.torproject.org/user/gk/tor-browser-build.git/commit/?h=bug_31567&id=a479eeb424496b781d7ea9cf7b2d9a6404d7c32f).

comment:33 Changed 7 weeks ago by mcs

r=mcs
The patch looks good to me, and most importantly it seems to fix the problem. Maybe Richard should take a look at the mingw patches too, but I think we are good here.

comment:34 in reply to:  33 Changed 7 weeks ago by gk

Replying to mcs:

r=mcs
The patch looks good to me, and most importantly it seems to fix the problem. Maybe Richard should take a look at the mingw patches too, but I think we are good here.

Sounds good to me. I'll leave the ticket open to wait for Richard's feedback here but merged the fix to master (commit a479eeb424496b781d7ea9cf7b2d9a6404d7c32f) to have it in our next nightly builds (there aren't that many before the first alpha needs to get out).

comment:35 Changed 7 weeks ago by pospeselr

The mingw patches look to good to me.

comment:36 Changed 7 weeks ago by gk

Parent ID: #30322
Resolution: fixed
Status: needs_reviewclosed

Thanks!

comment:37 Changed 7 weeks ago by gk

Keywords: tbb-update added
Note: See TracTickets for help on using tickets.