Skip to content

Commit

Permalink
tls: use after free in tls_wrap
Browse files Browse the repository at this point in the history
The root cause is that `req_wrap` is created in `StreamBase::Write`
and passed to `TLSWrap::DoWrite`. In the TLS case the object gets
disposed and replaced with a new instance, but the caller's pointer is
never updated. When the `StreamBase::Write` method returns, it returns
a pointer to the freed object to the caller. In some cases when the
object memory has already been reused an assert is hit in
`WriteWrap::SetAllocatedStorage` because the pointer is non-null.

PR-URL: nodejs#18860
Refs: nodejs#18676
Reviewed-By: Anna Henningsen <[email protected]>
  • Loading branch information
kfarnung authored and addaleax committed Feb 27, 2018
1 parent 9169449 commit 484e06d
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 16 deletions.
33 changes: 17 additions & 16 deletions src/tls_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -310,10 +310,12 @@ void TLSWrap::EncOut() {


void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) {
// Report back to the previous listener as well. This is only needed for the
// "empty" writes that are passed through directly to the underlying stream.
if (req_wrap != nullptr)
previous_listener_->OnStreamAfterWrite(req_wrap, status);
if (current_empty_write_ != nullptr) {
WriteWrap* finishing = current_empty_write_;
current_empty_write_ = nullptr;
finishing->Done(status);
return;
}

if (ssl_ == nullptr)
status = UV_ECANCELED;
Expand Down Expand Up @@ -579,18 +581,17 @@ int TLSWrap::DoWrite(WriteWrap* w,
// However, if there is any data that should be written to the socket,
// the callback should not be invoked immediately
if (BIO_pending(enc_out_) == 0) {
// We destroy the current WriteWrap* object and create a new one that
// matches the underlying stream, rather than the TLSWrap itself.

// Note: We cannot simply use w->object() because of the "optimized"
// way in which we read persistent handles; the JS object itself might be
// destroyed by w->Dispose(), and the Local<Object> we have is not a
// "real" handle in the sense the V8 is aware of its existence.
Local<Object> req_wrap_obj =
w->GetAsyncWrap()->persistent().Get(env()->isolate());
w->Dispose();
w = underlying_stream()->CreateWriteWrap(req_wrap_obj);
return stream_->DoWrite(w, bufs, count, send_handle);
CHECK_EQ(current_empty_write_, nullptr);
current_empty_write_ = w;
StreamWriteResult res =
underlying_stream()->Write(bufs, count, send_handle);
if (!res.async) {
env()->SetImmediate([](Environment* env, void* data) {
TLSWrap* self = static_cast<TLSWrap*>(data);
self->OnStreamAfterWrite(self->current_empty_write_, 0);
}, this, object());
}
return 0;
}
}

Expand Down
1 change: 1 addition & 0 deletions src/tls_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ class TLSWrap : public AsyncWrap,
std::vector<uv_buf_t> pending_cleartext_input_;
size_t write_size_;
WriteWrap* current_write_ = nullptr;
WriteWrap* current_empty_write_ = nullptr;
bool write_callback_scheduled_ = false;
bool started_;
bool established_;
Expand Down

0 comments on commit 484e06d

Please sign in to comment.