bcrypt_ext: Add compatibility with libxcrypt #164
Conversation
|
The failing tests are not releated to this change. |
|
Any ETA on getting this merged? Bcrypt on with We pointed our Gemfile at https://github.com/besser82/bcrypt-ruby/tree/libxcrypt and it totally fixes the problem. |
tenderlove
left a comment
There was a problem hiding this comment.
This PR looks good, but I'm curious which change was actually necessary to fix the bug. Seems like only the second change is necessary (and maybe we should just change it to rb_str_new2).
ext/mri/bcrypt_ext.c
Outdated
| if(!salt) return Qnil; | ||
|
|
||
| str_salt = rb_str_new2(salt); | ||
| str_salt = rb_str_new(salt, strlen(salt)); |
There was a problem hiding this comment.
Is this actually necessary to fix the bug? It seems like rb_str_new2 already does that. (rb_str_new2 is just an alias for rb_str_new_cstr)
|
I've updated the PR. |
|
JFYI: Fedora-28 with libxcrypt is released. |
|
Fedora 28 was release 2 days ago and at the moment you can't use bcrypt on it. Can we get that fixed? Thanks! |
|
@duritong |
|
Unfortunately it's not a solution for rvm/rbenv/etc-based workflow. |
* master: Use RBX 3 Try updating Bundler too Test on more Rubies in CI; looser version definition Update RG and see if that fixes the build Update lockfile so newer Ruby works with JSON gem
|
I have just pushed version 3.1.12.rc1 to RubyGems.org which should solve this problem. Can someone please confirm that |
|
Works fine here (F28 w/ all updates, ruby-2.5.0p0 via rvm), thanks. |
|
Yes, now everything is ok on F28 |
|
Thanks. It fixes issue with
|
I believe this bugfix is needed: bcrypt-ruby/bcrypt-ruby#164
The return values of
crypt()andcrypt_gensalt()should be treated as real c-strings instead of binary data by the module.