address memory leaks in awshttp

There appears to be a leak in json.zig, which had minimal modifications for
snake/camel case, including use of an allocator. It is not obvious that
the leaks were added by these changes, so I will leave this issue for
later investigation.

README.md

@@ -95,15 +95,14 @@ TODO List:
 * ✓ Implement generic response body -> Response type handling (right now, this is hard-coded)
 * ✓ Implement codegen for services with xml structures (using Smithy models)
 * ✓ Implement codegen for others (using Smithy models)
-* Switch to aws-c-cal upsream once [PR for full static musl build support is merged](https://github.com/awslabs/aws-c-cal/pull/89) (see Dockerfile)
+* Switch to aws-c-cal upstream once [PR for full static musl build support is merged](https://github.com/awslabs/aws-c-cal/pull/89) (see Dockerfile)
 * Remove compiler 0.7.1 shims when 0.8.0 is released
 (new 2021-05-29. I will proceed in this order unless I get other requests)
-* Implement [AWS query protocol](https://awslabs.github.io/smithy/1.0/spec/aws/aws-query-protocol.html). This is the protocol in use by sts.getcalleridentity
-* Implement [AWS Json 1.0 protocol](https://awslabs.github.io/smithy/1.0/spec/aws/aws-json-1_0-protocol.html)
-* Implement [AWS Json 1.1 protocol](https://awslabs.github.io/smithy/1.0/spec/aws/aws-json-1_1-protocol.html)
-* Implement [AWS restXml protocol](https://awslabs.github.io/smithy/1.0/spec/aws/aws-restxml-protocol.html)
-* Implement [AWS EC2 query protocol](https://awslabs.github.io/smithy/1.0/spec/aws/aws-ec2-query-protocol.html)
-
+* ✓ Implement [AWS query protocol](https://awslabs.github.io/smithy/1.0/spec/aws/aws-query-protocol.html). This is the protocol in use by sts.getcalleridentity. Total service count 18
+* Implement [AWS Json 1.0 protocol](https://awslabs.github.io/smithy/1.0/spec/aws/aws-json-1_0-protocol.html). Includes dynamodb. Total service count 18
+* Implement [AWS Json 1.1 protocol](https://awslabs.github.io/smithy/1.0/spec/aws/aws-json-1_1-protocol.html). Includes ecs. Total service count 105
+* Implement [AWS restXml protocol](https://awslabs.github.io/smithy/1.0/spec/aws/aws-restxml-protocol.html). Includes S3. Total service count 4.
+* ✓ Implement [AWS EC2 query protocol](https://awslabs.github.io/smithy/1.0/spec/aws/aws-ec2-query-protocol.html). Includes EC2. Total service count 1.
 
 Compiler wishlist/watchlist:
 

src/aws.zig

@@ -55,22 +55,32 @@ pub const Aws = struct {
         });
         log.debug("proto: {s}", .{service.aws_protocol});
 
+        // It seems as though there are 3 major branches of the 6 protocols.
+        // 1. query/ec2_query, which are identical until you get to complex
+        //    structures. TBD if the shortcut we're taking for query to make
+        //    it return json will work on EC2, but my guess is yes.
+        // 2. *json*: These three appear identical for input (possible difference
+        //    for empty body serialization), but differ in error handling.
+        //    We're not doing a lot of error handling here, though.
+        // 3. rest_xml: This is a one-off for S3, never used since
         switch (service.aws_protocol) {
-            .query => return self.callQuery(request, service, action, options),
-            .ec2_query => @compileError("EC2 Query protocol not yet supported"),
-            .rest_json_1 => @compileError("REST Json 1 protocol not yet supported"),
-            .json_1_0 => @compileError("Json 1.0 protocol not yet supported"),
-            .json_1_1 => @compileError("Json 1.1 protocol not yet supported"),
+            .query, .ec2_query => return self.callQuery(request, service, action, options),
+            .rest_json_1, .json_1_0, .json_1_1 => @compileError("REST Json, Json 1.0/1.1 protocol not yet supported"),
             .rest_xml => @compileError("REST XML protocol not yet supported"),
         }
     }
 
     // Call using query protocol. This is documented as an XML protocol, but
-    // throwing a JSON accept header seems to work
+    // throwing a JSON accept header seems to work. EC2Query is very simliar to
+    // Query, so we'll handle both here. Realistically we probably don't effectively
+    // handle lists and maps properly anyway yet, so we'll go for it and see
+    // where it breaks. PRs and/or failing test cases appreciated.
     fn callQuery(self: Self, comptime request: anytype, service: anytype, action: anytype, options: Options) !FullResponse(request) {
         var buffer = std.ArrayList(u8).init(self.allocator);
         defer buffer.deinit();
         const writer = buffer.writer();
+        // TODO: transformation function should be refactored for operation
+        // with a Writer passed in so we don't have to allocate
         const transformer = struct {
             allocator: *std.mem.Allocator,
 
@@ -98,6 +108,7 @@ pub const Aws = struct {
                 .sigv4_service_name = service.sigv4_name,
             },
         );
+        // TODO: Can response handling be reused?
         defer response.deinit();
         if (response.response_code != 200) {
             log.err("call failed! return status: {d}", .{response.response_code});

src/awshttp.zig

@@ -77,7 +77,10 @@ const SigningOptions = struct {
 const HttpResult = struct {
     response_code: u16, // actually 3 digits can fit in u10
     body: []const u8,
+    allocator: *std.mem.Allocator,
+
     pub fn deinit(self: HttpResult) void {
+        self.allocator.free(self.body);
         httplog.debug("http result deinit complete", .{});
         return;
     }
@@ -263,6 +266,14 @@ pub const AwsHttp = struct {
     /// HttpResult currently contains the body only. The addition of Headers
     /// and return code would be a relatively minor change
     pub fn makeRequest(self: Self, endpoint: EndPoint, method: []const u8, path: []const u8, body: []const u8, signing_options: ?SigningOptions) !HttpResult {
+        // Since we're going to pass these into C-land, we need to make sure
+        // our inputs have sentinals
+        const method_z = try self.allocator.dupeZ(u8, method);
+        defer self.allocator.free(method_z);
+        const path_z = try self.allocator.dupeZ(u8, path);
+        defer self.allocator.free(path_z);
+        const body_z = try self.allocator.dupeZ(u8, body);
+        defer self.allocator.free(body_z);
         // TODO: Try to re-encapsulate this
         // var http_request = try createRequest(method, path, body);
 
@@ -270,14 +281,14 @@ pub const AwsHttp = struct {
         var http_request = c.aws_http_message_new_request(c_allocator);
         defer c.aws_http_message_release(http_request);
 
-        if (c.aws_http_message_set_request_method(http_request, c.aws_byte_cursor_from_c_str(@ptrCast([*c]const u8, method))) != c.AWS_OP_SUCCESS)
+        if (c.aws_http_message_set_request_method(http_request, c.aws_byte_cursor_from_c_str(@ptrCast([*c]const u8, method_z))) != c.AWS_OP_SUCCESS)
             return AwsError.SetRequestMethodError;
 
-        if (c.aws_http_message_set_request_path(http_request, c.aws_byte_cursor_from_c_str(@ptrCast([*c]const u8, path))) != c.AWS_OP_SUCCESS)
+        if (c.aws_http_message_set_request_path(http_request, c.aws_byte_cursor_from_c_str(@ptrCast([*c]const u8, path_z))) != c.AWS_OP_SUCCESS)
             return AwsError.SetRequestPathError;
 
         httplog.debug("body length: {d}", .{body.len});
-        const body_cursor = c.aws_byte_cursor_from_c_str(@ptrCast([*c]const u8, body));
+        const body_cursor = c.aws_byte_cursor_from_c_str(@ptrCast([*c]const u8, body_z));
         const request_body = c.aws_input_stream_new_from_cursor(c_allocator, &body_cursor);
         defer c.aws_input_stream_destroy(request_body);
         if (body.len > 0) {
@@ -290,8 +301,9 @@ pub const AwsHttp = struct {
         var context = RequestContext{
             .allocator = self.allocator,
         };
+        defer context.deinit();
         var tls_connection_options: ?*c.aws_tls_connection_options = null;
-        const host = try std.fmt.allocPrint(self.allocator, "{s}", .{endpoint.host});
+        const host = try self.allocator.dupeZ(u8, endpoint.host);
         defer self.allocator.free(host);
         try self.addHeaders(http_request.?, host, body);
         if (std.mem.eql(u8, endpoint.scheme, "https")) {
@@ -434,6 +446,7 @@ pub const AwsHttp = struct {
         const rc = HttpResult{
             .response_code = context.response_code.?,
             .body = final_body,
+            .allocator = self.allocator,
         };
         return rc;
     }
@@ -535,9 +548,9 @@ pub const AwsHttp = struct {
         }
         defer c.aws_signable_destroy(signable);
 
-        const signing_region = try std.fmt.allocPrint(self.allocator, "{s}", .{options.region});
+        const signing_region = try std.fmt.allocPrintZ(self.allocator, "{s}", .{options.region});
         defer self.allocator.free(signing_region);
-        const signing_service = try std.fmt.allocPrint(self.allocator, "{s}", .{options.service});
+        const signing_service = try std.fmt.allocPrintZ(self.allocator, "{s}", .{options.service});
         defer self.allocator.free(signing_service);
         const temp_signing_config = c.bitfield_workaround_aws_signing_config_aws{
             .algorithm = .AWS_SIGNING_ALGORITHM_V4,
@@ -647,7 +660,7 @@ pub const AwsHttp = struct {
             return AwsError.AddHeaderError;
 
         if (body.len > 0) {
-            const len = try std.fmt.allocPrint(self.allocator, "{d}", .{body.len});
+            const len = try std.fmt.allocPrintZ(self.allocator, "{d}", .{body.len});
             // This defer seems to work ok, but I'm a bit concerned about why
             defer self.allocator.free(len);
             const content_length_header = c.aws_http_header{
@@ -833,7 +846,7 @@ fn fullCast(comptime T: type, val: anytype) T {
 fn regionSubDomain(allocator: *std.mem.Allocator, service: []const u8, region: []const u8, useDualStack: bool) !EndPoint {
     const environment_override = std.os.getenv("AWS_ENDPOINT_URL");
     if (environment_override) |override| {
-        const uri = try std.fmt.allocPrint(allocator, "{s}", .{override});
+        const uri = try allocator.dupeZ(u8, override);
         return endPointFromUri(allocator, uri);
     }
     // Fallback to us-east-1 if global endpoint does not exist.
@@ -847,7 +860,7 @@ fn regionSubDomain(allocator: *std.mem.Allocator, service: []const u8, region: [
         else => "amazonaws.com",
     };
 
-    const uri = try std.fmt.allocPrint(allocator, "https://{s}{s}.{s}.{s}", .{ service, dualstack, realregion, domain });
+    const uri = try std.fmt.allocPrintZ(allocator, "https://{s}{s}.{s}.{s}", .{ service, dualstack, realregion, domain });
     const host = uri["https://".len..];
     httplog.debug("host: {s}, scheme: {s}, port: {}", .{ host, "https", 443 });
     return EndPoint{
@@ -921,15 +934,16 @@ const RequestContext = struct {
     const Self = @This();
 
     pub fn deinit(self: Self) void {
-        self.allocator.free(self.body);
+        // We're going to leave it to the caller to free the body
+        // if (self.body) |b| self.allocator.free(b);
         if (self.headers) |hs| {
-            for (hs) |h| {
+            for (hs.items) |h| {
                 // deallocate the copied values
                 self.allocator.free(h.name);
                 self.allocator.free(h.value);
             }
             // deallocate the structure itself
-            h.deinit();
+            hs.deinit();
         }
     }
 
@@ -937,11 +951,11 @@ const RequestContext = struct {
         var orig_body: []const u8 = "";
         if (self.body) |b| {
             orig_body = try self.allocator.dupeZ(u8, b);
-            self.allocator.free(self.body.?);
+            self.allocator.free(b);
             self.body = null;
         }
         defer self.allocator.free(orig_body);
-        self.body = try std.fmt.allocPrint(self.allocator, "{s}{s}", .{ orig_body, fragment });
+        self.body = try std.fmt.allocPrintZ(self.allocator, "{s}{s}", .{ orig_body, fragment });
     }
 
     pub fn addHeader(self: *Self, name: []const u8, value: []const u8) !void {

src/main.zig

@@ -36,7 +36,13 @@ pub fn main() anyerror!void {
     const test_json = false;
     if (test_json) try jsonFun();
 
-    const allocator = std.heap.c_allocator;
+    const c_allocator = std.heap.c_allocator;
+    var gpa = std.heap.GeneralPurposeAllocator(.{}){
+        .backing_allocator = c_allocator,
+    };
+    defer if (!gpa.deinit()) @panic("memory leak detected");
+    const allocator = &gpa.allocator;
+    // const allocator = std.heap.c_allocator;
 
     const options = aws.Options{
         .region = "us-west-2",