search

GUI / nginx-upstream-dynamic-servers

An nginx module to resolve domain names inside upstreams and keep them up to date.
MIT License
311 stars 74 forks source link

Do not use any local memory pool #21

Closed ariya closed 7 years ago

ariya commented 8 years ago

Most Nginx code assumes that it is working with a global memory pool. Using a local memory pool is inviting a series of use-after-free operations.

ariya commented 8 years ago

This is likely going to fix issue #18 (as well as #13).

wandenberg commented 8 years ago

@ariya your changes may solve the other issues but creates a new one. It will leak memory. May be a small amount of memory, but each time a DNS changes it will lose memory. I was thinking in a way that we can add or remove/disable a peer without reinitialize the upstream configuration. Not losing memory or creating a reference to a dead peer. I'm working in a patch in this way. If someone can help testing would be nice.

ariya commented 8 years ago

Due to the way the upstream data structure is managed inside Nginx, the memory growth is inevitable. Without this change (local memory pool), the growth is masked by releasing the allocated memory from time to time. This is incorrect, because every other modules (from keep-alive, ip-hash, zone) have access to the upstream data and potentially read from/write to freed memory (use-after-free).

IOW, this patch forces the correctness of the memory handling, both by this module and the rest.

What you plan to work on @wandenberg is optimization, i.e. to ensure that the memory growth is controllable. I do agree that we need to work on that. I also believe that it can't work without the correct memory usage introduced in this change.

What about opening a separate/new issue to discuss memory optimization strategy? I have some ideas as well (using the down flag, preallocate servers, etc).

GoodGame commented 7 years ago

Hello! I have same problems on my servers. "It will leak memory" any ideas how to fix it?

ariya commented 7 years ago

This has been in the queue for 6 months. I doubt that it will be merged anytime soon. Hence, I'm closing it.

alexzzh commented 1 year ago

inevitable

@wandenberg "I was thinking in a way that we can add or remove/disable a peer without reinitialize the upstream configuration. Not losing memory or creating a reference to a dead peer" ---> any update? thanks for your contribution!