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

fixed memory leak of ssl session reuse in dyups and session sticky module #1708

Merged
merged 5 commits into from
Nov 29, 2022

Conversation

chobits
Copy link
Member

@chobits chobits commented Nov 25, 2022

try to fix #1016

  • todo: added test case for session reuse into dyups.t
  • todo: added test case for session reuse into session_sticky.t

@chobits chobits changed the title dyups: fixed memory leak of ssl-session-reuse logic dyups: fixed memory leak of ssl session reuse Nov 25, 2022
@chobits
Copy link
Member Author

chobits commented Nov 25, 2022

@chobits chobits requested a review from yzprofile November 25, 2022 14:16
@chobits
Copy link
Member Author

chobits commented Nov 25, 2022

ngx_http_dyups_save_peer_session
   -> ctx->ssl_session = ssl_session;
// ctx is alloced from r->pool, which get reference of ssl_sessoin. 
// After r is released, ssl_session is not freed .
//
// There is another problem, since the ssl_session exists on the current ctx of r.
// After new request r' comes in, there is no ssl session saved on the new r'/ctx'.
// In such case,  ssl session reuse does not take effect
  1. Session leaks
  2. Session reuse logic does not take effect.

@chobits chobits added this to the 2.3.5 milestone Nov 26, 2022
It reuses ssl session reuse of round robin module now, and
removes duplicated codes in session sticky module.
@chobits chobits changed the title dyups: fixed memory leak of ssl session reuse fixed memory leak of ssl session reuse in dyups and session sticky module Nov 26, 2022
@chobits chobits requested a review from dinic November 26, 2022 07:25
@@ -246,16 +234,8 @@ ngx_http_upstream_session_sticky_init_peer(ngx_http_request_t *r,
sspd->sscf = sscf;
sspd->get_rr_peer = ngx_http_upstream_get_round_robin_peer;

r->upstream->peer.data = sspd;
Copy link
Member Author

@chobits chobits Nov 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We make sure that (sspd == &sspd->rr). See:

typedef struct {
    /* the round robin data must be first */
    ngx_http_upstream_rr_peer_data_t    rrp;   <- &sspd->rrp
...
ngx_http_upstream_ss_peer_data_t;              <- sspd

(refer to nginx core module: ngx_http_upstream_ip_hash_module.c)

ngx_http_upstream_session_sticky_set_peer_session;
r->upstream->peer.save_session =
ngx_http_upstream_session_sticky_save_peer_session;
#endif
Copy link
Member Author

@chobits chobits Nov 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont need to re-write set_session() and save_session() function when u->peer.data is pointer of ngx_http_upstream_rr_peer_data_t, see ngx_http_upstream_ip_hash_module.c.

Only when u->peer.data is changed to module private data, it needs to re-write these two callbacks. For example, see ngx_http_upstream_keepalive_module.c or dyups module.

@chobits chobits requested a review from nandsky November 28, 2022 01:54
Copy link
Collaborator

@nandsky nandsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个module里有不少code style问题,本次pr要不一起调整了?

Copy link
Collaborator

@nandsky nandsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chobits chobits merged commit 6624643 into alibaba:master Nov 29, 2022
@chobits
Copy link
Member Author

chobits commented Nov 29, 2022

这个module里有不少code style问题,本次pr要不一起调整了?

has been merged. May next time

@chobits chobits modified the milestones: 2.3.5, 2.4.0 Feb 8, 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 this pull request may close these issues.

dyups: https session重用的内存泄漏 // dyups: memory leak of ssl session reuse
3 participants