chore: Remove more direct uses of free, replacing them with unique ptrs (#542)

Remove all but three direct uses of `JS_free`, replacing them with either `mozilla::UniquePtr`, or `std::vector`. The resulting pattern is that direct calls to `JS_free` are no longer needed, and functions that take ownership of memory now need to use calls to `.release()` on the unique pointers.

I also noticed that I missed a call-site to `fastly_http_req_uri_get` when converting over to the host_api, so I've updated that as well.

Author: elliottt

runtime/js-compute-runtime/builtins/crypto-algorithm.cpp

@@ -1,7 +1,9 @@
 #include "openssl/rsa.h"
 #include "openssl/sha.h"
 #include <iostream>
+#include <optional>
 #include <span>
+#include <vector>
 
 #include "crypto-algorithm.h"
 #include "crypto-key-rsa-components.h"
@@ -52,49 +54,47 @@ const EVP_MD *createDigestAlgorithm(JSContext *cx, JS::HandleObject key) {
 }
 // This implements https://w3c.github.io/webcrypto/#sha-operations for all
 // the SHA algorithms that we support.
-std::optional<std::span<uint8_t>> rawDigest(JSContext *cx, std::span<uint8_t> data,
-                                            const EVP_MD *algorithm, size_t buffer_size) {
+std::optional<std::vector<uint8_t>> rawDigest(JSContext *cx, std::span<uint8_t> data,
+                                              const EVP_MD *algorithm, size_t buffer_size) {
   unsigned int size;
-  auto buf = static_cast<unsigned char *>(JS_malloc(cx, buffer_size));
-  if (!buf) {
-    JS_ReportOutOfMemory(cx);
-    return std::nullopt;
-  }
-  if (!EVP_Digest(data.data(), data.size(), buf, &size, algorithm, NULL)) {
+  std::vector<uint8_t> buf(buffer_size, 0);
+  if (!EVP_Digest(data.data(), data.size(), buf.data(), &size, algorithm, NULL)) {
     // 2. If performing the operation results in an error, then throw an OperationError.
     // TODO: Change to an OperationError DOMException
     JS_ReportErrorUTF8(cx, "SubtleCrypto.digest: failed to create digest");
-    JS_free(cx, buf);
     return std::nullopt;
   }
-  return std::span<uint8_t>(buf, size);
+  return {std::move(buf)};
 };
 
 // This implements https://w3c.github.io/webcrypto/#sha-operations for all
 // the SHA algorithms that we support.
 JSObject *digest(JSContext *cx, std::span<uint8_t> data, const EVP_MD *algorithm,
                  size_t buffer_size) {
   unsigned int size;
-  auto buf = static_cast<unsigned char *>(JS_malloc(cx, buffer_size));
+  mozilla::UniquePtr<uint8_t[], JS::FreePolicy> buf{
+      static_cast<uint8_t *>(JS_malloc(cx, buffer_size))};
   if (!buf) {
     JS_ReportOutOfMemory(cx);
     return nullptr;
   }
-  if (!EVP_Digest(data.data(), data.size(), buf, &size, algorithm, NULL)) {
+  if (!EVP_Digest(data.data(), data.size(), buf.get(), &size, algorithm, NULL)) {
     // 2. If performing the operation results in an error, then throw an OperationError.
     // TODO: Change to an OperationError DOMException
     JS_ReportErrorUTF8(cx, "SubtleCrypto.digest: failed to create digest");
-    JS_free(cx, buf);
     return nullptr;
   }
   // 3. Return a new ArrayBuffer containing result.
   JS::RootedObject array_buffer(cx);
-  array_buffer.set(JS::NewArrayBufferWithContents(cx, size, buf));
+  array_buffer.set(JS::NewArrayBufferWithContents(cx, size, buf.get()));
   if (!array_buffer) {
-    JS_free(cx, buf);
     JS_ReportOutOfMemory(cx);
     return nullptr;
   }
+
+  // `array_buffer` now owns `buf`
+  static_cast<void>(buf.release());
+
   return array_buffer;
 };
 
@@ -603,13 +603,12 @@ JSObject *CryptoAlgorithmRSASSA_PKCS1_v1_5_Sign_Verify::sign(JSContext *cx, JS::
     return nullptr;
   }
 
-  auto digestOption = ::builtins::rawDigest(cx, data, algorithm, EVP_MD_size(algorithm));
-  if (!digestOption.has_value()) {
+  auto digest = ::builtins::rawDigest(cx, data, algorithm, EVP_MD_size(algorithm));
+  if (!digest.has_value()) {
     // TODO Rename error to OperationError
     JS_ReportErrorLatin1(cx, "OperationError");
     return nullptr;
   }
-  auto digest = digestOption.value();
 
   // 2. Perform the signature generation operation defined in Section 8.2 of [RFC3447] with the
   //  key represented by the [[handle]] internal slot of key as the signer's private key and the
@@ -643,24 +642,23 @@ JSObject *CryptoAlgorithmRSASSA_PKCS1_v1_5_Sign_Verify::sign(JSContext *cx, JS::
   }
 
   size_t signature_length;
-  if (EVP_PKEY_sign(ctx, nullptr, &signature_length, digest.data(), digest.size()) <= 0) {
+  if (EVP_PKEY_sign(ctx, nullptr, &signature_length, digest->data(), digest->size()) <= 0) {
     // TODO Rename error to OperationError
     JS_ReportErrorLatin1(cx, "OperationError");
     return nullptr;
   }
 
   // 4. Let signature be the value S that results from performing the operation.
-  uint8_t *signature = reinterpret_cast<uint8_t *>(JS_malloc(cx, signature_length));
-  if (EVP_PKEY_sign(ctx, signature, &signature_length, digest.data(), digest.size()) <= 0) {
+  mozilla::UniquePtr<uint8_t[], JS::FreePolicy> signature{static_cast<uint8_t *>(JS_malloc(cx, signature_length))};
+  if (EVP_PKEY_sign(ctx, signature.get(), &signature_length, digest->data(), digest->size()) <= 0) {
     // TODO Rename error to OperationError
     JS_ReportErrorLatin1(cx, "OperationError");
-    JS_free(cx, signature);
     return nullptr;
   }
 
   // 5. Return a new ArrayBuffer associated with the relevant global object of this [HTML], and
   // containing the bytes of signature.
-  JS::RootedObject buffer(cx, JS::NewArrayBufferWithContents(cx, signature_length, signature));
+  JS::RootedObject buffer(cx, JS::NewArrayBufferWithContents(cx, signature_length, signature.get()));
   if (!buffer) {
     // We can be here is the array buffer was too large -- if that was the case then a
     // JSMSG_BAD_ARRAY_LENGTH will have been created. No other failure scenarios in this path will
@@ -669,9 +667,12 @@ JSObject *CryptoAlgorithmRSASSA_PKCS1_v1_5_Sign_Verify::sign(JSContext *cx, JS::
       // TODO Rename error to InternalError
       JS_ReportErrorLatin1(cx, "InternalError");
     }
-    JS_free(cx, signature);
     return nullptr;
   }
+
+  // `signature` is now owned by `buffer`
+  static_cast<void>(signature.release());
+
   return buffer;
 }
 
@@ -1033,4 +1034,4 @@ JSObject *CryptoAlgorithmSHA512::digest(JSContext *cx, std::span<uint8_t> data)
   return ::builtins::digest(cx, data, EVP_sha512(), SHA512_DIGEST_LENGTH);
 }
 
-} // namespace builtins
+} // namespace builtins

runtime/js-compute-runtime/builtins/crypto-key.cpp

@@ -495,11 +495,12 @@ JSObject *CryptoKey::createRSA(JSContext *cx, CryptoAlgorithmRSASSA_PKCS1_v1_5_I
     return nullptr;
   }
 
-  uint8_t *p = reinterpret_cast<uint8_t *>(calloc(keyData->exponent.size(), sizeof(uint8_t)));
+  auto p = mozilla::MakeUnique<uint8_t[]>(keyData->exponent.size());
   auto exp = keyData->exponent;
-  std::copy(exp.begin(), exp.end(), p);
+  std::copy(exp.begin(), exp.end(), p.get());
 
-  JS::RootedObject buffer(cx, JS::NewArrayBufferWithContents(cx, keyData->exponent.size(), p));
+  JS::RootedObject buffer(cx,
+                          JS::NewArrayBufferWithContents(cx, keyData->exponent.size(), p.get()));
   // `buffer` takes ownership of `p` if the call to NewArrayBufferWithContents was successful
   // if the call was not successful, we need to free `p` before exiting from the function.
   if (!buffer) {
@@ -510,10 +511,12 @@ JSObject *CryptoKey::createRSA(JSContext *cx, CryptoAlgorithmRSASSA_PKCS1_v1_5_I
       // TODO Rename error to InternalError
       JS_ReportErrorLatin1(cx, "InternalError");
     }
-    JS_free(cx, p);
     return nullptr;
   }
 
+  // At this point, `buffer` owns the memory managed by `p`.
+  static_cast<void>(p.release());
+
   // Set the publicExponent attribute of algorithm to the BigInteger representation of the RSA
   // public exponent.
   JS::RootedObject byte_array(cx,

runtime/js-compute-runtime/builtins/request-response.cpp

@@ -380,12 +380,11 @@ bool RequestOrResponse::parse_body(JSContext *cx, JS::HandleObject self, JS::Uni
   JS::RootedValue result(cx);
 
   if constexpr (result_type == RequestOrResponse::BodyReadResult::ArrayBuffer) {
-    auto *rawBuf = buf.release();
-    JS::RootedObject array_buffer(cx, JS::NewArrayBufferWithContents(cx, len, rawBuf));
+    JS::RootedObject array_buffer(cx, JS::NewArrayBufferWithContents(cx, len, buf.get()));
     if (!array_buffer) {
-      JS_free(cx, rawBuf);
       return RejectPromiseWithPendingError(cx, result_promise);
     }
+    static_cast<void>(buf.release());
     result.setObject(*array_buffer);
   } else {
     JS::RootedString text(cx, JS_NewStringCopyUTF8N(cx, JS::UTF8Chars(buf.get(), len)));
@@ -505,7 +504,7 @@ bool RequestOrResponse::content_stream_read_then_handler(JSContext *cx, JS::Hand
       }
     }
 
-    auto buf = static_cast<char *>(JS_malloc(cx, buf_size + 1));
+    JS::UniqueChars buf{static_cast<char *>(JS_malloc(cx, buf_size + 1))};
     if (!buf) {
       JS_ReportOutOfMemory(cx);
       return false;
@@ -516,7 +515,6 @@ bool RequestOrResponse::content_stream_read_then_handler(JSContext *cx, JS::Hand
     for (uint32_t index = 0; index < contentsLength; index++) {
       JS::RootedValue val(cx);
       if (!JS_GetElement(cx, contents, index, &val)) {
-        JS_free(cx, buf);
         return false;
       }
       {
@@ -529,7 +527,7 @@ bool RequestOrResponse::content_stream_read_then_handler(JSContext *cx, JS::Hand
         if (length) {
           static_assert(CHAR_BIT == 8, "Strange char");
           auto bytes = reinterpret_cast<char *>(JS_GetUint8ArrayData(array, &is_shared, nogc));
-          memcpy(buf + offset, bytes, length);
+          memcpy(buf.get() + offset, bytes, length);
           offset += length;
         }
       }
@@ -538,20 +536,17 @@ bool RequestOrResponse::content_stream_read_then_handler(JSContext *cx, JS::Hand
 #ifdef DEBUG
     bool foundBodyParser;
     if (!JS_HasElement(cx, catch_handler, 2, &foundBodyParser)) {
-      JS_free(cx, buf);
       return false;
     }
     MOZ_ASSERT(foundBodyParser);
 #endif
     // Now we can call parse_body on the result
     JS::RootedValue body_parser(cx);
     if (!JS_GetElement(cx, catch_handler, 2, &body_parser)) {
-      JS_free(cx, buf);
       return false;
     }
     auto parse_body = (ParseBodyCB *)body_parser.toPrivate();
-    JS::UniqueChars body(buf);
-    return parse_body(cx, self, std::move(body), offset);
+    return parse_body(cx, self, std::move(buf), offset);
   }
 
   JS::RootedValue val(cx);

runtime/js-compute-runtime/builtins/shared/text-encoder.cpp

@@ -29,14 +29,14 @@ bool TextEncoder::encode(JSContext *cx, unsigned argc, JS::Value *vp) {
 
   size_t chars_len;
   JS::UniqueChars chars = ::encode(cx, args[0], &chars_len);
-
-  auto *rawChars = chars.release();
-  JS::RootedObject buffer(cx, JS::NewArrayBufferWithContents(cx, chars_len, rawChars));
+  JS::RootedObject buffer(cx, JS::NewArrayBufferWithContents(cx, chars_len, chars.get()));
   if (!buffer) {
-    JS_free(cx, rawChars);
     return false;
   }
 
+  // `buffer` now owns `chars`
+  static_cast<void>(chars.release());
+
   JS::RootedObject byte_array(cx, JS_NewUint8ArrayWithBuffer(cx, buffer, 0, chars_len));
   if (!byte_array) {
     return false;

runtime/js-compute-runtime/js-compute-builtins.cpp

@@ -788,17 +788,15 @@ bool fetch(JSContext *cx, unsigned argc, Value *vp) {
       if (!backend) {
         auto handle = builtins::Request::request_handle(request);
 
-        fastly_world_string_t uri_str;
-        fastly_error_t err;
-        if (fastly_http_req_uri_get(handle.handle, &uri_str, &err)) {
+        auto res = handle.get_uri();
+        if (auto *err = res.to_err()) {
+          HANDLE_ERROR(cx, *err);
+        } else {
           JS_ReportErrorLatin1(cx,
                                "No backend specified for request with url %s. "
                                "Must provide a `backend` property on the `init` object "
                                "passed to either `new Request()` or `fetch`",
-                               uri_str.ptr);
-          JS_free(cx, uri_str.ptr);
-        } else {
-          HANDLE_ERROR(cx, err);
+                               res.unwrap().begin());
         }
         return ReturnPromiseRejectedWithPendingError(cx, args);
       }