Project

General

Profile

Bug #15926

Edge case issue with String#uminus

Added by luke-gru (Luke Gruber) about 1 month ago. Updated 18 days ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:93159]

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

str-uminus-freeze-fix.patch (1.38 KB) str-uminus-freeze-fix.patch jeremyevans0 (Jeremy Evans), 06/19/2019 02:04 AM
str-uminus-freeze-fix-v2.patch (1.58 KB) str-uminus-freeze-fix-v2.patch jeremyevans0 (Jeremy Evans), 06/19/2019 05:03 PM

Associated revisions

Revision 7582287e
Added by jeremyevans (Jeremy Evans) 18 days ago

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]

History

Updated by jeremyevans0 (Jeremy Evans) about 1 month 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 1 month 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 1 month 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 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);
}

I agree, that is a better way to go. Attached is a new patch that does that and adds a couple more tests.

#4

Updated by jeremyevans (Jeremy Evans) 18 days 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]

Also available in: Atom PDF