walkor / workerman

An asynchronous event driven PHP socket framework. Supports HTTP, Websocket, SSL and other custom protocols.
http://www.workerman.net
MIT License
11.03k stars 2.25k forks source link

`/src/Protocols/Http.php` `Content-Length` 疑问 #969

Closed tianyiw2013 closed 7 months ago

tianyiw2013 commented 9 months ago

https://github.com/walkor/workerman/blob/1c24fe43f58a8026f675551d5e1dcc9f3e48b76e/src/Protocols/Http.php#L134C1-L134C1

        if ($pos = stripos($header, "\r\nContent-Length: ")) {
            $length += (int)substr($header, $pos + 18, 10);
            $hasContentLength = true;
        } 

$length += (int)substr($header, $pos + 18, 10); 第三个参数为什么是固定数字10呢?是有特别的原因吗? 我觉得应该改成

$length += substr($header, $pos + 18, strpos($header, "\r\n", $pos + 18) - $pos - 18);
tourze commented 8 months ago

感觉你写的更加合理点。

twomiao commented 7 months ago
  1. 你这代码看似完整性,但功能来说他那个没问题的,你失去了可读性。
  2. 如何解决这问题,添加一行英文注释说明下10的含义就行了。
tianyiw2013 commented 7 months ago
  1. 你这代码看似完整性,但功能来说他那个没问题的,你失去了可读性。
  2. 如何解决这问题,添加一行英文注释说明下10的含义就行了。

源代码是$length += (int)substr($header, $pos + 18, 10);截取了10个字符长度,应该不正确,不一定是10个字符长度呢,然后强制转换为int,如果不强制转换的话,有可能截取到的结果是999\nxxx

https://github.com/walkor/workerman/blob/1c24fe43f58a8026f675551d5e1dcc9f3e48b76e/src/Protocols/Http.php#L133-L139

两个判断重复,第一个强制截取了10个字节,第二个好像永远不会执行。

twomiao commented 7 months ago
  1. 你这代码看似完整性,但功能来说他那个没问题的,你失去了可读性。
  2. 如何解决这问题,添加一行英文注释说明下10的含义就行了。

源代码是$length += (int)substr($header, $pos + 18, 10);截取了10个字符长度,应该不正确,不一定是10个字符长度呢,然后强制转换为int,如果不强制转换的话,有可能截取到的结果是999\nxxx

int 32 位最大才多少,他这个极限是999999999字节,你见过这么大的网页文本(9G)? 截取到字符串的情况,强制转换int类型他不是已经处理这情况了?(int) substr(xxx);

tianyiw2013 commented 7 months ago
  1. 你这代码看似完整性,但功能来说他那个没问题的,你失去了可读性。
  2. 如何解决这问题,添加一行英文注释说明下10的含义就行了。

源代码是$length += (int)substr($header, $pos + 18, 10);截取了10个字符长度,应该不正确,不一定是10个字符长度呢,然后强制转换为int,如果不强制转换的话,有可能截取到的结果是999\nxxx

int 32 位最大才多少,他这个极限是999999999字节,你见过这么大的网页文本(9G)? 截取到字符串的情况,强制转换int类型他不是已经处理这情况了?(int) substr(xxx);

你这么说确实没问题,但我忘记是因为什么发现的这个bug了,这行代码报过错~_~

twomiao commented 7 months ago
  1. 你这代码看似完整性,但功能来说他那个没问题的,你失去了可读性。
  2. 如何解决这问题,添加一行英文注释说明下10的含义就行了。

源代码是$length += (int)substr($header, $pos + 18, 10);截取了10个字符长度,应该不正确,不一定是10个字符长度呢,然后强制转换为int,如果不强制转换的话,有可能截取到的结果是999\nxxx

复现问题给作者,然后pr修复一下。你这样写作者应该不会采取的(增加开销不说,可读性差太多了。)

twomiao commented 7 months ago

标准http报文测试没有问题

<?php
$header = "HTTP/1.1 200 OK \r\nAccess-Control-Allow-Methods: POST, GET \r\nAccess-Control-Allow-Origin: https://www.baidu.com"." \r\nContent-Length: 105 \r\nContent-Type: application/json; charset=utf-8\r\n\r\n";
$pos = strpos($header, "\r\nContent-Length: ");

$value = substr($header, $pos + 18, 10);

$length = (int)$value;

var_dump($value, $length, $length === 105); 

输出结果:

string(10) "105 
Cont"
int(105)
bool(true)
xpader commented 7 months ago

有些看似不合理的代码,实则很巧妙,当然你的出发点也是没错的,更加严谨。