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

[itarato]

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]

/cc @tenderlove

[eregon]

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.

[tenderlove]

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.

[tjschuck]

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

[itarato]

Appreciate all the help 🙏