A crash bugfix for ngx_http_dyups_module

[cuber]

Upstream request will crash if there is a first shown prefix variable in dynamic update.

server {
  listen 8000;

  location / {
    dyups_interface;
  }
}

server {
  listen 8080;
  location / {
    proxy_pass http://$host;
  }
}

$curl "127.0.0.1:8000/upstream/dyhost" -d 'consistent_hash $arg_ds; server 127.0.0.1:8081; server 127.0.0.1:8082;'
success
$curl "127.0.0.1:8080/foobar" -H'host: dyhost'
#crash happened

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[FengXingYuXin]

我理解 修复代码中，楼主为了复用函数ngx_http_variables_init_vars，做了很多准备工作，只是为新变量结构体设置get_handler、data、flag，是否可以直接遍历cmcf->variables，针对get_handler为空的情况 按照prefix_variables类别，设置对应的get_handler、data、flags就行了？

[FengXingYuXin]

这里是不是可以逆序遍历，遇到get_hander不为NULL，直接退出？减少遍历的时间

[cuber]

我觉得修改成逆序遍历提前退出带来的提升并不会太大， 理由有如下两个

1. 相比于 dyups 的消耗, 遍历 cmcf->variables 的成本并不高
2. cmcf->variables 的遍历是顺序访存

[cuber]

@FengXingYuXin 已修改为逆序遍历 😄

[FengXingYuXin]

@cuber 还有一个疑问，如果请求ngx_http_request_t中已经分配了variables（存储变量值），正处于接收请求body的阶段（可能会有多次网络IO），如果dyups有更新（consistent_hash+新自定义变量），那在请求接收完body被转发时，触发渲染新变量的逻辑，但新变量并没有在variables中分配对应内存，所以ngx_http_get_indexed_variable时，还是会导致r->variables[index]的index越界吧？

[cuber]

@FengXingYuXin
的确有越界风险，而且是从 ngx_http_script_run 入口开始就会有风险了
考虑如下情况：

1. 请求 A 创建时 r->variables 是按照 cmcf->variables 个数来 alloc 的
2. A 正在异步等待 upstream body
3. 来了个 dyups 的请求 B，把 cmcf->variables 给 append 了一个 element
4. 前面异步等待的 body 的请求 A 回来了以后，在 ngx_http_script_run 阶段就会越界

    cmcf = ngx_http_get_module_main_conf(r, ngx_http_core_module);

    for (i = 0; i < cmcf->variables.nelts; i++) {
        if (r->variables[i].no_cacheable) {
            r->variables[i].valid = 0;
            r->variables[i].not_found = 0;
        }
    }

[cuber]

@FengXingYuXin
改了下，现在应该 ok 了

[yunoasgit]

@ https://github.com/yzprofile/ngx_http_dyups_module
Has this version been fixed?

[wangfakang]

@cuber could you resolve the conflict ? THX.

[cuber]

@wangfakang I have adapted this patch to the new version of Tengine, please take a look :-)

[chobits]

Hi all,

this issue has been fixed in dyups module, see yzprofile/ngx_http_dyups_module#104

[chobits]

Hi @cuber

I will close this issue later, because we have one pullrequest to update yzprofile/dyups module: #1255.

But I note that this pull request has one difference with yzprofile/dyups bugfix pr.

typedef struct {
    ngx_uint_t                           ref;
+    ngx_http_dyups_srv_conf_t           *duscf;       <<< here!!!
    ngx_http_upstream_init_peer_pt       init;
} ngx_http_dyups_upstream_srv_conf_t;

Can yzprofile/dyups bugfix pr replace this pull request?

---

We hope that tengine/dyups can keep pace with yzprofile/dyups.

[cuber]

Hi @chobits,
Actually, #1133 dose more than yzprofile/dyups#104
I will make another pr to yzprofile/dyups, so tengine can keep pace with the origin repository

[chobits]

hi all

Because #1255 (updated to yzprofile/dyups) has been merged.

This pr has conflicts again.

@cuber, you should open a pr to yzprofile/dyups first, our dev team will review your pr in yzprofile/dyups and then merge it to tengine/dyups.

[cuber]

Hi @chobits @wangfakang
I have created a new PR for yzprofile/ngx_http_dyups_module#104
In the mean time, i have found a dangling pointer bug for modules/ngx_http_upstream_check_module.
To fix this bug, i have create a new PR to tengine #1265