module: address coverity warning of memory leak

[mhdawson]

Covery reported the cach passed in the user as being overwritten by newly added automatic caching. Guard the automatica cachine to avoid this.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/vm

[mhdawson]

Coverity report which looks valid to me. Since code later on seems to use the used_cache_from_user to decided how to report the error (user reported error or automatic cache one), it seems like the right answer is to only use the automatic caching if a cache has not been provided by the user. @joyeecheung what do you think?

    uint8_t* data =
244            static_cast<uint8_t*>(cached_data_buf->Buffer()->Data());
     	15. alloc_fn: Storage is returned from allocation function operator new.
     	16. var_assign: Assigning: cached_data = storage returned from new v8::ScriptCompiler::CachedData(data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength(), v8::ScriptCompiler::CachedData::BufferNotOwned).
245        cached_data =
246            new ScriptCompiler::CachedData(data + cached_data_buf->ByteOffset(),
247                                           cached_data_buf->ByteLength());
248      }
249
250      Local<String> source_text = args[2].As<String>();
251      ScriptOrigin origin(isolate,
252                          url,
253                          line_offset,
254                          column_offset,
255                          true,                             // is cross origin
256                          -1,                               // script id
257                          Local<Value>(),                   // source map URL
258                          false,                            // is opaque (?)
259                          false,                            // is WASM
260                          true,                             // is ES Module
261                          host_defined_options);
262
263      CompileCacheEntry* cache_entry = nullptr;
     	17. Condition can_use_builtin_cache, taking true branch.
     	18. Condition realm->env()->use_compile_cache(), taking true branch.
264      if (can_use_builtin_cache && realm->env()->use_compile_cache()) {
265        cache_entry = realm->env()->compile_cache_handler()->GetOrInsert(
266            source_text, url, CachedCodeType::kESM);
267      }
     	19. Condition cache_entry != NULL, taking true branch.
     	20. Condition cache_entry->cache != std::nullptr_t(), taking true branch.
268      if (cache_entry != nullptr && cache_entry->cache != nullptr) {
269        // source will take ownership of cached_data.
     	
CID 361813: (#1 of 1): Resource leak (RESOURCE_LEAK)
21. overwrite_var: Overwriting cached_data in cached_data = cache_entry->CopyCache() leaks the storage that cached_data points to.
270        cached_data = cache_entry->CopyCache();
271      }

[joyeecheung]

Does DCHECK_IMPLIES(can_use_builtin_cache , !args[5]->IsArrayBufferView()); make the warning go away? The two are mutually exclusive, so it should not be possible for the overwrite to happen, but since the values come from JS land maybe coverity cannot understand it.

Also used_cache_from_user will go away in #52413 though again, the mutual exclusiveness come from JS land and I am not sure if coverity can understand…it might be because we will use the exiting state of an std::optional that will be overwritten to decide instead of a boolean that’s deduced from JS land arguments?

[mhdawson]

@joyeecheung I don't have a way to run locally, so I can't answer if adding DCHECK_IMPLIES(can_use_builtin_cache , !args[5]->IsArrayBufferView()); would resolve other than PRing/landing.

If its guarded at the JS level so that it will not use the autocache if the user has provided a cache I can just mark the report as a false positive if we don't think guarding in the C++ code makes sense.

[joyeecheung]

If its guarded at the JS level so that it will not use the autocache if the user has provided a cache

Yes, the auto cache is not used from the user-accessible path. The binding is shared by both vm.Module constructors as well as our internal ESM loader. The latter passes a Symbol as the 6th argument to signify that it's okay to use the on-disk cache. The former passes an ArrayBufferView as the 6th argument to signify that users provide a cache (which is in that ArrayBufferView). So the mutual exclusiveness comes from how the JS invokes the binding. To overwrite it it means the binding's 6th argument is both a Symbol and an ArrayBufferView which is impossible (but that's JS semantics, probably not something coverity can understand).

[mhdawson]

@joyeecheung just to be 100% sure I should close this just mark the report as a false positive in coverity, right?

[joyeecheung]

Yes