Skip to content
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.

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

fix sproto write_ff buffer overflow #1205

Merged
merged 2 commits into from
Jun 19, 2020
Merged

Conversation

wudeng
Copy link
Contributor

@wudeng wudeng commented Jun 19, 2020

buffer overflow occurs when packing a string with length = 2046 or 2047

@cloudwu
Copy link
Owner

cloudwu commented Jun 19, 2020

两个问题:

  1. 什么时候会 overflow ,没看出来?因为这里统计的是连续不为 0 的字节数量,是根据 src 数出来有 256*8 个的。
  2. 如果想避免 overflow ,srcsz < 256*8 是不够的。因为 srcsz 是总长度,但是此处 ff_srcstart 已经相比原始地址有向后的偏移了。

@lvzixun
Copy link
Contributor

lvzixun commented Jun 19, 2020

  1. https://github.com/cloudwu/sproto/blob/master/sproto.c#L1248-L1250 这里做了优化, 如果前8个字节是非零,那将会对后8个字节中如果非零个数大于6都算作是8个非零了。所以pack 2046个非零数据应该会读越界报错。
  2. 我觉得哪里应该改成(srcsz-i) < 256*8 ? (srcsz-i): 256*8

PS: 看样子是最多会越界访问2个字节,这个问题跟之前我修复write_ff cloudwu/sproto#15 蛮相似的。不过之前我修复的是写越界。

@cloudwu
Copy link
Owner

cloudwu commented Jun 19, 2020

我明白了,是因为最后一个 seg 补了一个或两个 0 ,导致统计数字 overflow 。但似乎这样,对应的 unpack 有点问题。

@lvzixun
Copy link
Contributor

lvzixun commented Jun 19, 2020

我明白了,是因为最后一个 seg 补了一个或两个 0 ,导致统计数字 overflow 。但似乎这样,对应的 unpack 有点问题。

unpack应该是会补零的。但是decode应该是没问题的,后面的2个0 也不会读。

@cloudwu
Copy link
Owner

cloudwu commented Jun 19, 2020

我觉得这里是不会发生 write overflow , 因为 bufsz 提前减了,并且在 write_ff 的时候判断了 bufsz 。所以这里发生的事 read overflow ,即 ff_srcstart 指向的数据不够长。

但是,这里的 read overflow 可能发生在所有的 write_ff 之处,光改这一处是不够的。

@wudeng
Copy link
Contributor Author

wudeng commented Jun 19, 2020

  1. https://github.com/cloudwu/sproto/blob/master/sproto.c#L1248-L1250 这里做了优化, 如果前8个字节是非零,那将会对后8个字节中如果非零个数大于6都算作是8个非零了。所以pack 2046个非零数据应该会读越界报错。
  2. 我觉得哪里应该改成(srcsz-i) < 256*8 ? (srcsz-i): 256*8

PS: 看样子是最多会越界访问2个字节,这个问题跟之前我修复write_ff cloudwu/sproto#15 蛮相似的。不过之前我修复的是写越界。

发现计算剩余长度的时候也不能直接 srcsz - i,要计算从最后一个字节到 ff_srcstart 的长度。。。我修正了一下

@wudeng
Copy link
Contributor Author

wudeng commented Jun 19, 2020

我觉得这里是不会发生 write overflow , 因为 bufsz 提前减了,并且在 write_ff 的时候判断了 bufsz 。所以这里发生的事 read overflow ,即 ff_srcstart 指向的数据不够长。

但是,这里的 read overflow 可能发生在所有的 write_ff 之处,光改这一处是不够的。

是的,这里只是 read overflow,不过我用不同长度的字符串测试下来,只有这里发生了overflow,其他地方并没有发生。

cloudwu added a commit that referenced this pull request Jun 19, 2020
@cloudwu
Copy link
Owner

cloudwu commented Jun 19, 2020

我仔细考虑了一下,的确只有这个分支有问题。但我认为这里的设计有点问题。如果回溯去看,

cloudwu/sproto@51ac280

这里也是在修同样的问题,但是没有改彻底。

我建议这样改:27f1823

看看对不对?

@wudeng
Copy link
Contributor Author

wudeng commented Jun 19, 2020

我仔细考虑了一下,的确只有这个分支有问题。但我认为这里的设计有点问题。如果回溯去看,

cloudwu/sproto@51ac280

这里也是在修同样的问题,但是没有改彻底。

我建议这样改:27f1823

看看对不对?

加一个判断条件就对了。
最后那个write_ff 如果 ff_n == 0 就没必要执行了

@cloudwu cloudwu merged commit 435aa38 into cloudwu:master Jun 19, 2020
wangyi0226 added a commit to wangyi0226/skynet that referenced this pull request Jun 20, 2020
fix sproto write_ff buffer overflow (cloudwu#1205)
fanlix pushed a commit to fanlix/skynet that referenced this pull request Jul 22, 2020
* bugfix , see cloudwu#1205

* fix bug

Co-authored-by: Cloud Wu <cloudwu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants