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

buffer size may be too small #269

Closed
akf00000 opened this issue Apr 7, 2023 · 2 comments · Fixed by #270
Closed

buffer size may be too small #269

akf00000 opened this issue Apr 7, 2023 · 2 comments · Fixed by #270

Comments

@akf00000
Copy link

akf00000 commented Apr 7, 2023

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 akf00000 changed the title wrong buffer size buffer size may be too small Apr 7, 2023
@u5surf
Copy link
Collaborator

u5surf commented Apr 7, 2023

@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?

u5surf added a commit to u5surf/nginx-module-vts that referenced this issue Apr 16, 2023
@vozlt
Copy link
Owner

vozlt commented Apr 17, 2023

@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.

u5surf added a commit that referenced this issue Apr 17, 2023
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 a pull request may close this issue.

3 participants