src: move req_wrap_queue to base class of ReqWrap

Introduce a second base class for `ReqWrap` that does not depend on a template parameter and move the `req_wrap_queue_` field to it. This addresses undefined behaviour that occurs when casting to `ReqWrap<uv_req_t>` in the `ReqWrap` constructor. Refs: #26131 PR-URL: #26148 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Feb 28, 2019 · c6d5af5 · c6d5af5
1 parent 007b2fa
commit c6d5af5
Show file tree

Hide file tree

Showing 6 changed files with 39 additions and 18 deletions.
diff --git a/src/env.cc b/src/env.cc
@@ -420,7 +420,7 @@ void Environment::RegisterHandleCleanups() {
 }
 
 void Environment::CleanupHandles() {
-  for (ReqWrap<uv_req_t>* request : req_wrap_queue_)
+  for (ReqWrapBase* request : req_wrap_queue_)
     request->Cancel();
 
   for (HandleWrap* handle : handle_wrap_queue_)

diff --git a/src/env.h b/src/env.h
@@ -877,8 +877,7 @@ class Environment {
 #endif
 
   typedef ListHead<HandleWrap, &HandleWrap::handle_wrap_queue_> HandleWrapQueue;
-  typedef ListHead<ReqWrap<uv_req_t>, &ReqWrap<uv_req_t>::req_wrap_queue_>
-          ReqWrapQueue;
+  typedef ListHead<ReqWrapBase, &ReqWrapBase::req_wrap_queue_> ReqWrapQueue;
 
   inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; }
   inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; }

diff --git a/src/node_postmortem_metadata.cc b/src/node_postmortem_metadata.cc
@@ -28,15 +28,14 @@
   V(Environment_HandleWrapQueue, head_, ListNode_HandleWrap,                  \
     Environment::HandleWrapQueue::head_)                                      \
   V(ListNode_HandleWrap, next_, uintptr_t, ListNode<HandleWrap>::next_)       \
-  V(ReqWrap, req_wrap_queue_, ListNode_ReqWrapQueue,                          \
-    ReqWrap<uv_req_t>::req_wrap_queue_)                                       \
   V(Environment_ReqWrapQueue, head_, ListNode_ReqWrapQueue,                   \
     Environment::ReqWrapQueue::head_)                                         \
-  V(ListNode_ReqWrap, next_, uintptr_t, ListNode<ReqWrap<uv_req_t>>::next_)
+  V(ListNode_ReqWrap, next_, uintptr_t, ListNode<ReqWrapBase>::next_)
 
 extern "C" {
 int nodedbg_const_ContextEmbedderIndex__kEnvironment__int;
 uintptr_t nodedbg_offset_ExternalString__data__uintptr_t;
+uintptr_t nodedbg_offset_ReqWrap__req_wrap_queue___ListNode_ReqWrapQueue;
 
 #define V(Class, Member, Type, Accessor)                                      \
   NODE_EXTERN uintptr_t NODEDBG_OFFSET(Class, Member, Type);
@@ -51,6 +50,9 @@ int GenDebugSymbols() {
       ContextEmbedderIndex::kEnvironment;
 
   nodedbg_offset_ExternalString__data__uintptr_t = NODE_OFF_EXTSTR_DATA;
+  nodedbg_offset_ReqWrap__req_wrap_queue___ListNode_ReqWrapQueue =
+      OffsetOf<ListNode<ReqWrapBase>, ReqWrap<uv_req_t>>(
+          &ReqWrap<uv_req_t>::req_wrap_queue_);
 
   #define V(Class, Member, Type, Accessor)                                    \
     NODEDBG_OFFSET(Class, Member, Type) = OffsetOf(&Accessor);

diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc
@@ -252,7 +252,8 @@ static void GetActiveRequests(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
   std::vector<Local<Value>> request_v;
-  for (auto w : *env->req_wrap_queue()) {
+  for (ReqWrapBase* req_wrap : *env->req_wrap_queue()) {
+    AsyncWrap* w = req_wrap->GetAsyncWrap();
     if (w->persistent().IsEmpty())
       continue;
     request_v.push_back(w->GetOwner());

diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h
@@ -11,16 +11,16 @@
 
 namespace node {
 
+ReqWrapBase::ReqWrapBase(Environment* env) {
+  env->req_wrap_queue()->PushBack(this);
+}
+
 template <typename T>
 ReqWrap<T>::ReqWrap(Environment* env,
                     v8::Local<v8::Object> object,
                     AsyncWrap::ProviderType provider)
-    : AsyncWrap(env, object, provider) {
-
-  // FIXME(bnoordhuis) The fact that a reinterpret_cast is needed is
-  // arguably a good indicator that there should be more than one queue.
-  env->req_wrap_queue()->PushBack(reinterpret_cast<ReqWrap<uv_req_t>*>(this));
-
+    : AsyncWrap(env, object, provider),
+      ReqWrapBase(env) {
   Reset();
 }
 
@@ -51,6 +51,11 @@ void ReqWrap<T>::Cancel() {
     uv_cancel(reinterpret_cast<uv_req_t*>(&req_));
 }
 
+template <typename T>
+AsyncWrap* ReqWrap<T>::GetAsyncWrap() {
+  return this;
+}
+
 // Below is dark template magic designed to invoke libuv functions that
 // initialize uv_req_t instances in a unified fashion, to allow easier
 // tracking of active/inactive requests.

diff --git a/src/req_wrap.h b/src/req_wrap.h
@@ -10,8 +10,24 @@
 
 namespace node {
 
+class ReqWrapBase {
+ public:
+  explicit inline ReqWrapBase(Environment* env);
+
+  virtual ~ReqWrapBase() {}
+
+  virtual void Cancel() = 0;
+  virtual AsyncWrap* GetAsyncWrap() = 0;
+
+ private:
+  friend int GenDebugSymbols();
+  friend class Environment;
+
+  ListNode<ReqWrapBase> req_wrap_queue_;
+};
+
 template <typename T>
-class ReqWrap : public AsyncWrap {
+class ReqWrap : public AsyncWrap, public ReqWrapBase {
  public:
   inline ReqWrap(Environment* env,
                  v8::Local<v8::Object> object,
@@ -23,21 +39,19 @@ class ReqWrap : public AsyncWrap {
   // Call this after a request has finished, if re-using this object is planned.
   inline void Reset();
   T* req() { return &req_; }
-  inline void Cancel();
+  inline void Cancel() final;
+  inline AsyncWrap* GetAsyncWrap() override;
 
   static ReqWrap* from_req(T* req);
 
   template <typename LibuvFunction, typename... Args>
   inline int Dispatch(LibuvFunction fn, Args... args);
 
  private:
-  friend class Environment;
   friend int GenDebugSymbols();
   template <typename ReqT, typename U>
   friend struct MakeLibuvRequestCallback;
 
-  ListNode<ReqWrap> req_wrap_queue_;
-
   typedef void (*callback_t)();
   callback_t original_callback_ = nullptr;