Opened 2 months ago

Closed 6 weeks ago

#27728 closed defect (fixed)

Signature mismatch for crypto_strongest_rand() in ed-donna code breaks -flto compilation

Reported by: Hello71 Owned by: nickm
Priority: Very High Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: regression, 035-must
Cc: haxxpop Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

./src/lib/crypt_ops/crypto_rand.h:24:1: error: variable ‘crypto_strongest_rand’ redeclared as function
 MOCK_DECL(void,crypto_strongest_rand,(uint8_t *out, size_t out_len));
 ^
./src/lib/crypt_ops/crypto_rand.h:24:1: error: variable ‘crypto_strongest_rand’ redeclared as function
src/lib/crypt_ops/crypto_rand.c:338:1: note: previously declared here
 MOCK_IMPL(void,
 ^

Child Tickets

Change History (16)

comment:1 Changed 2 months ago by nickm

Milestone: Tor: 0.3.5.x-final

comment:2 Changed 2 months ago by nickm

Component: - Select a componentCore Tor/Tor

version, compiler, etc? This builds fine for me.

Also, I don't understand why the compiler thinks that line 24 of crypto_rand.h is happening *after* line 338 of crypto_rand.c.

comment:3 Changed 2 months ago by Hello71

tor 035166e (master as of about 6 hours ago), gcc 8.2.1 with -O3 -flto and other flags. I assume the order is across files due to the LTO.

It looks like src/ext/ed25519/ref10/src_ext_ed25519_ref10_libed25519_ref10_a-keypair.o and src/ext/ed25519/donna/src_ext_ed25519_donna_libed25519_donna_a-ed25519_tor.o are built without TOR_UNIT_TESTS, so they declare crypto_strongest_rand as a function instead of a variable. I checked this by putting #ifndef TOR_UNIT_TESTS #error bad include #endif in src/lib/crypt_ops/crypto_rand.h and then remaking src/test/test, relying on automake's dependency tracking.

Last edited 2 months ago by Hello71 (previous) (diff)

comment:4 Changed 2 months ago by Hello71

gcc 8.2.1+20180831-1

comment:5 Changed 8 weeks ago by Hello71

specifically, steps to reproduce:

  1. install gcc, maybe you need 8.2.1+20180831-1 (I use Arch, fyi), probably 8.2.0 or even earlier will break too
  1. configure tor git master with CFLAGS='-O3 -flto' LDFLAGS=-flto (probably -O2 works too, but I only tried -O3)
  1. make src/test/test

alternatively:

  1. insert:
    #ifndef TOR_UNIT_TESTS
    #error bad include
    #endif
    

in src/lib/crypt_ops/crypto_rand.h.

  1. make src/test/test.

because only the test executable is being built, all object files must define crypto_strongest_rand as the same type. this requires that src/lib/crypt_ops/crypto_rand.h must always be included with TOR_UNIT_TESTS (or without, but we are forcing only tests to be built here)

comment:6 Changed 8 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:7 Changed 8 weeks ago by nickm

Summary: test: Build an HSv3 descriptor with authorized client is apparently brokenSignature mismatch for crypto_strongest_rand() in ed-donna code breaks -flto compilation

comment:8 Changed 8 weeks ago by nickm

Status: acceptedneeds_review

Okay -- I've had to install GCC 8.2 locally, and after a bit I've successfully reproduced this. So I don't forget, the configuration line I used was:

./configure CC=/opt/gcc-8.2/bin/gcc CFLAGS='-O3 -flto' LDFLAGS='-flto' \
    AR=/opt/gcc-8.2/bin/gcc-ar \
    RANLIB=/opt/gcc-8.2/bin/gcc-ranlib \
    NM=/opt/gcc-8.2/gcc-nm

I've added a simple fix as branch bug27728, with PR at https://github.com/torproject/tor/pull/354 .

I'm seeing a few other GCC 8.2 warnings on the source code -- I'll try to check whether they're LTO only.

comment:9 Changed 8 weeks ago by nickm

Keywords: regression added

comment:10 Changed 8 weeks ago by nickm

The other warnings seem to be LTO-only. I'll look to see if any of them are troubling.

comment:11 Changed 8 weeks ago by nickm

Opened #27772 for the other warnings

comment:12 Changed 7 weeks ago by dgoulet

Cc: dgoulet removed
Reviewer: dgoulet

comment:13 Changed 7 weeks ago by dgoulet

Status: needs_reviewmerge_ready

lgtm; Travis is also happy.

comment:14 Changed 6 weeks ago by nickm

Keywords: 035-must added

Add the 035-must tag to some assertion failures, hangs, ci-blockers, etc.

comment:15 Changed 6 weeks ago by nickm

Priority: MediumVery High

Mark all 035-must tickets as "very high"

comment:16 Changed 6 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.