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.

Sign up for GitHub

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

Jump to bottom

tls_wrap: reach error reporting for UV_EPROTO #4885

Closed
wants to merge 2 commits into from
Closed

tls_wrap: reach error reporting for UV_EPROTO #4885

wants to merge 2 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Jan 26, 2016

Do not swallow error details when reporting UV_EPROTO asynchronously,
and when creating artificial errors.

Fix: #3692

R = @bnoordhuis or @shigeki

cc @nodejs/crypto

@indutny
tls_wrap: reach error reporting for UV_EPROTO
80364ef 
Do not swallow error details when reporting UV_EPROTO asynchronously,
and when creating artificial errors.

Fix: nodejs#3692
@indutny indutny mentioned this pull request Jan 26, 2016
Closed
@indutny
Copy link
Member Author

indutny commented Jan 26, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/1385/

@indutny indutny added tls Issues and PRs related to the tls subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lts-watch-v4.x labels Jan 26, 2016
@shigeki
Copy link
Contributor

shigeki commented Jan 27, 2016

LGTM

bnoordhuis
bnoordhuis reviewed Jan 27, 2016
View reviewed changes
src/tls_wrap.cc
static char msg[] = "Write after DestroySSL";
char* tmp = new char[sizeof(msg)];
memcpy(tmp, msg, sizeof(msg));
error_ = tmp;
Copy link
Member

Choose a reason for hiding this comment

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

I'd add a CHECK_EQ(error_, nullptr), otherwise it's a memory leak waiting to happen.

EDIT: Or call ClearError() first.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

@indutny
Copy link
Member Author

indutny commented Jan 27, 2016

@bnoordhuis replied and fixed.

@jasnell
Copy link
Member

jasnell commented Jan 27, 2016

LGTM

@indutny
Copy link
Member Author

indutny commented Jan 27, 2016

@bnoordhuis PTAL ;)

@bnoordhuis
Copy link
Member

LGTM

@indutny
Copy link
Member Author

indutny commented Jan 27, 2016

Landed in ff4006c, thank you everyone!

@indutny indutny closed this Jan 27, 2016
indutny added a commit that referenced this pull request Jan 27, 2016
@indutny
tls_wrap: reach error reporting for UV_EPROTO
ff4006c 
Do not swallow error details when reporting UV_EPROTO asynchronously,
and when creating artificial errors.

Fix: #3692
PR-URL: #4885
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Jan 28, 2016
@indutny @rvagg
tls_wrap: reach error reporting for UV_EPROTO
fd2a95b 
Do not swallow error details when reporting UV_EPROTO asynchronously,
and when creating artificial errors.

Fix: #3692
PR-URL: #4885
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Feb 8, 2016
@indutny @rvagg
tls_wrap: reach error reporting for UV_EPROTO
d6fbd81 
Do not swallow error details when reporting UV_EPROTO asynchronously,
and when creating artificial errors.

Fix: #3692
PR-URL: #4885
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@rvagg rvagg mentioned this pull request Feb 8, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
@indutny
tls_wrap: reach error reporting for UV_EPROTO
2778ccb 
Do not swallow error details when reporting UV_EPROTO asynchronously,
and when creating artificial errors.

Fix: #3692
PR-URL: #4885
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
@indutny
tls_wrap: reach error reporting for UV_EPROTO
c908ff3 
Do not swallow error details when reporting UV_EPROTO asynchronously,
and when creating artificial errors.

Fix: #3692
PR-URL: #4885
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
@indutny
tls_wrap: reach error reporting for UV_EPROTO
03944fe 
Do not swallow error details when reporting UV_EPROTO asynchronously,
and when creating artificial errors.

Fix: #3692
PR-URL: #4885
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@indutny
tls_wrap: reach error reporting for UV_EPROTO
8cece20 
Do not swallow error details when reporting UV_EPROTO asynchronously,
and when creating artificial errors.

Fix: nodejs#3692
PR-URL: nodejs#4885
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@indutny @shigeki @jasnell @bnoordhuis @MylesBorins