Opened 2 years ago

Closed 22 months ago

Last modified 14 months ago

#23663 closed defect (invalid)

ESR52 codebase is incompatible with anything below Universal C Runtime (CRT) in Windows

Reported by: cypherpunks Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: tbb-security
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

First, Mozilla fixed VS2015 compilation https://hg.mozilla.org/mozilla-central/rev/e597b8795fbe
Second, Jacek "fixed" ffvpx compilation on mingw. And it became incompatible with UCRT on mingw (Jacek, don't do that again!) https://hg.mozilla.org/mozilla-central/rev/5680a55b2ec1
Third, Mozilla dropped support for building with Visual Studio 2013 https://bugzilla.mozilla.org/show_bug.cgi?id=1186064 in Firefox 50. But not only that! They started vigorously removing all their patches for it! https://bugzilla.mozilla.org/show_bug.cgi?id=1277155 and almost all other compatibility shims/workarounds were removed before ESR52.

Child Tickets

Change History (12)

comment:1 Changed 2 years ago by gk

Status: newneeds_information

Could you elaborate what this bug is about? I have a hard time understanding the problem given that Tor Browser based on esr52 is working fine with the msvcr100.dll we ship.

comment:2 Changed 2 years ago by cypherpunks

Status: needs_informationnew

Are you sure? Did you see what was removed in the last bug? Citing:

-    /* we use n - 1 here because if the buffer is not big enough, the MS
-     * runtime libraries don't add a terminating zero at the end. MSDN
-     * recommends to provide _snprintf/_vsnprintf() a buffer size that
-     * is one less than the actual buffer, and zero it before calling
-     * _snprintf/_vsnprintf() to workaround this problem.
-     * See http://msdn.microsoft.com/en-us/library/1kt27hek(v=vs.80).aspx */

Now you use them without workarounds and who knows what else...

comment:3 in reply to:  2 Changed 2 years ago by gk

Status: newneeds_information

Replying to cypherpunks:

Are you sure? Did you see what was removed in the last bug? Citing:

-    /* we use n - 1 here because if the buffer is not big enough, the MS
-     * runtime libraries don't add a terminating zero at the end. MSDN
-     * recommends to provide _snprintf/_vsnprintf() a buffer size that
-     * is one less than the actual buffer, and zero it before calling
-     * _snprintf/_vsnprintf() to workaround this problem.
-     * See http://msdn.microsoft.com/en-us/library/1kt27hek(v=vs.80).aspx */

Now you use them without workarounds and who knows what else...

Well, basically all of that which you are pointing to was behing _MSC_VER which is not defined in our builds anyway. Thus, we are/were not affected by that. Or am I missing something?

comment:4 Changed 2 years ago by cypherpunks

Status: needs_informationnew

Don't you see that Jacek's patch activated compat shims for mingw? They were removed later as useless for UCRT (but needed for <= msvcr120.dll!).

comment:5 in reply to:  4 ; Changed 2 years ago by gk

Status: newneeds_information

Replying to cypherpunks:

Don't you see that Jacek's patch activated compat shims for mingw? They were removed later as useless for UCRT (but needed for <= msvcr120.dll!).

Oh, okay. You are just concerned about https://hg.mozilla.org/mozilla-central/rev/5680a55b2ec1? I thought about cases in the other patches as well as you posted them in the description. But as I said they are guarded by _MSC_VER defines which are not used by mingw-w64 anyway.

So it seems

-if CONFIG['OS_ARCH'] == 'WINNT':
-    SOURCES += [
-        '../compat/strtod.c'

is the thing that is bothering you. Back then this got introduced to fix compilation with mingw-w64. But that's not an issue anymore without this particular code. So, what exactly is the problem with that removal for our mingw-w64 builds as they are building fine now? And could you point to the security problematic that you think is obvious with removing those three code lines? (the one you mentioned in comment:2 does not seem to be it)

comment:6 in reply to:  5 ; Changed 2 years ago by cypherpunks

Status: needs_informationnew

Replying to gk:

Replying to cypherpunks:

Don't you see that Jacek's patch activated compat shims for mingw? They were removed later as useless for UCRT (but needed for <= msvcr120.dll!).

Oh, okay. You are just concerned about https://hg.mozilla.org/mozilla-central/rev/5680a55b2ec1?

Of course, no.

I thought about cases in the other patches as well as you posted them in the description. But as I said they are guarded by _MSC_VER defines which are not used by mingw-w64 anyway.

But they should have been adapted to mingw where it's about CRT bugs.

So it seems

-if CONFIG['OS_ARCH'] == 'WINNT':
-    SOURCES += [
-        '../compat/strtod.c'

is the thing that is bothering you. Back then this got introduced to fix compilation with mingw-w64. But that's not an issue anymore without this particular code.

They, probably, don't use CRT then.

So, what exactly is the problem with that removal for our mingw-w64 builds as they are building fine now?

Building fine, but working?

And could you point to the security problematic that you think is obvious with removing those three code lines? (the one you mentioned in comment:2 does not seem to be it)

No, the security problematic is that ESR52 was never tested with anything below UCRT and in general:

It makes it very expensive for us to fix bugs in already-released versions of the libraries because we are no longer actively working in the codebases for those versions, so fixes must be individually backported and tested. The result is that we usually fix only serious security vulnerabilities in old versions of the libraries. Other bugs are generally fixed only for the next major version. (M$)

comment:7 in reply to:  6 ; Changed 2 years ago by gk

Status: newneeds_information

Replying to cypherpunks:

Replying to gk:

Replying to cypherpunks:

Don't you see that Jacek's patch activated compat shims for mingw? They were removed later as useless for UCRT (but needed for <= msvcr120.dll!).

Oh, okay. You are just concerned about https://hg.mozilla.org/mozilla-central/rev/5680a55b2ec1?

Of course, no.

I thought about cases in the other patches as well as you posted them in the description. But as I said they are guarded by _MSC_VER defines which are not used by mingw-w64 anyway.

But they should have been adapted to mingw where it's about CRT bugs.

Why? Removing those patches does not change anything with respect to mingw-w64. Those code parts did not get used for it before code removal either.

So it seems

-if CONFIG['OS_ARCH'] == 'WINNT':
-    SOURCES += [
-        '../compat/strtod.c'

is the thing that is bothering you. Back then this got introduced to fix compilation with mingw-w64. But that's not an issue anymore without this particular code.

They, probably, don't use CRT then.

So, what exactly is the problem with that removal for our mingw-w64 builds as they are building fine now?

Building fine, but working?

What is not working due to those code changes?

And could you point to the security problematic that you think is obvious with removing those three code lines? (the one you mentioned in comment:2 does not seem to be it)

No, the security problematic is that ESR52 was never tested with anything below UCRT and in general:

It was, we shipped alpha releases before we switched Tor Browser stable users to ESR 52.

It makes it very expensive for us to fix bugs in already-released versions of the libraries because we are no longer actively working in the codebases for those versions, so fixes must be individually backported and tested. The result is that we usually fix only serious security vulnerabilities in old versions of the libraries. Other bugs are generally fixed only for the next major version. (M$)

Where is this quote from?

comment:8 in reply to:  7 ; Changed 2 years ago by cypherpunks

Status: needs_informationnew

Replying to gk:

Replying to cypherpunks:

Replying to gk:

Replying to cypherpunks:

Don't you see that Jacek's patch activated compat shims for mingw? They were removed later as useless for UCRT (but needed for <= msvcr120.dll!).

Oh, okay. You are just concerned about https://hg.mozilla.org/mozilla-central/rev/5680a55b2ec1?

Of course, no.

I thought about cases in the other patches as well as you posted them in the description. But as I said they are guarded by _MSC_VER defines which are not used by mingw-w64 anyway.

But they should have been adapted to mingw where it's about CRT bugs.

Why? Removing those patches does not change anything with respect to mingw-w64. Those code parts did not get used for it before code removal either.

Because you're using CRT, obviously. Patches for MSVC don't change anything, but for CRT do, e.g. https://hg.mozilla.org/mozilla-central/rev/398f38361dc2#l10.10

So it seems

-if CONFIG['OS_ARCH'] == 'WINNT':
-    SOURCES += [
-        '../compat/strtod.c'

is the thing that is bothering you. Back then this got introduced to fix compilation with mingw-w64. But that's not an issue anymore without this particular code.

They, probably, don't use CRT then.

So, what exactly is the problem with that removal for our mingw-w64 builds as they are building fine now?

Building fine, but working?

What is not working due to those code changes?

Depends on whether the used implementation is correct.

And could you point to the security problematic that you think is obvious with removing those three code lines? (the one you mentioned in comment:2 does not seem to be it)

No, the security problematic is that ESR52 was never tested with anything below UCRT and in general:

It was, we shipped alpha releases before we switched Tor Browser stable users to ESR 52.

By Mozilla, was meant.

It makes it very expensive for us to fix bugs in already-released versions of the libraries because we are no longer actively working in the codebases for those versions, so fixes must be individually backported and tested. The result is that we usually fix only serious security vulnerabilities in old versions of the libraries. Other bugs are generally fixed only for the next major version. (M$)

Where is this quote from?

https://blogs.msdn.microsoft.com/vcblog/2014/06/10/the-great-c-runtime-crt-refactoring/

comment:9 in reply to:  8 Changed 2 years ago by gk

Resolution: invalid
Status: newclosed

Replying to cypherpunks:

Replying to gk:

Replying to cypherpunks:

Replying to gk:

Replying to cypherpunks:

Don't you see that Jacek's patch activated compat shims for mingw? They were removed later as useless for UCRT (but needed for <= msvcr120.dll!).

Oh, okay. You are just concerned about https://hg.mozilla.org/mozilla-central/rev/5680a55b2ec1?

Of course, no.

I thought about cases in the other patches as well as you posted them in the description. But as I said they are guarded by _MSC_VER defines which are not used by mingw-w64 anyway.

But they should have been adapted to mingw where it's about CRT bugs.

Why? Removing those patches does not change anything with respect to mingw-w64. Those code parts did not get used for it before code removal either.

Because you're using CRT, obviously. Patches for MSVC don't change anything, but for CRT do, e.g. https://hg.mozilla.org/mozilla-central/rev/398f38361dc2#l10.10

That one does not affect us either. We don't use jemalloc in mingw-w64 Windows builds and MOZ_CRT is not defined either. Thus, removing this part is not mingw-w64 related.

So, if you have concrete things that are bugs and are affecting us, please report them. I think the ticket as-is is invalid.

comment:10 Changed 22 months ago by cypherpunks

Keywords: ff59-esr added
Resolution: invalid
Status: closedreopened

As you don't care it is required for security, it is a MUST since ff57 https://bugzilla.mozilla.org/show_bug.cgi?id=1380609

comment:11 in reply to:  10 Changed 22 months ago by gk

Keywords: ff59-esr removed
Resolution: invalid
Status: reopenedclosed

Replying to cypherpunks:

As you don't care it is required for security, it is a MUST since ff57 https://bugzilla.mozilla.org/show_bug.cgi?id=1380609

I don't think so. We got Firefox nightly builds going with basically our toolchain and https://bugzilla.mozilla.org/show_bug.cgi?id=1380609 did not cause any bustage.

comment:12 Changed 14 months ago by crs

Oh, that's why 8.0a9 crashes on shutdown!
https://hg.mozilla.org/mozilla-central/rev/73f1010eea4c

Note: See TracTickets for help on using tickets.