vozlt/nginx-module-vts

buffer size may be too small

akf00000 opened this issue · 2 comments

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.

@akf00000 Hi, thanks report!
It is curious for me why did it looks good to us then because it has already fixed once at #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

@u5surf
At first, I thought that the size of one node(ngx_http_vhost_traffic_status_node_t) 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_set_key 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.