Skip to content

[Cache] fix: remove unwanted cast to int #54776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 30, 2024
Merged

[Cache] fix: remove unwanted cast to int #54776

merged 1 commit into from
Apr 30, 2024

Conversation

Ahummeling
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #54773
License MIT

As discussed in the related issue, this type cast is unnecessary and can lead to unexpected behavior due to integer overflow.

I took a quick look if a testcase could be implemented, but this is not so trivial, as we'd either have to somehow configure a redis server to end up with a state that results in very high scan cursor values, or we'd have to implement a mock for the redis implementation, so we can provide all the edge case values for our tests.

@carsonbot carsonbot added this to the 5.4 milestone Apr 29, 2024
@Ahummeling Ahummeling force-pushed the fix/cache-int-overflow branch 2 times, most recently from 62d7114 to fcf6df4 Compare April 29, 2024 21:26
@Ahummeling Ahummeling force-pushed the fix/cache-int-overflow branch from fcf6df4 to 3d174a9 Compare April 29, 2024 21:27
@Ahummeling
Copy link
Contributor Author

Ahummeling commented Apr 29, 2024 •
edited
Loading

The changes proposed by the failing fabbot.io job are completely unrelated to my changes.

curl https://fabbot.io/patch/symfony/symfony/54776/3d174a96ff253a877f81fe7216e1e72eae9d1d0e/cs.diff | patch -p0

Perhaps this check was added after this file was last edited?
I am unsure whether my PR should include the suggested fabbot.io changes or not. If they should be included, I'll happily do so.

diff --git a/src/Symfony/Component/Cache/Traits/RedisTrait.php b/src/Symfony/Component/Cache/Traits/RedisTrait.php
index 33f37d828e..0512f75cf0 100644
--- a/src/Symfony/Component/Cache/Traits/RedisTrait.php
+++ b/src/Symfony/Component/Cache/Traits/RedisTrait.php
@@ -310,8 +310,10 @@ trait RedisTrait
         } elseif (is_a($class, \RedisArray::class, true)) {
             foreach ($hosts as $i => $host) {
                 switch ($host['scheme']) {
-                    case 'tcp': $hosts[$i] = $host['host'].':'.$host['port']; break;
-                    case 'tls': $hosts[$i] = 'tls://'.$host['host'].':'.$host['port']; break;
+                    case 'tcp': $hosts[$i] = $host['host'].':'.$host['port'];
+                        break;
+                    case 'tls': $hosts[$i] = 'tls://'.$host['host'].':'.$host['port'];
+                        break;
                     default: $hosts[$i] = $host['path'];
                 }
             }
@@ -331,8 +333,10 @@ trait RedisTrait
             $initializer = static function () use ($class, $params, $hosts) {
                 foreach ($hosts as $i => $host) {
                     switch ($host['scheme']) {
-                        case 'tcp': $hosts[$i] = $host['host'].':'.$host['port']; break;
-                        case 'tls': $hosts[$i] = 'tls://'.$host['host'].':'.$host['port']; break;
+                        case 'tcp': $hosts[$i] = $host['host'].':'.$host['port'];
+                            break;
+                        case 'tls': $hosts[$i] = 'tls://'.$host['host'].':'.$host['port'];
+                            break;
                         default: $hosts[$i] = $host['path'];
                     }
                 }
@@ -347,9 +351,12 @@ trait RedisTrait
                     $redis->setOption(\Redis::OPT_TCP_KEEPALIVE, $params['tcp_keepalive']);
                 }
                 switch ($params['failover']) {
-                    case 'error': $redis->setOption(\RedisCluster::OPT_SLAVE_FAILOVER, \RedisCluster::FAILOVER_ERROR); break;
-                    case 'distribute': $redis->setOption(\RedisCluster::OPT_SLAVE_FAILOVER, \RedisCluster::FAILOVER_DISTRIBUTE); break;
-                    case 'slaves': $redis->setOption(\RedisCluster::OPT_SLAVE_FAILOVER, \RedisCluster::FAILOVER_DISTRIBUTE_SLAVES); break;
+                    case 'error': $redis->setOption(\RedisCluster::OPT_SLAVE_FAILOVER, \RedisCluster::FAILOVER_ERROR);
+                        break;
+                    case 'distribute': $redis->setOption(\RedisCluster::OPT_SLAVE_FAILOVER, \RedisCluster::FAILOVER_DISTRIBUTE);
+                        break;
+                    case 'slaves': $redis->setOption(\RedisCluster::OPT_SLAVE_FAILOVER, \RedisCluster::FAILOVER_DISTRIBUTE_SLAVES);
+                        break;
                 }
 
                 return $redis;

nicolas-grekas
nicolas-grekas approved these changes Apr 29, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can ignore fabbot indeed.

@derrabus derrabus added the Cache label Apr 30, 2024
@carsonbot carsonbot changed the title fix: remove unwanted cast to int [Cache] fix: remove unwanted cast to int Apr 30, 2024
@nicolas-grekas
Copy link
Member

Thank you @Ahummeling.

@nicolas-grekas nicolas-grekas merged commit ad0caee into symfony:5.4 Apr 30, 2024
9 of 12 checks passed
@Ahummeling Ahummeling deleted the fix/cache-int-overflow branch April 30, 2024 09:19
This was referenced Jun 2, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@xabbuh xabbuh xabbuh approved these changes

@dsdeboer dsdeboer dsdeboer approved these changes

@JoeriThissen-Simplicate JoeriThissen-Simplicate JoeriThissen-Simplicate approved these changes

Assignees
No one assigned
Labels
Bug Cache Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
7 participants
@Ahummeling @nicolas-grekas @xabbuh @dsdeboer @JoeriThissen-Simplicate @derrabus @carsonbot