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

https: fix memory leak with https.request() #8647

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@imyller
Member

imyller commented Sep 18, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

https

Description of change

If calling https.request() with options.headers.host defined and options.servername undefined, https.Agent.createSocket mutates connection options after https.Agent.addRequest has created empty socket pool array with mismatching connection name.

This results in two socket pool arrays being created and only the last one gets eventually deleted by removeSocket - effectively causing a memory leak.

This commit fixes the leak by making sure that addRequest does the same modifications to options object as the createSocket.

createSocket is intentionally left unmodified to prevent userland regressions.

Test case included.

Fixes: #6687

/cc @nodejs/http @nodejs/crypto

@imyller

This comment has been minimized.

Show comment
Hide comment
options.servername = options.host;
const hostHeader = req.getHeader('host');
if (hostHeader) {
options.servername = hostHeader.replace(/:.*$/, '');

This comment has been minimized.

@mscdex

mscdex Sep 18, 2016

Contributor

I'd prefer just using hostHeader.slice() and hostHeader.indexOf(), but if others are not comfortable with that, at the very least the regex should be /:[\s\S]*$/ to match any character (including newlines).

@mscdex

mscdex Sep 18, 2016

Contributor

I'd prefer just using hostHeader.slice() and hostHeader.indexOf(), but if others are not comfortable with that, at the very least the regex should be /:[\s\S]*$/ to match any character (including newlines).

This comment has been minimized.

@imyller

imyller Sep 18, 2016

Member

I left the hostHeader cleanup intentionally unmodified to match the original feature implemented PR #1110

I did not want to fix two unrelated issues in single PR dedicated to memory leak.

@imyller

imyller Sep 18, 2016

Member

I left the hostHeader cleanup intentionally unmodified to match the original feature implemented PR #1110

I did not want to fix two unrelated issues in single PR dedicated to memory leak.

Show outdated Hide outdated test/parallel/test-https-agent-sockets-leak.js
@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Sep 18, 2016

Member

LGTM

Member

indutny commented Sep 18, 2016

LGTM

@cjihrig

LGTM

@JacksonTian

This comment has been minimized.

Show comment
Hide comment
@JacksonTian

JacksonTian Sep 18, 2016

Contributor

LGTM

Contributor

JacksonTian commented Sep 18, 2016

LGTM

@imyller

This comment has been minimized.

Show comment
Hide comment
@imyller

imyller Sep 18, 2016

Member

FreeBSD CI failed due to force pushed commit

New CI run: https://ci.nodejs.org/job/node-test-pull-request/4108/

Member

imyller commented Sep 18, 2016

FreeBSD CI failed due to force pushed commit

New CI run: https://ci.nodejs.org/job/node-test-pull-request/4108/

@imyller

This comment has been minimized.

Show comment
Hide comment
@imyller

imyller Sep 18, 2016

Member

I'm going to add lts-agenda label because this leak affects current LTS release too.
Feel free to remove if fixing this in LTS is not relevant.

Member

imyller commented Sep 18, 2016

I'm going to add lts-agenda label because this leak affects current LTS release too.
Feel free to remove if fixing this in LTS is not relevant.

@imyller

This comment has been minimized.

Show comment
Hide comment
@imyller
Member

imyller commented Sep 19, 2016

@jasnell

LGTM

Show outdated Hide outdated test/parallel/test-https-agent-sockets-leak.js
https: fix memory leak with https.request()
If calling `https.request()` with `options.headers.host` defined
and `options.servername` undefined, `https.Agent.createSocket` mutates
connection `options` after `https.Agent.addRequest` has created empty
socket pool array with mismatching connection name. This results in two
socket pool arrays being created and only the last one gets eventually
deleted by `removeSocket` - causing a memory leak.

This commit fixes the leak by making sure that `addRequest` does the
same modifications to `options` object as the `createSocket`.

`createSocket` is intentionally left unmodified to prevent userland
regressions.

Test case included.

Fixes: #6687
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 20, 2016

Member

Trying CI again due to the AIX error: https://ci.nodejs.org/job/node-test-pull-request/4179/
Seems unrelated but just to be safe...

Member

jasnell commented Sep 20, 2016

Trying CI again due to the AIX error: https://ci.nodejs.org/job/node-test-pull-request/4179/
Seems unrelated but just to be safe...

@imyller imyller self-assigned this Sep 20, 2016

@imyller

This comment has been minimized.

Show comment
Hide comment
@imyller

imyller Sep 20, 2016

Member

If CI is green, I'll land this.

Member

imyller commented Sep 20, 2016

If CI is green, I'll land this.

@imyller

This comment has been minimized.

Show comment
Hide comment
@imyller

imyller Sep 21, 2016

Member

I'll start landing this:

  • Three Four LGTMs
  • No objections
  • Requested modifications have been made
  • CI tests passed (only unrelated CI failures)
Member

imyller commented Sep 21, 2016

I'll start landing this:

  • Three Four LGTMs
  • No objections
  • Requested modifications have been made
  • CI tests passed (only unrelated CI failures)

imyller added a commit to imyller/node that referenced this pull request Sep 21, 2016

https: fix memory leak with https.request()
If calling `https.request()` with `options.headers.host` defined
and `options.servername` undefined, `https.Agent.createSocket` mutates
connection `options` after `https.Agent.addRequest` has created empty
socket pool array with mismatching connection name. This results in two
socket pool arrays being created and only the last one gets eventually
deleted by `removeSocket` - causing a memory leak.

This commit fixes the leak by making sure that `addRequest` does the
same modifications to `options` object as the `createSocket`.

`createSocket` is intentionally left unmodified to prevent userland
regressions.

Test case included.

PR-URL: nodejs#8647
Fixes: nodejs#6687
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jackson Tian <shvyo1987@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@imyller

This comment has been minimized.

Show comment
Hide comment
@imyller

imyller Sep 21, 2016

Member

landed in db5a879

Member

imyller commented Sep 21, 2016

landed in db5a879

@imyller imyller closed this Sep 21, 2016

@imyller imyller removed their assignment Sep 21, 2016

@MylesBorins MylesBorins removed the lts-agenda label Sep 30, 2016

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

@imyller I'm removing the agenda label for right now as there does not appear to be anything controversial about this change. After it has lived in a release for at least two weeks we can backport

Member

MylesBorins commented Sep 30, 2016

@imyller I'm removing the agenda label for right now as there does not appear to be anything controversial about this change. After it has lived in a release for at least two weeks we can backport

Fishrock123 added a commit that referenced this pull request Oct 11, 2016

https: fix memory leak with https.request()
If calling `https.request()` with `options.headers.host` defined
and `options.servername` undefined, `https.Agent.createSocket` mutates
connection `options` after `https.Agent.addRequest` has created empty
socket pool array with mismatching connection name. This results in two
socket pool arrays being created and only the last one gets eventually
deleted by `removeSocket` - causing a memory leak.

This commit fixes the leak by making sure that `addRequest` does the
same modifications to `options` object as the `createSocket`.

`createSocket` is intentionally left unmodified to prevent userland
regressions.

Test case included.

PR-URL: #8647
Fixes: #6687
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jackson Tian <shvyo1987@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Nov 18, 2016

https: fix memory leak with https.request()
If calling `https.request()` with `options.headers.host` defined
and `options.servername` undefined, `https.Agent.createSocket` mutates
connection `options` after `https.Agent.addRequest` has created empty
socket pool array with mismatching connection name. This results in two
socket pool arrays being created and only the last one gets eventually
deleted by `removeSocket` - causing a memory leak.

This commit fixes the leak by making sure that `addRequest` does the
same modifications to `options` object as the `createSocket`.

`createSocket` is intentionally left unmodified to prevent userland
regressions.

Test case included.

PR-URL: #8647
Fixes: #6687
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jackson Tian <shvyo1987@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Nov 22, 2016

Merged

v4.7.0 proposal #9736

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment