Skip to content

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

Merged
chobits merged 5 commits into
alibaba:masterfrom
chobits:master
Nov 29, 2022
Merged

fixed memory leak of ssl session reuse in dyups and session sticky module#1708
chobits merged 5 commits into
alibaba:masterfrom
chobits:master

Conversation

@chobits

@chobits chobits commented Nov 25, 2022

Copy link
Copy Markdown
Member

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

chobits commented Nov 25, 2022

Copy link
Copy Markdown
Member Author

@chobits
chobits requested a review from yzprofile November 25, 2022 14:16
@chobits

chobits commented Nov 25, 2022

Copy link
Copy Markdown
Member Author
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
sspd->sscf = sscf;
sspd->get_rr_peer = ngx_http_upstream_get_round_robin_peer;

r->upstream->peer.data = sspd;

@chobits chobits Nov 26, 2022

Copy link
Copy Markdown
Member Author

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

@chobits chobits Nov 26, 2022

Copy link
Copy Markdown
Member Author

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

@nandsky nandsky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@nandsky nandsky left a comment

Copy link
Copy Markdown
Collaborator

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

chobits commented Nov 29, 2022

Copy link
Copy Markdown
Member Author

这个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