Opened 3 years ago

Closed 19 months ago

Last modified 18 months ago

#14821 closed defect (fixed)

--enable-expensive-hardening broken with clang

Reported by: Sebastian Owned by:
Priority: High Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 027-backport, 2016-bug-retrospective
Cc: gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When passing -fsanitize=address, clang internally defines _FORTIFY_SOURCE=0. This conflicts with the _FORTIFY_SOURCE=2 we like to set beforehand, so we don't enable -fsanitize=address at all. Perhaps we should try to use fsanitize-address, if it fails try again with _FORTIFY_SOURCE remoeved, and only then resolve that it isn't available.

I am calling this 0.2.7 territory for now, but if the fix turns out to be easy we could conceivably backport it.

Child Tickets

Change History (14)

comment:2 Changed 3 years ago by Sebastian

Ok, my idea above was not a good one. We should prefer FORTIFY_SOURCE over AddressSanitizer. Will have to use gcc I guess unless there's a great idea for this somewhere

comment:3 Changed 3 years ago by nickm

Is FORTIFY_SOURCE really better than asan?

comment:4 Changed 3 years ago by Sebastian

According to the link it is, and I thought deferring to the asan people on this was smart.

comment:5 Changed 3 years ago by gk

Cc: gk added

comment:6 Changed 3 years ago by Sebastian

I'm starting to re-evaluate my opinion if we shouldn't just get rid of FORTIFY_SOURCE=2 if it breaks -fsanitize=address. I've recently patched my configure script to allow just that very frequently to track down a bunch of bugs. In production builds which don't use expensive hardening we still have the FORTIFY_SOURCE

comment:7 Changed 3 years ago by nickm

Status: newassigned

comment:8 Changed 2 years ago by nickm

Keywords: 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:9 Changed 2 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.???

Make all non-needs_review, non-needs_revision, 027-triaged-1-out items belong to 0.2.???

comment:10 Changed 21 months ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.8.x-final
Priority: MediumHigh
Severity: Normal

Given the bugginess of some of GCC's hardening options, I think, we should look harder at this.

comment:11 Changed 19 months ago by nickm

Keywords: 027-backport added; 027-triaged-1-out removed

ug, apparently clang 3.7 broke everybody by messing up autotools builds, so that clang only builds its sanitizers correctly with cmake. So first I had to fix that on my desktop.

Then I managed to figure out that the only compilation problems were related to strcmp() and to some parenthesis in test_util.

And then I realized that it's going to complain a lot about leaks in the tests unless I say ASAN_OPTIONS=detect_leaks=0 .

And I found a couple of actual bugs with the tests, so I'm going to open tickets for those. But for now, the branch bug14821 could be what we need here. The part of this not applying to test_util.c could be an 0.2.7 backport but I'm a bit nervous.

comment:12 Changed 19 months ago by nickm

Status: assignedneeds_review

comment:13 Changed 19 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Backported to 0.2.7 as 67e5d49d8a995c6d3b8bf4177046271a7d4dd157 ; part left in master as 1318c1611fed301f44d69a2d6e4f012efd94c9cc.

comment:14 Changed 18 months ago by nickm

Keywords: 2016-bug-retrospective added

Mark more tickets for severe bug retrospective, based on Priority and date and hand-inspection.

Note: See TracTickets for help on using tickets.