Skip to content

[Bugfix] Fixing a rare bug when calling BCrypt::Engine#hash_secret - which produces nil accidentally 1 out of 500 cases#270

Merged
tenderlove merged 1 commit into
bcrypt-ruby:masterfrom
itarato:bugfix/PA-missing-gc-guard
Jun 22, 2023
Merged

[Bugfix] Fixing a rare bug when calling BCrypt::Engine#hash_secret - which produces nil accidentally 1 out of 500 cases#270
tenderlove merged 1 commit into
bcrypt-ruby:masterfrom
itarato:bugfix/PA-missing-gc-guard

Conversation

@itarato
Copy link
Copy Markdown
Contributor

@itarato itarato commented Jun 22, 2023 •
edited
Loading

Issue

The following script almost always cannot complete in TruffleRuby:

require("bcrypt")

1000.times do
	BCrypt::Engine.hash_secret("test", BCrypt::Engine.generate_salt(5))
end

At various points it either returns nil:

bcrypt-ruby/ext/mri/bcrypt_ext.c

Line 98 in e402d69

if(!value || !args.data) return Qnil;

Or it segfaults:

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00007f633b0ec4d5, pid=23812, tid=23812
#
# JRE version: OpenJDK Runtime Environment GraalVM CE 17.0.7-dev+4.1 (17.0.7+4) (build 17.0.7+4-jvmci-23.1-b02)
# Java VM: OpenJDK 64-Bit Server VM GraalVM CE 17.0.7-dev+4.1 (17.0.7+4-jvmci-23.1-b02, mixed mode, sharing, tiered, jvmci, jvmci compiler, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# J 24970 jvmci org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.profiledPERoot([Ljava/lang/Object;)Ljava/lang/Object; jdk.internal.vm.compiler (57 bytes) @ 0x00007f633b0ec4d5 [0x00007f633b0ebce0+0x00000000000007f5] (_crypt_blowfish_rn#1)
#

Bisecting pointed out to this line causing the problem:

bcrypt-ruby/ext/mri/bcrypt_ext.c

Lines 89 to 90 in e402d69

args.key = NIL_P(key) ? NULL : StringValueCStr(key);
args.setting = NIL_P(setting) ? NULL : StringValueCStr(setting);
as well as

bcrypt-ruby/ext/mri/bcrypt_ext.c

Line 38 in e402d69

args.prefix = StringValueCStr(prefix);
and

bcrypt-ruby/ext/mri/bcrypt_ext.c

Line 40 in e402d69

args.input = NIL_P(input) ? NULL : StringValuePtr(input);

@eregon pointed out that these operations are not GC safe as the underlying value can be freed before its last use. Adding GC guards fixed the problem.

From the context we've gathered it seems CRuby is a bit more lucky not failing with this right now, though syntactically it seems to be beneficial to have this change equally (hence the fix in the gem).

@tjschuck
Copy link
Copy Markdown
Collaborator

tjschuck commented Jun 22, 2023

/cc @tenderlove

@itarato itarato marked this pull request as draft June 22, 2023 14:09
@eregon
Copy link
Copy Markdown
Contributor

eregon commented Jun 22, 2023 •
edited
Loading

i.e., it's the frequent problem of StringValueCStr, StringValuePtr, RSTRING_PTR and friends, which return a char* from a VALUE. The VALUE (Ruby String) must be RB_GC_GUARD after the last usage of the char*, otherwise the Ruby String can be GC'd, free the char*, but the char* is still accessed after.

This is quite error-prone, but it is the CRuby GC approach/semantics which relies on stack & register scanning.

@itarato itarato changed the title [Bugfix] Fixing a rare bug when calling BCrypt::Engine#hash_secret - which produces nil accidentally 1 out of 500 cases in TruffleRuby [Bugfix] Fixing a rare bug when calling BCrypt::Engine#hash_secret - which produces nil accidentally 1 out of 500 cases Jun 22, 2023
@tenderlove
Copy link
Copy Markdown
Collaborator

tenderlove commented Jun 22, 2023 •
edited
Loading

Yes, great catch. This is definitely a case where the compiler could optimize out register / stack writes. I'm happy to merge this and ship a new version.

@itarato @eregon
Fixing a rare bug when calling BCrypt::Engine#hash_secret - which pro…
53ec1e1 
…duces nil accidentally 1 out of 500 cases in TruffleRuby.

Co-authored-by: Benoit Daloze <benoit.daloze@oracle.com>
Co-authored-by: Peter Arato <it.arato@gmail.com>
@itarato itarato force-pushed the bugfix/PA-missing-gc-guard branch from 255be67 to 53ec1e1 Compare June 22, 2023 16:24
@itarato itarato marked this pull request as ready for review June 22, 2023 16:32
@tenderlove tenderlove merged commit d4e4813 into bcrypt-ruby:master Jun 22, 2023
@eregon eregon mentioned this pull request Jun 22, 2023
Closed
@tjschuck
Copy link
Copy Markdown
Collaborator

tjschuck commented Jun 22, 2023

@tenderlove cut the release for this today: bcrypt version 3.1.19.

@itarato
Copy link
Copy Markdown
Contributor Author

itarato commented Jun 23, 2023

Appreciate all the help 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@itarato @tjschuck @eregon @tenderlove