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: slice buffer properly in ClearOut #4184

Closed
wants to merge 2 commits into from
Closed

tls_wrap: slice buffer properly in ClearOut #4184

wants to merge 2 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Dec 8, 2015

Fix incorrect slicing of cleartext buffer in TLSWrap::ClearOut.

Fix: #4161

@indutny
tls_wrap: slice buffer properly in ClearOut
a83c191 
Fix incorrect slicing of cleartext buffer in `TLSWrap::ClearOut`.

Fix: nodejs#4161
@indutny indutny mentioned this pull request Dec 8, 2015
Closed
@indutny
Copy link
Member Author

indutny commented Dec 8, 2015

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

@indutny
Copy link
Member Author

indutny commented Dec 8, 2015

cc @nodejs/crypto

@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++. labels Dec 8, 2015
@indutny
Copy link
Member Author

indutny commented Dec 8, 2015

CI is green

mscdex
mscdex reviewed Dec 8, 2015
View reviewed changes
test/parallel/test-tls-inception.js
@@ -60,7 +62,7 @@ a.listen(common.PORT, function() {
});
ssl.setEncoding('utf8');
ssl.once('data', function(data) {
assert.equal('hello', data);
assert.equal(body.toString(), data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe to assume the entire body will always come in one data event?

Copy link
Member Author

Choose a reason for hiding this comment

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

Argh, of course no.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@mscdex
Copy link
Contributor

mscdex commented Dec 8, 2015

LGTM

@indutny
Copy link
Member Author

indutny commented Dec 8, 2015

Landed in c0cb80e, thank you!

@indutny indutny closed this Dec 8, 2015
indutny added a commit that referenced this pull request Dec 8, 2015
@indutny
tls_wrap: slice buffer properly in ClearOut
c0cb80e 
Fix incorrect slicing of cleartext buffer in `TLSWrap::ClearOut`.

Fix: #4161
PR-URL: #4184
Reviewed-By: Brian White <mscdex@mscdex.net>
@indutny indutny deleted the fix/gh-4161 branch December 8, 2015 05:19
santigimeno
santigimeno reviewed Dec 8, 2015
View reviewed changes
test/parallel/test-tls-inception.js
ssl.once('data', function(data) {
assert.equal('hello', data);
buf += data;
gotHello = true;
});
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be on?
Also, is gotHello really necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Arghh... It should be on indeed. @santigimeno would it be interesting to you to submit a PR with a fix for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

gotHello seems to be necessary, but could be replaced with common.mustCall in ssl.on('end', ...)

Copy link
Member

Choose a reason for hiding this comment

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

I said about gotHello because would the test exit if no end event was received?
Yes, I can send a PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, @santigimeno . Please don't forget to cc me on that PR ;)

Copy link
Member

Choose a reason for hiding this comment

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

@indutny PR: #4195. Thanks

indutny added a commit that referenced this pull request Dec 9, 2015
@indutny @rvagg
tls_wrap: slice buffer properly in ClearOut
939f305 
Fix incorrect slicing of cleartext buffer in `TLSWrap::ClearOut`.

Fix: #4161
PR-URL: #4184
Reviewed-By: Brian White <mscdex@mscdex.net>
@rvagg rvagg mentioned this pull request Dec 17, 2015
Closed
indutny added a commit that referenced this pull request Dec 30, 2015
@indutny
tls_wrap: slice buffer properly in ClearOut
10a75c3 
Fix incorrect slicing of cleartext buffer in `TLSWrap::ClearOut`.

Fix: #4161
PR-URL: #4184
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
@indutny
tls_wrap: slice buffer properly in ClearOut
0efc35e 
Fix incorrect slicing of cleartext buffer in `TLSWrap::ClearOut`.

Fix: #4161
PR-URL: #4184
Reviewed-By: Brian White <mscdex@mscdex.net>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
Merged
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@indutny
tls_wrap: slice buffer properly in ClearOut
2116299 
Fix incorrect slicing of cleartext buffer in `TLSWrap::ClearOut`.

Fix: nodejs#4161
PR-URL: nodejs#4184
Reviewed-By: Brian White <mscdex@mscdex.net>
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.
4 participants
@indutny @mscdex @santigimeno @MylesBorins