Project

General

Profile

Actions

Bug #15926

closed

Edge case issue with String#uminus

Added by luke-gru (Luke Gruber) over 5 years ago. Updated over 5 years ago.

Status:
Closed
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

Updated by jeremyevans0 (Jeremy Evans) over 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) over 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) over 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 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.

Actions #4

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

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0