Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19161 closed defect (fixed)

test for libscrypt_scrypt() fails

Reported by: isis Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: TorCoreTeam201605
Cc: isis Actual Points: 0.2
Parent ID: Points: 0
Reviewer: isis Sponsor:

Description

On a Debian jessie system, possibly due libscrypt.so thinking that the "log" function is undefined. (Running nm -D `locate libscrypt.so.0` confirms this.)

There's a config.log which shows this happening attached. (See line 4311.)

Nick thinks we need to add -lm to the compiler flags for the configure test.

Child Tickets

Attachments (3)

config.log.tar.xz (21.6 KB) - added by isis 3 years ago.
config.log.1.tar.xz (22.6 KB) - added by isis 3 years ago.
config.log.2.tar.xz (22.5 KB) - added by isis 3 years ago.

Download all attachments as: .zip

Change History (21)

Changed 3 years ago by isis

Attachment: config.log.tar.xz added

comment:1 Changed 3 years ago by isis

Milestone: Tor: 0.2.8.x-final

comment:2 Changed 3 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 3 years ago by nickm

[And it looks like the -lm is missing because of "pow" being builtin...]

/usr/bin/gcc-4.9.real -fstack-protector-strong -fPIE -pie -D_FORTIFY_SOURCE=2 -Wformat -Wformat-security -Werror=format-security -std=gnu99 -o conftest -g -O2 -I${top_srcdir}/src/common conftest.c -lm -lpthread -ldl
conftest.c:69:6: warning: conflicting types for built-in function 'pow'
 char pow ();
      ^

comment:4 Changed 3 years ago by nickm

Status: acceptedneeds_review

Does bug19161_028 fix this for you? It's a one-liner.

comment:5 Changed 3 years ago by nickm

Actual Points: 0
Points: 0

(This took 10 minutes.)

comment:6 Changed 3 years ago by isis

Keywords: TorCoreTeam201605 added
Reviewer: isis

comment:7 Changed 3 years ago by cypherpunks

One nitpick, use quotation (using [ and ]) for all of the parameters (including the empty ones).

comment:8 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

isis says this fails, with failures to find types for uintfoo_t and void*. Usually that means that we've added some compiler flag that stops building or linking entirely. Waiting on a config.log analysis to see what the issue was.

Changed 3 years ago by isis

Attachment: config.log.1.tar.xz added

comment:9 in reply to:  8 Changed 3 years ago by isis

Replying to nickm:

isis says this fails, with failures to find types for uintfoo_t and void*. Usually that means that we've added some compiler flag that stops building or linking entirely. Waiting on a config.log analysis to see what the issue was.


New config.log attached.

comment:10 Changed 3 years ago by nickm

uggggggggggggggg. starting around line 7617 is where we see the problem: linking doesn't work because we try to include '-libscrypt' but not '-lm'.

bug19161_028 now has an extra fix to search for libm even harder.

Changed 3 years ago by isis

Attachment: config.log.2.tar.xz added

comment:11 in reply to:  10 ; Changed 3 years ago by isis

Replying to nickm:

uggggggggggggggg. starting around line 7617 is where we see the problem: linking doesn't work because we try to include '-libscrypt' but not '-lm'.

bug19161_028 now has an extra fix to search for libm even harder.


All the AC_CHECK_SIZEOF checks are still failing due to not adding -lm, see attached config.log.2.tar.xz.

comment:12 Changed 3 years ago by nickm

bug19161_028_v2 is far simpler.

comment:13 in reply to:  12 Changed 3 years ago by isis

Replying to nickm:

bug19161_028_v2 is far simpler.


Okay, that works fine for me (although obviously doesn't link in libscrypt or try to use it).

comment:14 Changed 3 years ago by nickm

Actual Points: 00.1
Status: needs_revisionneeds_review

Okay. Let's get it reviewed then.

comment:15 in reply to:  11 Changed 3 years ago by isis

Replying to isis:

Replying to nickm:

uggggggggggggggg. starting around line 7617 is where we see the problem: linking doesn't work because we try to include '-libscrypt' but not '-lm'.

bug19161_028 now has an extra fix to search for libm even harder.


All the AC_CHECK_SIZEOF checks are still failing due to not adding -lm, see attached config.log.2.tar.xz.


And fourteen pages of GNU Autoconf documentation later… I think I have a patch which fixes this so that libscript is linked in when available, and when it is linked in, if we do need -lm, then -lm is also added where needed. It's available in my bug19161_028 branch, which is based on nickm's branch of the same name.

My patch works for me, but it'd be nice to know that it doesn't break things for anyone else.

comment:16 Changed 3 years ago by nickm

Current plan: try to do my bug19161_028_v2 in 0.2.8 (since it's likely to require fewer rounds of break-and-test-and-fix), and something based on isis's bug19161_028 in 0.2.9 (since it's better).

comment:17 Changed 3 years ago by nickm

Actual Points: 0.10.2
Resolution: fixed
Status: needs_reviewclosed

Merging bug19161_028_v2 to 0.2.8. For 0.2.9, let's look more at isis's bug19161_028. I worry that as written it will add -lm even in places where it isn't needed. Also, the current approach adds -lscrypt in places where it isn't needed, which isn't good either. I'll make a separate ticket for that, however, so we can close this.

comment:18 Changed 3 years ago by nickm

I'll make a separate ticket for that, however, so we can close this.

Done as #19174

Note: See TracTickets for help on using tickets.