Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#23442 closed task (fixed)

Error building firefox for Windows 64 in security/pkix/lib/pkixnames.cpp

Reported by: boklm Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201710R
Cc: tbb-team Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We are getting the following error when trying to build firefox for Windows 64:

/var/tmp/dist/mingw-w64/helpers/x86_64-w64-mingw32-g++ -std=gnu++11 -mwindows -o pkixnames.o -c   -DNDEBUG=1 -DTRIMMED=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/var/tmp/build/firefox-89417fed09e5/security/pkix -I/var/tmp/build/firefox-89417fed09e5/obj-mingw/security/pkix -I/var/tmp/build/firefox-89417fed09e5/security/pkix/include -I/var/tmp/build/firefox-89417fed09e5/obj-mingw/dist/include  -I/var/tmp/build/firefox-89417fed09e5/obj-mingw/dist/include/nspr -I/var/tmp/build/firefox-89417fed09e5/obj-mingw/dist/include/nss         -DMOZILLA_CLIENT -include /var/tmp/build/firefox-89417fed09e5/obj-mingw/mozilla-config.h -MD -MP -MF .deps/pkixnames.o.pp  -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wc++14-compat -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-format -fno-lifetime-dse -fno-exceptions -fno-strict-aliasing -mms-bitfields -fno-rtti -fno-exceptions -fno-math-errno -pipe  -g -O -fomit-frame-pointer  -Wall -Wextra -pedantic-errors -Wno-error=shadow  /var/tmp/build/firefox-89417fed09e5/security/pkix/lib/pkixnames.cpp
/var/tmp/build/firefox-89417fed09e5/security/pkix/lib/pkixnames.cpp: In function 'bool mozilla::pkix::{anonymous}::FinishIPv6Address(uint8_t (&)[16], int, int)':
/var/tmp/build/firefox-89417fed09e5/security/pkix/lib/pkixnames.cpp:1712:32: error: 'memmove' was not declared in this scope
           componentsToMove * 2u);
                                ^
