Bug #15926
closedEdge case issue with String#uminus
Description
I was working on issue related to code in rb_fstring
(https://github.com/ruby/ruby/pull/2233) and saw some weird behavior in the function,
freezing the given string if it's not a "bare" string and it's small enough to be embedded.
The issue comes up in the following edge case:
class MyString < String
end
non_frozen = MyString.new("nonfrozen")
frozen = -non_frozen # deduplicates, but shouldn't freeze receiver
non_frozen << " added" # raises FrozenError
I'm not sure what the correct behavior should be with a subclass and String#uminus. Should it return
a frozen regular String or a frozen copy of the given class's string?
Not a practical concern (not often come upon I'm sure), but I think a valid one.
Files
Updated by jeremyevans0 (Jeremy Evans) about 5 years ago
I agree that this is a bug. I'm not sure if rb_fstring
's behavior should be changed (hopefully a more knowledgeable committer can weigh in), so a conservative approach to fix this is to dup the receiver before passing to rb_fstring
, if the receiver's class is not String
and the object is not already frozen. The attached patch does this.
Updated by luke-gru (Luke Gruber) about 5 years ago
Hi Jeremy, thanks for the patch. I agree with the strategy of just dealing with this in str_uminus
directly instead of fiddling around in the tricky rb_fstring
function. However there's still an issue when the string is a regular String, but tainted or has ivars. In this case, it still gets frozen. I think the check should be something like:
if (!BARE_STRING_P(str) && !rb_obj_frozen_p(str)) {
str = rb_str_dup(str);
}
The documentation of str_uminus
says the receiver will be deduplicated and returned unless tainted or has ivars, which is all well and good, but IMO it should never freeze the receiver under any circumstance. If it can't be deduplicated, it should return a frozen copy.
Updated by jeremyevans0 (Jeremy Evans) about 5 years ago
luke-gru (Luke Gruber) wrote:
Hi Jeremy, thanks for the patch. I agree with the strategy of just dealing with this in
str_uminus
directly instead of fiddling around in the trickyrb_fstring
function. However there's still an issue when the string is a regular String, but tainted or has ivars. In this case, it still gets frozen. I think the check should be something like:if (!BARE_STRING_P(str) && !rb_obj_frozen_p(str)) { str = rb_str_dup(str); }
I agree, that is a better way to go. Attached is a new patch that does that and adds a couple more tests.
Updated by jeremyevans (Jeremy Evans) about 5 years ago
- Status changed from Open to Closed
Applied in changeset git|7582287eb27e6b649789ce31ffdcbbb9ffcaf726.
Make String#-@ not freeze receiver if called on unfrozen subclass instance
rb_fstring behavior in this case is to freeze the receiver. I'm
not sure if that should be changed, so this takes the conservative
approach of duping the receiver in String#-@ before passing
to rb_fstring.
Fixes [Bug #15926]