Don't match from the middle of a character - #219
Conversation
e874771 to
685585a
Compare
| return true if pos == 0 || pos == @string.bytesize | ||
| return true if @string.bytesize == @string.length | ||
| begin | ||
| @string.b.byteslice(0, pos).force_encoding(@string.encoding).valid_encoding? |
There was a problem hiding this comment.
@eregon This implementation is slow. Is there any better way to detect character boundary in TruffleRuby?
There was a problem hiding this comment.
Could we validate on pos= instead maybe? That would avoid adding overhead on the fast path.
This seems indeed very slow, and I think between the existing exception and this expensive check, the existing exception is better, because users are very unlikely to hit it.
So I think no changes on TruffleRuby + skipping that test is better.
The exception is fatal though as we can see here, that's something we should fix in TruffleRuby to be some Ruby exception instead: truffleruby/truffleruby#4474
There was a problem hiding this comment.
(FWIW there is Primitive.string_is_character_head? but that's a private Primitive so it cannot be used outside TruffleRuby's core library.)
There was a problem hiding this comment.
If we move the validation to pos=, we can't avoid other cases such as get_byte.
I'll not change the TruffleRuby implementation in this PR.
There was a problem hiding this comment.
Thanks, I think not changing TruffleRuby is the best trade-off in this PR.
I'll consider exposing Primitive.string_is_character_head? as a public Primitive for the next release.
There was a problem hiding this comment.
OK. Could you work on it as a follow-up PR?
I'll merge this PR as-is.
|
Returning |
This case (match from the middle of a character) must be a "no match" case. This is a fast path for this case. (This also avoids false-positive match by |
`#pos=` accepts an arbitrary byte offset, so the scan position can fall in the middle of a multi-byte character such as UTF-16BE. Matching from there made the regexp engine read a whole character starting at that position, reading past the end of the string. On CRuby this is an out-of-bounds read. On JRuby a search such as `#scan_until` could loop without terminating. On TruffleRuby it aborts with an internal error while building the matched substring. But we don't change the TruffleRuby implementation because we don't have a fast implementation for this. Refuse to match unless the current position is the head of a character.
685585a to
e85effe
Compare
(ruby/strscan#219) `#pos=` accepts an arbitrary byte offset, so the scan position can fall in the middle of a multi-byte character such as UTF-16BE. Matching from there made Onigmo read a whole character starting at that position, reading past the end of the string. On CRuby this is an out-of-bounds read. On JRuby a search such as `#scan_until` could loop without terminating. On TruffleRuby it aborts with an internal error while building the matched substring. But we don't change the TruffleRuby implementation because we don't have a fast implementation for this. Refuse to match unless the current position is the head of a character. ruby/strscan@a552ea6f49
#pos=accepts an arbitrary byte offset, so the scan position can fall in the middle of a multi-byte character such as UTF-16BE. Matching from there made Onigmo read a whole character starting at that position, reading past the end of the string.On CRuby this is an out-of-bounds read.
On JRuby a search such as
#scan_untilcould loop without terminating.On TruffleRuby it aborts with an internal error while building the matched substring. But we don't change the TruffleRuby implementation because we don't have a fast implementation for this.
Refuse to match unless the current position is the head of a character.