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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80364ef
tls_wrap: reach error reporting for UV_EPROTO
indutny Jan 26, 2016
3348b74
fix
indutny Jan 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,7 +760,7 @@ function afterWrite(status, handle, req, err) {
}

if (status < 0) {
var ex = exceptionWithHostPort(status, 'write', req.address, req.port);
var ex = errnoException(status, 'write', req.error);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change necessary? Also, I think this a) makes it a semver-major, and b) users will be less than pleased with the loss of the address/port information.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no req.address or req.port on write. I think it has been put there mistakingly.

The only difference that is going to be is the absence of err.address, which was previously undefined. Therefore I consider this a bugfix.

debug('write failure', ex);
self._destroy(ex, req.cb);
return;
Expand Down
11 changes: 9 additions & 2 deletions src/stream_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,15 @@ class StreamReq {
explicit StreamReq(DoneCb cb) : cb_(cb) {
}

inline void Done(int status) {
cb_(static_cast<Req*>(this), status);
inline void Done(int status, const char* error_str = nullptr) {
Req* req = static_cast<Req*>(this);
Environment* env = req->env();
if (error_str != nullptr) {
req->object()->Set(env->error_string(),
OneByteString(env->isolate(), error_str));
}

cb_(req, status);
}

private:
Expand Down
20 changes: 13 additions & 7 deletions src/tls_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +92,15 @@ void TLSWrap::MakePending() {
}


bool TLSWrap::InvokeQueued(int status) {
bool TLSWrap::InvokeQueued(int status, const char* error_str) {
if (pending_write_items_.IsEmpty())
return false;

// Process old queue
WriteItemList queue;
pending_write_items_.MoveBack(&queue);
while (WriteItem* wi = queue.PopFront()) {
wi->w_->Done(status);
wi->w_->Done(status, error_str);
delete wi;
}

Expand Down Expand Up @@ -484,11 +484,12 @@ bool TLSWrap::ClearIn() {

// Error or partial write
int err;
Local<Value> arg = GetSSLError(written, &err, &error_);
const char* error_str = nullptr;
Local<Value> arg = GetSSLError(written, &err, &error_str);
if (!arg.IsEmpty()) {
MakePending();
if (!InvokeQueued(UV_EPROTO))
ClearError();
InvokeQueued(UV_EPROTO, error_str);
delete[] error_str;
clear_in_->Reset();
}

Expand Down Expand Up @@ -589,8 +590,13 @@ int TLSWrap::DoWrite(WriteWrap* w,
return 0;
}

if (ssl_ == nullptr)
if (ssl_ == nullptr) {
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.

return UV_EPROTO;
}

crypto::MarkPopErrorOnReturn mark_pop_error_on_return;

Expand Down Expand Up @@ -775,7 +781,7 @@ void TLSWrap::DestroySSL(const FunctionCallbackInfo<Value>& args) {
wrap->MakePending();

// And destroy
wrap->InvokeQueued(UV_ECANCELED);
wrap->InvokeQueued(UV_ECANCELED, "Canceled because of SSL destruction");

// Destroy the SSL structure and friends
wrap->SSLWrap<TLSWrap>::DestroySSL();
Expand Down
2 changes: 1 addition & 1 deletion src/tls_wrap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class TLSWrap : public AsyncWrap,
bool ClearIn();
void ClearOut();
void MakePending();
bool InvokeQueued(int status);
bool InvokeQueued(int status, const char* error_str = nullptr);

inline void Cycle() {
// Prevent recursion
Expand Down
29 changes: 29 additions & 0 deletions test/parallel/test-tls-junk-server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
'use strict';
const common = require('../common');

if (!common.hasCrypto) {
console.log('1..0 # Skipped: missing crypto');
return;
}

const assert = require('assert');
const https = require('https');
const net = require('net');

const server = net.createServer(function(s) {
s.once('data', function() {
s.end('I was waiting for you, hello!', function() {
s.destroy();
});
});
});

server.listen(common.PORT, function() {
const req = https.request({ port: common.PORT });
req.end();

req.once('error', common.mustCall(function(err) {
assert(/unknown protocol/.test(err.message));
server.close();
}));
});