Fix potential double stream release

This might result in the remainingStreams being incremented twice and then overflowing to a float, because they're initially PHP_INT_MAX.
amphp · Jul 9, 2020 · 4cc8b27 · 4cc8b27
1 parent 7898365
commit 4cc8b27
Showing 1 changed file with 15 additions and 8 deletions.
diff --git a/src/Connection/Internal/Http2ConnectionProcessor.php b/src/Connection/Internal/Http2ConnectionProcessor.php
@@ -908,6 +908,8 @@ public function reserveStream(): void
     public function unreserveStream(): void
     {
         ++$this->remainingStreams;
+
+        \assert($this->remainingStreams <= $this->concurrentStreamLimit);
     }
 
     public function getRemainingStreams(): int
@@ -1189,7 +1191,7 @@ private function applySetting(int $setting, int $value): void
     {
         switch ($setting) {
             case Http2Parser::INITIAL_WINDOW_SIZE:
-                if ($value > 2147483647) { // (1 << 31) - 1
+                if ($value < 0 || $value > 2147483647) { // (1 << 31) - 1
                     $this->handleConnectionException(new Http2ConnectionException(
                         "Invalid window size: {$value}",
                         Http2Parser::FLOW_CONTROL_ERROR
@@ -1239,7 +1241,7 @@ private function applySetting(int $setting, int $value): void
                 return;
 
             case Http2Parser::MAX_CONCURRENT_STREAMS:
-                if ($value > 2147483647) { // (1 << 31) - 1
+                if ($value < 0 || $value > 2147483647) { // (1 << 31) - 1
                     $this->handleConnectionException(new Http2ConnectionException(
                         "Invalid concurrent streams value: {$value}",
                         Http2Parser::PROTOCOL_ERROR
@@ -1252,6 +1254,9 @@ private function applySetting(int $setting, int $value): void
 
                 $this->concurrentStreamLimit = $value;
                 $this->remainingStreams = $this->concurrentStreamLimit - $priorUsedStreams;
+
+                \assert($this->remainingStreams <= $this->concurrentStreamLimit);
+
                 return;
 
             case Http2Parser::HEADER_TABLE_SIZE: // TODO Respect this setting from the server
@@ -1361,6 +1366,14 @@ private function releaseStream(int $streamId, ?\Throwable $exception = null): vo
 
         $stream = $this->streams[$streamId];
 
+        unset($this->streams[$streamId]);
+
+        if ($streamId & 1) { // Client-initiated stream.
+            $this->remainingStreams++;
+
+            \assert($this->remainingStreams <= $this->concurrentStreamLimit);
+        }
+
         if ($stream->responsePending || $stream->body || $stream->trailers) {
             /**
              * @psalm-suppress DeprecatedClass
@@ -1411,12 +1424,6 @@ private function releaseStream(int $streamId, ?\Throwable $exception = null): vo
             });
         }
 
-        unset($this->streams[$streamId]);
-
-        if ($streamId & 1) { // Client-initiated stream.
-            $this->remainingStreams++;
-        }
-
         if (!$this->streams && !$this->socket->isClosed()) {
             $this->socket->unreference();
         }