make[5]: *** [pkixnames.o] Error 1
make[5]: Leaving directory `/var/tmp/build/firefox-89417fed09e5/obj-mingw/security/pkix'

Child Tickets

Change History (10)

comment:1 Changed 2 years ago by gk

Looks like https://bugzilla.mozilla.org/show_bug.cgi?id=1305396 is showing its ugly head again?

comment:3 Changed 2 years ago by boklm

Adding an #include <cstring> to pkixnames.cpp is fixing the build issue:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_20636_v5&id=f7826cf2476406e668b049006c154374d546ab91

But maybe it can be fixed in the same way as https://bugzilla.mozilla.org/show_bug.cgi?id=1199624

However I'm wondering why we don't have the same issue for x86 builds.

comment:4 in reply to:  3 Changed 2 years ago by cypherpunks

Replying to boklm:

Adding an #include <cstring> to pkixnames.cpp is fixing the build issue:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_20636_v5&id=f7826cf2476406e668b049006c154374d546ab91

Not fixing. It's not even a workaround.
"The proper fix needs to be consistent with the fix for bug 1189891: change the code to use std::equals and similar instead of mem*, and remove all #include <cstring>." Because of https://bugzilla.mozilla.org/show_bug.cgi?id=1189891#c0 and other funny things.

But maybe it can be fixed in the same way as https://bugzilla.mozilla.org/show_bug.cgi?id=1199624

It should have been fixed there "for memcmp/memmove/memset functions".
Also 2 occurrences of memcpy in https://dxr.mozilla.org/mozilla-esr52/source/security/manager/ssl/SSLServerCertVerification.cpp#1007 should be fixed in the same way.

However I'm wondering why we don't have the same issue for x86 builds.

A lot of reasons why mem* were declared there, but all of them were bugs.

comment:5 Changed 2 years ago by gk

Keywords: TorBrowserTeam201710 added; TorBrowserTeam201709 removed

Items for October 2017

comment:6 Changed 2 years ago by boklm

Keywords: TorBrowserTeam201710R added; TorBrowserTeam201710 removed
Status: newneeds_review

This patch has been merged upstream, and apply cleanly to our branch:
https://hg.mozilla.org/mozilla-central/raw-rev/0a5c3789fac1

comment:7 Changed 2 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Cherry-picked onto tor-browser-52.4.1.esr-7.5-1 (commit e6cd07862cf0de06b9cc7c091f9d2dd32cd76280), thanks.

comment:8 Changed 2 years ago by tom

Parent ID: #23229

Replying to cypherpunks:

Replying to boklm:

Adding an #include <cstring> to pkixnames.cpp is fixing the build issue:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_20636_v5&id=f7826cf2476406e668b049006c154374d546ab91

Not fixing. It's not even a workaround.
"The proper fix needs to be consistent with the fix for bug 1189891: change the code to use std::equals and similar instead of mem*, and remove all #include <cstring>." Because of https://bugzilla.mozilla.org/show_bug.cgi?id=1189891#c0 and other funny things.

But maybe it can be fixed in the same way as https://bugzilla.mozilla.org/show_bug.cgi?id=1199624

It should have been fixed there "for memcmp/memmove/memset functions".
Also 2 occurrences of memcpy in https://dxr.mozilla.org/mozilla-esr52/source/security/manager/ssl/SSLServerCertVerification.cpp#1007 should be fixed in the same way.

However I'm wondering why we don't have the same issue for x86 builds.

A lot of reasons why mem* were declared there, but all of them were bugs.

I spoke with Keeler about this. From his recollection there were no security concerns with the changes, it was just toolchain weirdness. He guesses that it was mostly a coincidence that we had to make those changes in security/pkix but not security/manager.

If there's no build that failing with it now he doesn't see a strong reason to move to std::copy, etc., but he is very concern about our cert verifier failing, and asked if they have testcases or steps to reproduce.

comment:9 Changed 2 years ago by tom

I can't fix the parent id, it won't let me add it back (and I couldn't comment without removing it.)

It was #23229

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

Replying to tom:

Replying to cypherpunks:

Replying to boklm:

Adding an #include <cstring> to pkixnames.cpp is fixing the build issue:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_20636_v5&id=f7826cf2476406e668b049006c154374d546ab91

Not fixing. It's not even a workaround.
"The proper fix needs to be consistent with the fix for bug 1189891: change the code to use std::equals and similar instead of mem*, and remove all #include <cstring>." Because of https://bugzilla.mozilla.org/show_bug.cgi?id=1189891#c0 and other funny things.

But maybe it can be fixed in the same way as https://bugzilla.mozilla.org/show_bug.cgi?id=1199624

It should have been fixed there "for memcmp/memmove/memset functions".
Also 2 occurrences of memcpy in https://dxr.mozilla.org/mozilla-esr52/source/security/manager/ssl/SSLServerCertVerification.cpp#1007 should be fixed in the same way.

However I'm wondering why we don't have the same issue for x86 builds.

A lot of reasons why mem* were declared there, but all of them were bugs.

I spoke with Keeler about this. From his recollection there were no security concerns with the changes, it was just toolchain weirdness. He guesses that it was mostly a coincidence that we had to make those changes in security/pkix but not security/manager.

If there's no build that failing with it now he doesn't see a strong reason to move to std::copy, etc., but he is very concern about our cert verifier failing, and asked if they have testcases or steps to reproduce.

That's why I asked you to talk to a person with enough competence. There's no coincidence in this case. Did you show him a citation from my comment? It's from https://bugzilla.mozilla.org/show_bug.cgi?id=1199624#c1 by Brian Smith. And if you "remove all #include <cstring>" from https://dxr.mozilla.org/mozilla-esr52/rev/f9df5238dca13e40b8128faba317df25e2f69249/security/manager/ssl/SSLServerCertVerification.cpp#97 first, then you "see a strong reason to move to std::copy, etc." So, now it's easier to fix this bug instead of doing useless testing. Thanks.

Note: See TracTickets for help on using tickets.