Skip to content

Commit 484e06d

Browse files
kfarnungaddaleax
authored andcommitted
tls: use after free in tls_wrap
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: #18860 Refs: #18676 Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent 9169449 commit 484e06d

File tree

2 files changed

+18
-16
lines changed

2 files changed

+18
-16
lines changed

src/tls_wrap.cc

+17-16
Original file line numberDiff line numberDiff line change
@@ -310,10 +310,12 @@ void TLSWrap::EncOut() {
310310

311311

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

318320
if (ssl_ == nullptr)
319321
status = UV_ECANCELED;
@@ -579,18 +581,17 @@ int TLSWrap::DoWrite(WriteWrap* w,
579581
// However, if there is any data that should be written to the socket,
580582
// the callback should not be invoked immediately
581583
if (BIO_pending(enc_out_) == 0) {
582-
// We destroy the current WriteWrap* object and create a new one that
583-
// matches the underlying stream, rather than the TLSWrap itself.
584-
585-
// Note: We cannot simply use w->object() because of the "optimized"
586-
// way in which we read persistent handles; the JS object itself might be
587-
// destroyed by w->Dispose(), and the Local<Object> we have is not a
588-
// "real" handle in the sense the V8 is aware of its existence.
589-
Local<Object> req_wrap_obj =
590-
w->GetAsyncWrap()->persistent().Get(env()->isolate());
591-
w->Dispose();
592-
w = underlying_stream()->CreateWriteWrap(req_wrap_obj);
593-
return stream_->DoWrite(w, bufs, count, send_handle);
584+
CHECK_EQ(current_empty_write_, nullptr);
585+
current_empty_write_ = w;
586+
StreamWriteResult res =
587+
underlying_stream()->Write(bufs, count, send_handle);
588+
if (!res.async) {
589+
env()->SetImmediate([](Environment* env, void* data) {
590+
TLSWrap* self = static_cast<TLSWrap*>(data);
591+
self->OnStreamAfterWrite(self->current_empty_write_, 0);
592+
}, this, object());
593+
}
594+
return 0;
594595
}
595596
}
596597

src/tls_wrap.h

+1
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ class TLSWrap : public AsyncWrap,
152152
std::vector<uv_buf_t> pending_cleartext_input_;
153153
size_t write_size_;
154154
WriteWrap* current_write_ = nullptr;
155+
WriteWrap* current_empty_write_ = nullptr;
155156
bool write_callback_scheduled_ = false;
156157
bool started_;
157158
bool established_;

0 commit comments

Comments
 (0)