Define SKIP_GNU token when building extension (Fixes FreeBSD >= 12) #189

adam12 · 2019-01-06T01:26:00Z

Defining the SKIP_GNU token as a compile flag forces the build of crypt_blowfish not to attempt redefinition of crypt and crypt_r which possibly conflict with the system libc depending on their function definition. This allows bcrypt-ruby to build again on FreeBSD >= 12.0.

I spent a bunch of time debugging the real reason for why this occurred:

The fine FreeBSD folk just patched out the conflicting function definitions from the vendored C source.

Solardiz suggested that bcrypt-ruby was interacting with crypt_blowfish incorrectly, which is entirely possible (I'll humbly defer to Solar Designer here) but I didn't see how/why during my investigation.

Compiling bcrypt_blowfish independently works fine on FreeBSD 12. It only fails when linked against anything that's included unistd.h.

bcrypt-ruby doesn't use crypt or crypt_r so I think we're safe defining this token and building without those function definitions. The only downside I see is we're now dependent on this token which might be internal only. I don't see it disappearing so I suspect the risk is minimal.

Openwall's crypt_blowfish library supports patching libc implementations. Part of this includes function definitions for `crypt` and `crypt_r`. Unfortunately, the function definition for `crypt_r` differs on FreeBSD >= 12 (and possibly others), which causes the extension to fail in building. This is due to the nature of how the extension works, where a C->Ruby extension is used, instead of just using a FFI such as Fiddle directly against `crypt_blowfish` and `crypt_gensalt`. The C->Ruby extension includes `ruby.h`, which in turn has a requirement on `unistd.h`, where the `crypt_r` function has already been defined, albeit with the different function signature. Defining the __SKIP_GNU token causes the `ow-crypt.h` header not to attempt defining(redefining) these two function definitions that may conflict with libc. We're not using either of them (only the re-entrant versions) so no harm done by skipping their definitions.

nanaya · 2019-01-07T18:10:36Z

From what I gather, the core (bcrypt) functionality is in crypt_blowfish.{c,h} (plus x86.S) and the rest are meant for usage as libc.

adam12 · 2019-01-07T18:27:05Z

I wonder if it's the inclusion of wrapper.c in the ext folder that's making it look like the libc wrapper code is being used?

In any case, both wrapper.o from $objs in extconf.rb and wrapper.c the file can be safely deleted from ext/ and the project will still build fine (with my patch).

nanaya · 2019-01-07T18:46:14Z

I couldn't get it to compile with that (maybe I did something wrong).

I think it's just system libc (included by ruby.h) and ow-crypt.h aren't supposed to be included together because the latter supposedly functions to fulfill the lack of relevant functions in the former.

I made two files (or whatever the correct term is) which proxy the required functions to ow-crypt.h and it seems to work fine as well (as in it passes the test).

adam12 · 2019-01-07T18:53:24Z

I made two files (or whatever the correct term is) which proxy the required functions to ow-crypt.h and it seems to work fine as well (as in it passes the test).

I tested your patch on FreeBSD 12 and it indeed works.

I couldn't get it to compile with that (maybe I did something wrong).

With my patch on FreeBSD 12?

I think it's just system libc (included by ruby.h) and ow-crypt.h aren't supposed to be included together because the latter supposedly functions to fulfill the lack of relevant functions in the former.

That makes a bit more sense, tho I could of swore that ow-crypt.h was the header meant to be used in applications (per README). It would seem to me that applications would be expected to be linked to libc.

nanaya · 2019-01-07T19:05:27Z

With my patch on FreeBSD 12?

The patch + removing wrapper.c & co.

That makes a bit more sense, tho I could of swore that ow-crypt.h was the header meant to be used in applications (per README). It would seem to me that applications would be expected to be linked to libc.

yeah I was confused as well but it kind of makes sense after a while.

adam12 · 2019-01-07T19:34:37Z

The patch + removing wrapper.c & co.

The patch on it's own should be sufficient. I just tested it again thinking I had maybe a false positive. I'd be curious to know if it is indeed busted and why.

Doubt it matters, but here's what I'm running:

$ freebsd-version -kru
12.0-RELEASE
12.0-RELEASE
12.0-RELEASE-p1

fliiiix · 2019-01-13T13:41:11Z

I just tried your patch works for me.

# freebsd-version -kru
12.0-RELEASE-p2
12.0-RELEASE-p2
12.0-RELEASE-p2

work/bcrypt-ruby $ ruby ./ext/mri/extconf.rb
creating Makefile
work/bcrypt-ruby $ make
compiling ./ext/mri/bcrypt_ext.c
compiling ./ext/mri/crypt_blowfish.c
cc -fPIC -O2 -pipe  -fstack-protector -isystem /usr/local/include -fno-strict-aliasing  -fPIC   -c ./ext/mri/x86.S -o x86.o
compiling ./ext/mri/crypt_gensalt.c
compiling ./ext/mri/wrapper.c
linking shared-object bcrypt_ext.so

Is there anything that needs to be done that this can be merged and released? (it breaks some projects for me)

adam12 · 2019-01-22T19:26:41Z

Can someone kick off Travis again? I think the 1 failure was a network timeout and not related to this patch.

fliiiix · 2019-01-22T20:27:59Z

@adam12 apparently you can close an reopen the pull request (travis-ci/travis-ci#576 (comment)) which should retrigger the pull request

adam12 · 2019-02-01T17:13:24Z

apparently you can close an reopen the pull request (travis-ci/travis-ci#576 (comment)) which should retrigger the pull request

@fliiiix right you were!

fliiiix · 2019-02-03T08:48:41Z

@tjschuck what needs to be done to get this merged?

rihadik · 2019-02-16T06:32:28Z

Same issue here, FreeBSD-12.

compiling bcrypt_ext.c
In file included from bcrypt_ext.c:2:
./ow-crypt.h:19:14: error: conflicting types for 'crypt_r'
extern char *crypt_r(__CONST char *key, __CONST char *setting, void *data);

rihadik · 2019-02-16T07:35:16Z

For us bcrypt gets fetched & installed as part of bundle install. Any way to dig deeper and patch bcrypt source code before installing it? When I try to patch the expanded file /path/to/vendor/bundle/ruby/2.5.0/gems/bcrypt-3.1.10/ext/mri/extconf.rb with $defs << "-D__SKIP_GNU" and run bundle install again it gets overwritten with the unpatched copy first.

nanaya · 2019-02-16T07:36:47Z

I just point bcrypt to my own patched fork wherever I use it until this gets merged (or fixed in some other way).

rihadik · 2019-02-16T09:04:05Z

% pwd
/path/to/www/vendor/bundle/ruby/2.5.0/gems/bcrypt-3.1.10
% ruby ./ext/mri/extconf.rb
creating Makefile
% make
compiling ./ext/mri/wrapper.c
./ext/mri/wrapper.c:188:7: error: conflicting types for 'crypt_r'
char *crypt_r(__CONST char *key, __CONST char *setting, void *data)
      ^
/usr/include/unistd.h:499:7: note: previous declaration is here
char    *crypt_r(const char *, const char *, struct crypt_data *);
         ^
1 error generated.
*** Error code 1

Stop.
make: stopped in /path/to/www/vendor/bundle/ruby/2.5.0/gems/bcrypt-3.1.10
% freebsd-version -kru
12.0-RELEASE-p3
12.0-RELEASE-p3
12.0-RELEASE-p3

rihadik · 2019-02-16T09:55:08Z

I just point bcrypt to my own patched fork

Sorry, but how do you do that? I'm assuming you're using a rails project.

nanaya · 2019-02-16T10:03:31Z

Something like this in Gemfile (or make your own fork if you want use specific version and adjust accordingly)

gem 'bcrypt', git: 'https://github.com/adam12/bcrypt-ruby', ref: '613daca'

rihadik · 2019-02-16T11:02:28Z

Sorry, but aren't you referring to adam's patch which you (and myself) had problems building with?

nanaya · 2019-02-16T11:07:21Z

That was referring to

The patch + removing wrapper.c & co.

The original patch (this PR) works fine (also it's the first thing I tried before even looking at this).

rihadik · 2019-02-16T11:33:52Z

Wow! I removed mentions of wrapper.c & wrapper.o from Makefile & ext/mri/Makefile (without deleting wrapper.c itself) and now the build succeeded! Thanks a lot!

% make
linking shared-object bcrypt_ext.so
%

rihadik · 2019-02-16T11:58:59Z

Building it one thing, installing it is another ) I tried copying the resultant bcrypt_ext.so to vendor/bundle/ruby/2.5.0/extensions/x86_64-freebsd-12/2.5.0-static/bcrypt-3.1.10 and created an empty file gem.build_complete there, like in other successfully built gems there, but bundle check still thinks bcrypt is missing :(

nanaya · 2019-02-16T12:02:27Z

What are you trying to do? If it's just installing bcrypt for rails or other ruby app using bundler/Gemfile, have you tried switching (by updating Gemfile) to the patched version I mentioned above?

rihadik · 2019-02-16T12:12:04Z

Replacing that line locally is harder than I thought :)

You are trying to install in deployment mode after changing
your Gemfile. Run `bundle install` elsewhere and add the
updated Gemfile.lock to version control.

The list of sources changed
The dependencies in your gemfile changed

You have added to the Gemfile:
* source: https://github.com/adam12/bcrypt-ruby (at 613daca)
* bcrypt

You have deleted from the Gemfile:
* bcrypt (~> 3.1.10)

Most of our other servers still use FreeBSD 10.x where there's no problem building bcrypt.

nanaya · 2019-02-16T12:14:41Z

As written there, you need to either update the lockfile somewhere else or (temporarily) disable deployment mode.

metalefty · 2019-02-19T01:26:29Z

Merge?

adam12 · 2019-02-25T19:32:51Z

@tjschuck @tenderlove were there any concerns with this PR? or anything need clarifying?

Thanks!

fliiiix · 2019-02-28T20:56:54Z

🎉 nice

fliiiix · 2019-03-08T16:50:37Z

Is there an ETA when this will be in a release? I would love to upgrade the FreeBSD package.

knu · 2019-04-08T22:17:40Z

@tenderlove I hope there'll be a new release soon!

metalefty · 2019-05-23T05:02:56Z

@tenderlove ping, why not make a release?

tenderlove · 2019-05-24T08:15:07Z

@metalefty tbh I didn't want to step on @tjschuck's toes. @tjschuck do you have time for a release? If not, I will do it 😊

metalefty · 2019-05-31T07:08:28Z

@tenderlove We FreeBSD users really appreciate if you guys make a new release 💯

tenderlove · 2019-05-31T16:02:37Z

@metalefty seems like @tjschuck is busy, so I'll just ship it today. Sorry this is taking so long

tenderlove · 2019-05-31T19:07:28Z

I shipped 3.1.13. Please let me know if there's anything else we need!

knu · 2019-06-01T01:20:11Z

Thanks! bcrypt 3.1.13 got successfully installed without an error on my FreeBSD 12 host.

tenderlove · 2019-06-03T16:21:11Z

@knu thanks for the feedback!

metalefty · 2019-06-04T03:39:37Z

Thanks you all!

fliiiix · 2019-06-05T06:25:49Z

Thank you for the release 👍
I just updated the port as well for FreeBSD https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=238332

adam12 force-pushed the skip-gnu-crypt-functions branch from 3b42d13 to 613daca Compare January 6, 2019 07:08

solardiz mentioned this pull request Jan 21, 2019

FreeBSD build is broken #188

Closed

adam12 closed this Feb 1, 2019

adam12 reopened this Feb 1, 2019

tenderlove merged commit 7644e36 into bcrypt-ruby:master Feb 28, 2019

metalefty mentioned this pull request May 23, 2019

FreeBSD 12 build is broken #196

Closed

benpratt mentioned this pull request Dec 27, 2019

'bundle install' fails on FreeBSD rapid7/metasploit-framework#12761

Closed

jlduran mentioned this pull request Aug 9, 2020

bcrypt 3.1.15 failing on FreeBSD 12 #233

Closed

Define SKIP_GNU token when building extension (Fixes FreeBSD >= 12) #189

Define SKIP_GNU token when building extension (Fixes FreeBSD >= 12) #189

Conversation

adam12 commented Jan 6, 2019

nanaya commented Jan 7, 2019

adam12 commented Jan 7, 2019

nanaya commented Jan 7, 2019 • edited Loading

adam12 commented Jan 7, 2019

nanaya commented Jan 7, 2019

adam12 commented Jan 7, 2019

fliiiix commented Jan 13, 2019 • edited Loading

adam12 commented Jan 22, 2019

fliiiix commented Jan 22, 2019

adam12 commented Feb 1, 2019

fliiiix commented Feb 3, 2019

rihadik commented Feb 16, 2019

rihadik commented Feb 16, 2019 • edited Loading

nanaya commented Feb 16, 2019

rihadik commented Feb 16, 2019 • edited Loading

rihadik commented Feb 16, 2019

nanaya commented Feb 16, 2019

rihadik commented Feb 16, 2019

nanaya commented Feb 16, 2019 • edited Loading

rihadik commented Feb 16, 2019

rihadik commented Feb 16, 2019

nanaya commented Feb 16, 2019

rihadik commented Feb 16, 2019 • edited Loading

nanaya commented Feb 16, 2019

metalefty commented Feb 19, 2019

adam12 commented Feb 25, 2019

fliiiix commented Feb 28, 2019

fliiiix commented Mar 8, 2019

knu commented Apr 8, 2019

metalefty commented May 23, 2019

tenderlove commented May 24, 2019

metalefty commented May 31, 2019

tenderlove commented May 31, 2019

tenderlove commented May 31, 2019

knu commented Jun 1, 2019

tenderlove commented Jun 3, 2019

metalefty commented Jun 4, 2019

fliiiix commented Jun 5, 2019

nanaya commented Jan 7, 2019 •

edited

Loading

fliiiix commented Jan 13, 2019 •

edited

Loading

rihadik commented Feb 16, 2019 •

edited

Loading

rihadik commented Feb 16, 2019 •

edited

Loading

nanaya commented Feb 16, 2019 •

edited

Loading

rihadik commented Feb 16, 2019 •

edited

Loading