Address review comments

oss-internship-2020/curl/CMakeLists.txt

@@ -22,19 +22,11 @@ set(CMAKE_CXX_STANDARD_REQUIRED True)
 option(CURL_SAPI_ENABLE_EXAMPLES "" ON)
 option(CURL_SAPI_ENABLE_TESTS "" ON)
 
-# Add callbacks used by examples
-if (CURL_SAPI_ENABLE_EXAMPLES)
+# Add callbacks used by examples and tests
+if (CURL_SAPI_ENABLE_EXAMPLES OR CURL_SAPI_ENABLE_TESTS)
   list(APPEND CURL_SAPI_CALLBACKS 
-    "${CMAKE_CURRENT_SOURCE_DIR}/examples/callbacks.h"
-    "${CMAKE_CURRENT_SOURCE_DIR}/examples/callbacks.cc"
-  )
-endif()
-
-# Add callbacks used by tests
-if (CURL_SAPI_ENABLE_TESTS)
-  list(APPEND CURL_SAPI_CALLBACKS 
-    "${CMAKE_CURRENT_SOURCE_DIR}/tests/callbacks.h"
-    "${CMAKE_CURRENT_SOURCE_DIR}/tests/callbacks.cc"
+    "${CMAKE_CURRENT_SOURCE_DIR}/callbacks/callbacks.h"
+    "${CMAKE_CURRENT_SOURCE_DIR}/callbacks/callbacks.cc"
   )
 endif()
 

oss-internship-2020/curl/README.md

@@ -75,8 +75,6 @@ The `examples` directory contains the sandboxed versions of example source codes
 
 To build these examples when building the library, the cmake variable `CURL_SAPI_ENABLE_EXAMPLES` must be set to `ON`. This enables Sandboxed API examples as well.
 
-The `callbacks.h` and `callbacks.cc` files implement all the callbacks used by the examples.
-
 ## Policy
 
 The `sandbox.h` file contains a policy allowing all is necessary for libcurl to perform simple requests. It is used by all the examples, except by example3. This example needs some additional policies and files in its namespace (since it uses HTTPS), and the file `example3.cc` shows how to easily extend an existing policy.
@@ -85,4 +83,8 @@ The `sandbox.h` file contains a policy allowing all is necessary for libcurl to
 
 The `tests` folder contains some test cases created using Google Test. The class `CurlTestUtils` is used to facilitate some tasks that all test cases need, including the setup of a mock local server on which test requests are performed.
 
-To build these tests when building the library, the cmake variable `CURL_SAPI_ENABLE_TESTS` must be set to `ON`. This enables Sandboxed API tests as well.
+To build these tests when building the library, the cmake variable `CURL_SAPI_ENABLE_TESTS` must be set to `ON`. This enables Sandboxed API tests as well.
+
+## Callbacks
+
+The `callbacks.h` and `callbacks.cc` files implement all the callbacks used by examples and tests.

oss-internship-2020/curl/callbacks/callbacks.cc

@@ -19,8 +19,8 @@
 
 #include "sandboxed_api/vars.h"
 
