#25862 closed enhancement (fixed)

Clean up wrapper script/CFLAGS and friends mix on Windows

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

Description (last modified by gk)

Our mix of wrapper scripts and usage of CFLAGS/LDFLAGS when compiling on Windows has grown over the years to a point where it gets confusing. We should get back to sanity here.

We needed the wrapper scripts to pass down all required flags to the depths of the Firefox build system, but meanwhile those scripts are used almost everywhere and CFLAGS/LDFLAGS additionally sometimes.

One option boklm brought up would be to use the CFLAGS/LDFLAGS defined in rbm.conf and enable those scripts only during Firefox's build.

Another just get rid of those wrapper scripts and fix the missing pieces in Firefox if those are still there.

At any rate, we should document where we need them in the first place to avoid lengthy digging in case someone is wondering in the future or now, while looking at our build scripts.

Child Tickets

Change History (13)

comment:1 Changed 20 months ago by gk

Description: modified (diff)

FWIW, I came up with this ticket because I hit link failures when building the rust std lib for Windows (while working on #25849). It turns out that the wrappers compile libbacktrace and jemalloc with stack protections support, but the linker when building the std lib is not regarding -lssp (see linker arg construction in https://github.com/rust-lang/rust/blob/1.25.0/src/librustc_back/target/windows_base.rs).

comment:2 Changed 20 months ago by boklm

I started working on this in branch bug_25862:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_25862&id=0bb676a92557784087370cf8f914dd33e476c2ad

I have been able to finish a build using this branch, however I did not check yet if it is reproducible on a 2nd machine, and if the hardening of the binaries is still good.

comment:3 in reply to:  2 Changed 20 months ago by boklm

Keywords: boklm201804 TorBrowserTeam201804R added
Status: newneeds_review

Replying to boklm:

I have been able to finish a build using this branch, however I did not check yet if it is reproducible on a 2nd machine, and if the hardening of the binaries is still good.

I have been able to reproduce this build on a 2nd machine. However, running the testsuite on this build, the win_DEP_ASLR test is failing on the firefox binaries. It seems to be caused by not having the linker flags in the g++ and gcc wrappers. A build with this fixup commit is fixing the issue:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_25862&id=683c06c8347860cd8dcf8acc4bd7d36a2d46b9f8

A new version of the commit, including the fixup, is in branch bug_25862_v2:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_25862_v2&id=34522dfd5345bc65794402b988dfd298620df0d1

comment:4 Changed 20 months ago by gk

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201804R removed

Moving review tickets to May.

comment:5 Changed 20 months ago by boklm

Keywords: boklm201805 added; boklm201804 removed

boklm201804 -> boklm201805

comment:7 Changed 19 months ago by gk

Description: modified (diff)

comment:8 Changed 19 months ago by gk

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201805R removed
Status: needs_reviewneeds_revision

Looks good to me. One thing we should change is:

-fstack-protector-all -Wstack-protector --param ssp-buffer-size=4

in the CFLAGS in rbm.conf. This line makes not much sense to me as there is no need to specify the buffer size if you are adding a canary to all functions. Mozilla is using -fstack-protector-strong and I believe we are doing the same for our Linux builds. Thus, let's go with that one for Windows, too?

comment:9 Changed 19 months ago by boklm

I pushed a new version of this patch in branch bug_25862_v4, using -fstack-protector-strong in the CFLAGS in rbm.conf:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_25862_v4&id=4e5b936e776b45093ca33dfd72690245bf3c68f5

comment:10 Changed 19 months ago by boklm

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201805 removed
Status: needs_revisionneeds_review

comment:11 Changed 19 months ago by gk

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201805R removed
Status: needs_reviewneeds_revision

I think we should get rid of the --param related part as well (sorry for being not clear enough), see: https://wiki.debian.org/Hardening#DEB_BUILD_HARDENING_STACKPROTECTOR_.28gcc.2Fg.2B-.2B-_-fstack-protector-strong.29

comment:12 Changed 19 months ago by boklm

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201805 removed
Status: needs_revisionneeds_review

I pushed a new version of the patch in branch bug_25862_v5, removing the --param option:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_25862_v5&id=8fd3d1dc7f27a96129328af98b69e18d161cef97

I also removed the -Wstack-protector flag as we will probably want to ignore warnings about functions without stack protector.

comment:13 Changed 19 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good and merged to master (commit 8fd3d1dc7f27a96129328af98b69e18d161cef97).

Note: See TracTickets for help on using tickets.