ThreadSafeFunction memory violation in ThreadSafeFinalize

[averrez]

Hi everyone,

I've been experiencing random rare crashes in both Mac and Windows when Napi::ThreadSafeFunction is deallocated. My investigation brought me to the New function:

template <typename ResourceString, typename ContextType,
          typename Finalizer, typename FinalizerDataType>
inline ThreadSafeFunction ThreadSafeFunction::New(napi_env env,
                                                  const Function& callback,
                                                  const Object& resource,
                                                  ResourceString resourceName,
                                                  size_t maxQueueSize,
                                                  size_t initialThreadCount,
                                                  ContextType* context,
                                                  Finalizer finalizeCallback,
                                                  FinalizerDataType* data,
                                                  napi_finalize wrapper) {
  static_assert(details::can_make_string<ResourceString>::value
      || std::is_convertible<ResourceString, napi_value>::value,
      "Resource name should be convertible to the string type");

  ThreadSafeFunction tsfn; // <---- Stack allocated.
  auto* finalizeData = new details::ThreadSafeFinalize<ContextType, Finalizer,
      FinalizerDataType>({ data, finalizeCallback, &tsfn._tsfn }); // <---- Pointer to ivar stored
  napi_status status = napi_create_threadsafe_function(env, callback, resource,
      Value::From(env, resourceName), maxQueueSize, initialThreadCount,
      finalizeData, wrapper, context, CallJS, &tsfn._tsfn);
  if (status != napi_ok) {
    delete finalizeData;
    NAPI_THROW_IF_FAILED(env, status, ThreadSafeFunction());
  }

  return tsfn;
}

I believe that the issue is that ThreadSafeFunction tsfn; is stack-allocated and a pointer to its ivar is stored in the ThreadSafeFinalize: { data, finalizeCallback, &tsfn._tsfn }

When function is being destroyed, the finalizer is called with this code in it:

if (finalizeData->tsfn) {
      *finalizeData->tsfn = nullptr;
}

If I understand correctly, it writes nullptr to the captured pointer, which is long gone by now.
Also, there are no other references in the code to that member. It feels like it's not used anywhere, and storing and nullifying it is redundant?

I also tested it with Xcode memory sanitizer and it confirmed that stack memory is being corrupted from ThreadSafeFinalize.

Thank you.

[KevinEady]

Hi @averrez ,

Thanks for spotting this. Yes, I think this is a leftover of the original implementation of holding the napi_threadsafe_function inside a std::unique_ptr and should have been removed in #546 but was overlooked. I will take this issue and fix with an incoming PR.