From: "mame (Yusuke Endoh) via ruby-core" <ruby-core@...>
Date: 2024-03-28T03:22:53+00:00
Subject: [ruby-core:117353] [Ruby master Feature#20394] Add an offset parameter to `String#to_i`

Issue #20394 has been updated by mame (Yusuke Endoh).


I thought of a security concern.

I suppose all methods proposed in this ticket would allow underscores as a digit separator. This is natural in Ruby, but usually an unnecessary feature outside of Ruby, including the Redis protocol.
This interpretation mismatch could be a source of vulnerability. I don't know if this will actually cause a problem in the Redis protocol, but in HTTP it is called "HTTP request/response smuggling".

https://cwe.mitre.org/data/definitions/444.html

`Integer(gets)` or `IO#scan_integer` converts the input to Integer without making sure that the contents are `/^[0-9]+$/`. I think that it is okay for a script that handles only trusted sources, but in general it is not recommended for a library used by various people.

----------------------------------------
Feature #20394: Add an offset parameter to `String#to_i`
https://bugs.ruby-lang.org/issues/20394#change-107511

* Author: byroot (Jean Boussier)
* Status: Open
----------------------------------------
### Context

I maintain the `redis-client` gem, and it comes with an optional swapable implementation in C that binds the `hiredis` C client, [which used to performs up to 5 times faster in some cases](https://github.com/redis-rb/redis-client/commit/9fabd57c6786a03fe0c6021eab5b181d9316d9d7).

I recently paired with @tenderlovemaking to try to close this gap, or even try to make the pure Ruby version faster, and we came up with several optimizations that now almost make both version on par (assuming YJIT is enabled).

An important source of performance loss, is that the Redis protocol is line based and to parse it in Ruby requires to slice a lot of small strings from the buffer. To give an example, here's how an Array with two String (`["foo", "plop"]`) is serialized in RESP3 (Redis protocol):

```
*2\r\n
$3\r\n
foo\r\n
$4\r\n
plop\r\n
```

From this you can understand that a big hotspot in the parser is essentially `Integer(gets)`.

With @tenderlovemaking we managed to get [a fairly significant perf boost](https://github.com/redis-rb/redis-client/commit/41b3abe94243d2598211d448c4e457a3585ff9d5#diff-a8b5ce23fb9396492f56bf0bd23090910918a488416cfb488cef8b5b34877328) by avoiding these string allocation using `String#getbyte` and [basically implementing a rudimentary `String#to_i(offset: )` in Ruby](https://github.com/redis-rb/redis-client/commit/41b3abe94243d2598211d448c4e457a3585ff9d5#diff-5f15c6483e788ee14f367f65fb951800d52341726f528bcddff1e2cd3e62cab9R105-R115).

But while the gains are huge with YJIT enabled, they are much more tame with the interpreter. And it feels a bit wrong to have to implement this sorts of things for performance reasons.

### `String#to_i(offset: )`

Similar to `String#unpack(offset:)` ([Feature #18254]), I believe `String#to_i(offset: )` would be useful.

### Alternative new `String#unpack` format

Another possibility would be to add a new format to `String#pack` `String#unpack` for decimal numbers. It sounds a bit weird at first, but given it supports things like Base64 and hexadecimal, perhaps it's not that much of a stretch?







-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/