rb_str_locktmp: raises when called on a Frozen String

[byroot]

Ref: ruby/openssl#831

RuntimeError: temporal locking immutable string
string.c:102:in `rb_str_locktmp'
string.c:102:in `rb_str_locktmp'
/home/runner/.rubies/truffleruby-24.1.1/lib/truffle/truffle/cext_ruby.rb:38:in `syswrite_nonblock'

Whereas MRI doesn't care whether the string is frozen or not, it will lock it either way:

VALUE
rb_str_locktmp(VALUE str)
{
    if (FL_TEST(str, STR_TMPLOCK)) {
        rb_raise(rb_eRuntimeError, "temporal locking already locked string");
    }
    FL_SET(str, STR_TMPLOCK);
    return str;
}

VALUE
rb_str_unlocktmp(VALUE str)
{
    if (!FL_TEST(str, STR_TMPLOCK)) {
        rb_raise(rb_eRuntimeError, "temporal unlocking already unlocked string");
    }
    FL_UNSET(str, STR_TMPLOCK);
    return str;
}

cc @eregon @nirvdrum

[eregon]

It's from

truffleruby/src/main/java/org/truffleruby/cext/CExtNodes.java

Lines 972 to 976 in c229bfd

	@Specialization
	RubyString rbStrLockTmpImmutable(ImmutableRubyString string) {
	throw new RaiseException(getContext(),
	coreExceptions().runtimeError("temporal locking immutable string", this));
	}

From a very quick look the error is because rb_str_locktmp() is called on a truly-immutable string (e.g. a frozen string literal or String#-@ or Symbol#name).
That means it shouldn't have state, which also means it shouldn't be possible to lock it.

Isn't string locking meant to avoid concurrent modifications to a string and so shouldn't be done if the string is frozen or indicates a bug in the caller (trying to mutate a frozen string)?

rb_str_locktmp() could be noop for immutable strings, but is that correct? If something queries whether it's locked then it wouldn't reflect it.

[byroot]

rb_str_locktmp() could be noop for immutable strings, but is that correct? If something queries whether it's locked then it wouldn't reflect it.

Yeah, not too sure. We probably should add some specs after that. IMO it would be OK for locktmp to noop on frozen strings, as long as it's speced. But yes, that could be surprising to some people.

[eregon]

Probably we should ask on https://bugs.ruby-lang.org/.
In my understanding, and given https://github.com/ruby/ruby/blob/3650f2460f9f58ed188ad289cfb46a1d005d2538/include/ruby/internal/intern/string.h#L615 and https://github.com/ruby/ruby/blob/3650f2460f9f58ed188ad289cfb46a1d005d2538/spec/ruby/optional/capi/string_spec.rb#L1205-L1209 I think CRuby should assert !frozen in rb_str_locktmp() & rb_str_unlocktmp().

I also wonder if as a side effect of rb_str_locktmp(frozen_string), trying to mutate that frozen string raises another error and not the correct FrozenError.

[byroot]

I think CRuby should assert !frozen in rb_str_locktmp() & rb_str_unlocktmp().

Why, because locking it would mutate its flags? I think that's OK.

I wouldn't be a big fan of such precondition, because that just means rb_str_locktmp suddenly become way more cumbersome to use.

[eregon]

I filed https://bugs.ruby-lang.org/issues/20998
Changing the flags of a frozen string literal seems a clear bug in CRuby.
Either way, I believe the caller should not attempt to lock a frozen string, because locking is meant to mutate exclusively some string, and one must not mutate frozen strings anyway.

[eregon]

Keeping this open to track what's decided on the CRuby issue, and also because we should behave the same for ImmutableRubyString and frozen-but-not-ImmutableRubyString strings.