-size_t WriteToMemoryTests(char* contents, size_t size, size_t num_bytes,
-                          void* userp) {
+size_t WriteToMemory(char* contents, size_t size, size_t num_bytes,
+                     void* userp) {
   size_t real_size = size * num_bytes;
   auto* mem = static_cast<sapi::LenValStruct*>(userp);
 

oss-internship-2020/curl/callbacks/callbacks.h

@@ -17,8 +17,8 @@
 
 #include <curl/curl.h>
 
-// Append contents to the string stored by userp (userp is a MemoryStruct*)
-extern "C" size_t WriteToMemoryTests(char* contents, size_t size,
-                                     size_t num_bytes, void* userp);
+// Append contents to the string stored by userp, which is a sapi::LenValStruct*
+extern "C" size_t WriteToMemory(char* contents, size_t size, size_t num_bytes,
+                                void* userp);
 
 #endif  // TESTS_CALLBACKS_H

oss-internship-2020/curl/curl_wrapper/curl_wrapper.cc

@@ -14,9 +14,6 @@
 
 #include "curl_wrapper.h"
 
-#include <cstdlib>
-#include <iostream>
-
 CURLcode curl_easy_setopt_ptr(CURL* handle, CURLoption option,
                               void* parameter) {
   return curl_easy_setopt(handle, option, parameter);

oss-internship-2020/curl/examples/callbacks.cc

@@ -1,42 +0,0 @@
-// Copyright 2020 Google LLC
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-#include "callbacks.h"
-
-#include <cstdlib>
-#include <cstring>
-#include <iostream>
-
-#include "sandboxed_api/vars.h"
-
-// Function taken from curl's getinmemory.c
-size_t WriteMemoryCallback(void* contents, size_t size, size_t nmemb,
-                           void* userp) {
-  size_t real_size = size * nmemb;
-  auto* mem = static_cast<sapi::LenValStruct*>(userp);
-
-  char* ptr = static_cast<char*>(realloc(mem->data, mem->size + real_size + 1));
-  if (ptr == nullptr) {  // Out of memory
-    std::cout << "not enough memory (realloc returned NULL)\n";
-    return 0;
-  }
-
-  mem->data = ptr;
-  auto data = static_cast<char*>(mem->data);
-  memcpy(&(data[mem->size]), contents, real_size);
-  mem->size += real_size;
-  data[mem->size] = 0;
-
-  return real_size;
-}

oss-internship-2020/curl/examples/callbacks.h

@@ -1,23 +0,0 @@
-// Copyright 2020 Google LLC
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-#ifndef EXAMPLES_CALLBACKS_H
-#define EXAMPLES_CALLBACKS_H
-
-#include <curl/curl.h>
-
-extern "C" size_t WriteMemoryCallback(void* contents, size_t size, size_t nmemb,
-                                      void* userp);
-
-#endif  // EXAMPLES_CALLBACKS_H

oss-internship-2020/curl/examples/example2.cc

@@ -20,11 +20,6 @@
 
 #include "../sandbox.h"
 
-struct MemoryStruct {
-  char* memory;
-  size_t size;
-};
-
 int main(int argc, char* argv[]) {
   gflags::ParseCommandLineFlags(&argc, &argv, true);
   google::InitGoogleLogging(argv[0]);
@@ -41,7 +36,7 @@ int main(int argc, char* argv[]) {
 
   // Generate pointer to WriteMemoryCallback function
   void* function_ptr;
-  status = sandbox.rpc_channel()->Symbol("WriteMemoryCallback", &function_ptr);
+  status = sandbox.rpc_channel()->Symbol("WriteToMemory", &function_ptr);
   if (!status.ok()) {
     LOG(FATAL) << "sapi::Sandbox::rpc_channel().Symbol failed: " << status;
   }
@@ -74,7 +69,7 @@ int main(int argc, char* argv[]) {
   }
 
   // Pass 'chunk' struct to the callback function
-  sapi::v::Struct<MemoryStruct> chunk;
+  sapi::v::LenVal chunk(0);
   curl_code =
       api.curl_easy_setopt_ptr(&curl, CURLOPT_WRITEDATA, chunk.PtrBoth());
   if (!curl_code.ok() || curl_code.value() != CURLE_OK) {
@@ -96,13 +91,11 @@ int main(int argc, char* argv[]) {
   }
 
   // Retrieve memory size
-  sapi::v::Int size;
-  size.SetRemote(&(static_cast<sapi::LenValStruct*>(chunk.GetRemote()))->size);
-  status = sandbox.TransferFromSandboxee(&size);
+  status = sandbox.TransferFromSandboxee(&chunk);
   if (!status.ok()) {
     LOG(FATAL) << "sandbox.TransferFromSandboxee failed: " << status;
   }
-  std::cout << "memory size: " << size.GetValue() << " bytes" << std::endl;
+  std::cout << "memory size: " << chunk.GetDataSize() << " bytes" << std::endl;
 
   // Cleanup curl
   status = api.curl_easy_cleanup(&curl);

oss-internship-2020/curl/tests/test_utils.cc

@@ -14,6 +14,7 @@
 
 #include "test_utils.h"
 
+#include <absl/strings/match.h>
 #include <fcntl.h>
 #include <netdb.h>
 #include <netinet/in.h>
@@ -40,7 +41,8 @@ absl::Status CurlTestUtils::CurlTestSetUp() {
   absl::StatusOr<CURL*> curl_handle = api_->curl_easy_init();
   if (!curl_handle.ok()) {
     return curl_handle.status();
-  } else if (!curl_handle.value()) {
+  }
+  if (!curl_handle.value()) {
     return absl::UnavailableError("curl_easy_init returned NULL ");
   }
   curl_ = std::make_unique<sapi::v::RemotePtr>(curl_handle.value());
@@ -53,7 +55,8 @@ absl::Status CurlTestUtils::CurlTestSetUp() {
                                          sapi_url.PtrBefore());
   if (!curl_code.ok()) {
     return curl_code.status();
-  } else if (curl_code.value() != CURLE_OK) {
+  }
+  if (curl_code.value() != CURLE_OK) {
     return absl::UnavailableError(
         "curl_easy_setopt_ptr returned with the error code " +
         curl_code.value());
@@ -63,7 +66,8 @@ absl::Status CurlTestUtils::CurlTestSetUp() {
   curl_code = api_->curl_easy_setopt_long(curl_.get(), CURLOPT_PORT, port_);
   if (!curl_code.ok()) {
     return curl_code.status();
-  } else if (curl_code.value() != CURLE_OK) {
+  }
+  if (curl_code.value() != CURLE_OK) {
     return absl::UnavailableError(
         "curl_easy_setopt_long returned with the error code " +
         curl_code.value());
@@ -72,7 +76,7 @@ absl::Status CurlTestUtils::CurlTestSetUp() {
   // Generate pointer to the WriteToMemory callback
   void* function_ptr;
   absl::Status symbol =
-      sandbox_->rpc_channel()->Symbol("WriteToMemoryTests", &function_ptr);
+      sandbox_->rpc_channel()->Symbol("WriteToMemory", &function_ptr);
   if (!symbol.ok()) {
     return symbol;
   }
@@ -83,18 +87,21 @@ absl::Status CurlTestUtils::CurlTestSetUp() {
                                          &remote_function_ptr);
   if (!curl_code.ok()) {
     return curl_code.status();
-  } else if (curl_code.value() != CURLE_OK) {
+  }
+  if (curl_code.value() != CURLE_OK) {
     return absl::UnavailableError(
         "curl_easy_setopt_ptr returned with the error code " +
         curl_code.value());
   }
 
   // Pass memory chunk object to the callback
+  chunk_ = std::make_unique<sapi::v::LenVal>(0);
   curl_code = api_->curl_easy_setopt_ptr(curl_.get(), CURLOPT_WRITEDATA,
-                                         chunk_.PtrBoth());
+                                         chunk_->PtrBoth());
   if (!curl_code.ok()) {
     return curl_code.status();
-  } else if (curl_code.value() != CURLE_OK) {
+  }
+  if (curl_code.value() != CURLE_OK) {
     return absl::UnavailableError(
         "curl_easy_setopt_ptr returned with the error code " +
         curl_code.value());
@@ -113,133 +120,101 @@ absl::StatusOr<std::string> CurlTestUtils::PerformRequest() {
   absl::StatusOr<int> curl_code = api_->curl_easy_perform(curl_.get());
   if (!curl_code.ok()) {
     return curl_code.status();
-  } else if (curl_code.value() != CURLE_OK) {
+  }
+  if (curl_code.value() != CURLE_OK) {
     return absl::UnavailableError(
         "curl_easy_perform returned with the error code " + curl_code.value());
   }
 
   // Get pointer to the memory chunk
-  sapi::v::GenericPtr remote_ptr;
-  remote_ptr.SetRemote(
-      &(static_cast<sapi::LenValStruct*>(chunk_.GetRemote()))->data);
-  absl::Status transfer = sandbox_->TransferFromSandboxee(&remote_ptr);
-  if (!transfer.ok()) {
-    return transfer;
-  }
-  void* chunk_ptr = (void*)remote_ptr.GetValue();
-
-  // Get the string
-  absl::StatusOr<std::string> response =
-      sandbox_->GetCString(sapi::v::RemotePtr(chunk_ptr));
-  if (!response.ok()) {
-    return response.status();
+  absl::Status status = sandbox_->TransferFromSandboxee(chunk_.get());
+  if (!status.ok()) {
+    return status;
   }
 
-  return response.value();
+  return std::string{reinterpret_cast<char*>(chunk_->GetData())};
 }
 
+namespace {
+
 // Read the socket until str is completely read
 std::string ReadUntil(const int socket, const std::string& str,
                       const size_t max_request_size) {
-  char buf[max_request_size] = {};
-  size_t read_bytes = 0;
+  std::string str_read;
+  str_read.reserve(max_request_size);
 
   // Read one char at a time until str is suffix of buf
-  do {
-    if (read_bytes >= max_request_size ||
-        read(socket, buf + read_bytes, 1) < 1) {
+  while (!absl::EndsWith(str_read, str)) {
+    char next_char;
+    if (str_read.size() >= max_request_size ||
+        read(socket, &next_char, 1) < 1) {
       return "";
     }
-    ++read_bytes;
-  } while (read_bytes < str.size() ||
-           std::string{buf + read_bytes - str.size()} != str);
+    str_read += next_char;
+  }
 
-  buf[read_bytes] = '\0';
-  return std::string{buf};
+  return str_read;
 }
 
 // Parse HTTP headers to return the Content-Length
 ssize_t GetContentLength(const std::string& headers) {
   // Find the Content-Length header
-  const char* length_header_start = strstr(headers.c_str(), "Content-Length: ");
+  std::string::size_type length_header_start = headers.find("Content-Length: ");
 
   // There is no Content-Length field
-  if (!length_header_start) {
+  if (length_header_start == std::string::npos) {
     return 0;
   }
 
   // Find Content-Length string
-  const char* length_start = length_header_start + strlen("Content-Length: ");
-  size_t length_bytes = strstr(length_start, "\r\n") - length_start;
+  std::string::size_type length_start =
+      length_header_start + std::string{"Content-Length: "}.size();
+  std::string::size_type length_bytes =
+      headers.find("\r\n", length_start) - length_start;
+
+  // length_bytes exceeds maximum
   if (length_bytes >= 64) {
     return -1;
   }
 
-  // Convert Content-Length string value to int
-  char content_length_string[64];
-  strncpy(content_length_string, length_start, length_bytes);
-
-  return atoi(content_length_string);
+  // Convert string to int and return
+  return std::stoi(headers.substr(length_start, length_bytes));
 }
 
 // Read exactly content_bytes from the socket
 std::string ReadExact(int socket, size_t content_bytes) {
-  char buf[content_bytes + 1] = {};
-  size_t read_bytes = 0;
+  std::string str_read;
+  str_read.reserve(content_bytes);
 
-  // Read until content_bytes chars are read
-  do {
-    int num_bytes;
-    if ((num_bytes = read(socket, buf + read_bytes,
-                          sizeof(buf) - read_bytes - 1)) < 1) {
+  // Read one char at a time until all chars are read
+  while (str_read.size() < content_bytes) {
+    char next_char;
+    if (read(socket, &next_char, 1) < 1) {
       return "";
     }
-    read_bytes += num_bytes;
-  } while (read_bytes < content_bytes);
+    str_read += next_char;
+  }
 
-  buf[content_bytes] = '\0';
-  return std::string{buf};
+  return str_read;
 }
 
-void CurlTestUtils::StartMockServer() {
-  // Get the socket file descriptor
-  int listening_socket = socket(AF_INET, SOCK_STREAM, 0);
-
-  // Create the socket address object
-  // The port is set to 0, meaning that it will be auto assigned
-  // Only local connections can access this socket
-  sockaddr_in socket_address{AF_INET, 0, htonl(INADDR_LOOPBACK)};
+// Listen on the socket and answer back to requests
+void ServerLoop(int listening_socket, sockaddr_in socket_address) {
   socklen_t socket_address_size = sizeof(socket_address);
-  if (listening_socket == -1) {
+
+  // Listen on the socket (maximum 1 connection)
+  if (listen(listening_socket, 1) == -1) {
     return;
   }
 
-  // Bind the file descriptor to the socket address object
-  if (bind(listening_socket, (sockaddr*)&socket_address, socket_address_size) ==
-      -1) {
-    return;
-  }
-
-  // Assign an available port to the socket address object
-  if (getsockname(listening_socket, (sockaddr*)&socket_address,
-                  &socket_address_size) == -1) {
-    return;
-  }
-
-  // Get the port number
-  port_ = ntohs(socket_address.sin_port);
-
-  // Set server_thread_ operation to socket listening
-  server_thread_ = std::thread([=] {
-    // Listen on the socket (maximum 1 connection)
-    if (listen(listening_socket, 1) == -1) {
-      return;
-    }
-
+  // Keep accepting connections until the thread is terminated
+  // (i.e. server_thread_ is assigned to a new thread or destroyed)
+  for (;;) {
     // File descriptor to the connection socket
     // This blocks the thread until a connection is established
-    int accepted_socket = accept(listening_socket, (sockaddr*)&socket_address,
-                                 (socklen_t*)&socket_address_size);
+    int accepted_socket =
+        accept(listening_socket, reinterpret_cast<sockaddr*>(&socket_address),
+               reinterpret_cast<socklen_t*>(&socket_address_size));
     if (accepted_socket == -1) {
       return;
     }
@@ -271,8 +246,7 @@ void CurlTestUtils::StartMockServer() {
         "HTTP/1.1 200 OK\nContent-Type: text/plain\nContent-Length: ";
 
     if (headers.substr(0, 3) == "GET") {
-      http_response += std::to_string(kSimpleResponse.size()) + "\r\n\r\n" +
-                       std::string{kSimpleResponse};
+      http_response += "2\r\n\r\nOK";
 
     } else if (headers.substr(0, 4) == "POST") {
       http_response +=
@@ -288,5 +262,40 @@ void CurlTestUtils::StartMockServer() {
 
     // Close the socket
     close(accepted_socket);
-  });
+  }
+}
+
+}  // namespace
+
+void CurlTestUtils::StartMockServer() {
+  // Get the socket file descriptor
+  int listening_socket = socket(AF_INET, SOCK_STREAM, 0);
+
+  // Create the socket address object
+  // The port is set to 0, meaning that it will be auto assigned
+  // Only local connections can access this socket
+  sockaddr_in socket_address{AF_INET, 0, htonl(INADDR_LOOPBACK)};
+  socklen_t socket_address_size = sizeof(socket_address);
+  if (listening_socket == -1) {
+    return;
+  }
+
+  // Bind the file descriptor to the socket address object
+  if (bind(listening_socket, reinterpret_cast<sockaddr*>(&socket_address),
+           socket_address_size) == -1) {
+    return;
+  }
+
+  // Assign an available port to the socket address object
+  if (getsockname(listening_socket,
+                  reinterpret_cast<sockaddr*>(&socket_address),
+                  &socket_address_size) == -1) {
+    return;
+  }
+
+  // Get the port number
+  port_ = ntohs(socket_address.sin_port);
+
+  // Set server_thread_ operation to socket listening
+  server_thread_ = std::thread(ServerLoop, listening_socket, socket_address);
 }

oss-internship-2020/curl/tests/test_utils.h

@@ -24,11 +24,6 @@
 // Helper class that can be used to test Curl Sandboxed
 class CurlTestUtils {
  protected:
-  struct MemoryStruct {
-    char* memory;
-    size_t size;
-  };
-
   // Initialize and set up the curl handle
   absl::Status CurlTestSetUp();
   // Clean up the curl handle
@@ -40,7 +35,7 @@ class CurlTestUtils {
   // Start a mock server (only once) that will manage connections for the tests
   // The server listens on a port asynchronously by creating a thread
   // The port number is stored in port_
-  // Responds with kSimpleResponse to a GET request
+  // Responds with "OK" to a GET request
   // Responds with the POST request fields to a POST request
   static void StartMockServer();
 
@@ -53,10 +48,8 @@ class CurlTestUtils {
   static constexpr absl::string_view kUrl = "http://127.0.0.1/";
   static long port_;
 
-  static constexpr absl::string_view kSimpleResponse = "OK";
-
  private:
-  sapi::v::LenVal chunk_{0};
+  std::unique_ptr<sapi::v::LenVal> chunk_;
 };
 
 #endif  // TESTS_H

oss-internship-2020/curl/tests/tests.cc

@@ -108,7 +108,7 @@ TEST_F(CurlTest, GETResponse) {
   SAPI_ASSERT_OK_AND_ASSIGN(std::string response, PerformRequest());
 
   // Compare response with expected response
-  ASSERT_EQ(response, kSimpleResponse);
+  ASSERT_EQ(response, "OK");
 }
 
 TEST_F(CurlTest, POSTResponse) {