skywind3000 / kcp

:zap: KCP - A Fast and Reliable ARQ Protocol
MIT License
15.2k stars 2.49k forks source link

Bug:ikcp_send 发送大于发送窗口的数据包时,会return -2,但是部分数据已经被拼接进segment,最终被发送出去。 #373

Closed mingcent closed 1 year ago

mingcent commented 1 year ago

file: ikcp.c, line:510 , ikcp_send函数中的 count 判断有点问题,当发送一个大于发送窗口的包,并返回-2时,数据包的前一段数据已经被拼接进segment,最终发送了一个不完整的数据包。后面的数据流在接收端都会解析失败。可以考虑把这个判断放在segment拼接前,或者在返回失败的时候,恢复原来的segment。

obuffer commented 1 year ago

主要注意的是这个已经是切片之后count了,初始值128不小了,可能作者的原意应该是既然超出了最初设计的最大值,其实已经是出错了,读不读得到前一个包关系已经不大了。

mingcent commented 1 year ago

主要注意的是这个已经是切片之后count了,初始值128不小了,可能作者的原意应该是既然超出了最初设计的最大值,其实已经是出错了,读不读得到前一个包关系已经不大了。

这里的大小确实不小了,一般不会触发这个限制。我也是在项目bug导致数据包不断增长的情况下遇到这个问题。这里的问题是流模式下,需要在包头放一个size字段,告知后面还有多少字节数据,以解决粘包问题。但是这里将发送失败的数据包开头的一部分数据被拼进segment,破坏了数据流。这样接收方在收到开头这一部分数据时,认为后面还有size大小的数据,但是这部分数据没有被发送出去,接收方会把后面其他包的数据当作前一个失败包的数据,后面的流数据解析都会差一个偏移量。

obuffer commented 1 year ago

既然返回-2了,那后面的数据不是已经是错误的了吗,读出来肯定也是错误的呀?这时候其实是不是已经可以断开连接了

skywind3000 commented 1 year ago

不会发生你说的情况的,ikcp_send 时,一个包要不被赛进去,要不不能:

https://github.com/skywind3000/kcp/blob/10ee2b30c8739408c0db98211df4ebce7b01c0ed/ikcp.c#L504-L513

大于 IKCP_WND_RCV 是不允许的。

mingcent commented 1 year ago

https://github.com/skywind3000/kcp/blob/10ee2b30c8739408c0db98211df4ebce7b01c0ed/ikcp.c#L489-L494

流模式下,这里在判断是否大于IKCP_WND_RCV前,已经把buffer中的数据往之前没有塞满的segment里面填充了一部分了呀。如果ikcp send失败,这部分数据是会被发出去的。

skywind3000 commented 1 year ago

https://github.com/skywind3000/kcp/commit/c0cb6ab3ac78cdcaba119eb1d2b0c6042db11a5c

修正了下返回值,如果是流模式,之前追加了内容,那么 -2 时会返回之前追加的长度,没追加才返回 -2 代表 EAGAIN 类似的意思。

mingcent commented 1 year ago

有一个小的建议不知是否合理:ikcp send 如果未发生错误, 则返回发送的总字节数,该字节数可能小于 在 len 参数中请求发送的数量。(目前的操作系统 send 函数也是这样)。 现在修正了返回值后,“如果是流模式,之前追加了内容,那么 -2 时会返回之前追加的长度”,这个没问题。但是如果 ikcp send 正常发送成功,返回值是0,而不是发送buffer的长度,这不符合规范。以及下面的这部分代码,如果内存分配失败,返回 -2, 也有这个问题。 https://github.com/skywind3000/kcp/blob/10ee2b30c8739408c0db98211df4ebce7b01c0ed/ikcp.c#L514-L537

skywind3000 commented 1 year ago

@mingcent 改了: https://github.com/skywind3000/kcp/commit/f2aa30ea21a63c9ceeb6393e7d5d0c85a05a52a7

xfgryujk commented 1 year ago

这段代码我也有疑惑,流模式是不是不应该限制一次发送的长度,至少不应该限制到IKCP_WND_RCV这么小。查了一下和 #325 的观点一样