vozlt / nginx-module-vts

Nginx virtual host traffic status module
BSD 2-Clause "Simplified" License
3.17k stars 456 forks source link

buffer size may be too small #269

Closed akf00000 closed 1 year ago

akf00000 commented 1 year ago

In function ngx_http_vhost_traffic_status_display_get_size,we calculate the buffer size,the max name size default is 1024(un * 1024),but in function ngx_http_vhost_traffic_status_shm_add_node,we add a vts node and set vtsn->len = (u_short) key->len,so the vtsn->len max value is 65535. this will cause memory out of bounds,when we call status cmd,because the buffer size we calculate may be too small.

u5surf commented 1 year ago

@akf00000 Hi, thanks report! It is curious for me why did it looks good to us then because it has already fixed once at https://github.com/vozlt/nginx-module-vts/pull/161 Indeed, I consider that it seems to satisfy using size_t to fit nginx style instead any specified primitive type. But as a concern, I mean that it might be too much to use size_t which is generally int64 as node name length. I could not deep dive how much does the size suit for that. @jongiddy @vozlt If you have any knowledge of the detail, can you tell us that?

vozlt commented 1 year ago

@u5surf At first, I thought that the size of one node(_ngx_http_vhost_traffic_status_nodet) would be 255 or less like src/http/modules/ngx_http_limit_conn_module.c, and I think I did it with u_char. But if users use _vhost_traffic_status_filter_by_setkey to use multiple variables (e.g. $uri) then 255 seems not enough. 65535 seems to be a good enough size to me, but I don't know how the user will use it, so it's okay to modify it if it's not a big problem overall.