Skip to content

Commit e6124bc

Browse files
andrewrkandrewrk
committed
std.http.Server: add safety for invalidated Head strings
and fix bad unit test API usage that it finds
1 parent 17875d5 commit e6124bc

File tree

3 files changed

+70
-38
lines changed

3 files changed

+70
-38
lines changed

‎lib/std/http/Client.zig

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -444,8 +444,8 @@ pub const Connection = struct {
444444

445445
pub const Response = struct {
446446
request: *Request,
447-
/// Pointers in this struct are invalidated with the next call to
448-
/// `receiveHead`.
447+
/// Pointers in this struct are invalidated when the response body stream
448+
/// is initialized.
449449
head: Head,
450450

451451
pub const Head = struct {
@@ -671,6 +671,16 @@ pub const Response = struct {
671671
try expectEqual(@as(u10, 418), parseInt3("418"));
672672
try expectEqual(@as(u10, 999), parseInt3("999"));
673673
}
674+
675+
/// Help the programmer avoid bugs by calling this when the string
676+
/// memory of `Head` becomes invalidated.
677+
fn invalidateStrings(h: *Head) void {
678+
h.bytes = undefined;
679+
h.reason = undefined;
680+
if (h.location) |*s| s.* = undefined;
681+
if (h.content_type) |*s| s.* = undefined;
682+
if (h.content_disposition) |*s| s.* = undefined;
683+
}
674684
};
675685

676686
/// If compressed body has been negotiated this will return compressed bytes.
@@ -682,7 +692,8 @@ pub const Response = struct {
682692
///
683693
/// See also:
684694
/// * `readerDecompressing`
685-
pub fn reader(response: *const Response, buffer: []u8) *Reader {
695+
pub fn reader(response: *Response, buffer: []u8) *Reader {
696+
response.head.invalidateStrings();
686697
const req = response.request;
687698
if (!req.method.responseHasBody()) return .ending;
688699
const head = &response.head;
@@ -703,6 +714,7 @@ pub const Response = struct {
703714
decompressor: *http.Decompressor,
704715
decompression_buffer: []u8,
705716
) *Reader {
717+
response.head.invalidateStrings();
706718
const head = &response.head;
707719
return response.request.reader.bodyReaderDecompressing(
708720
head.transfer_encoding,

‎lib/std/http/Server.zig

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,8 @@ pub fn receiveHead(s: *Server) ReceiveHeadError!Request {
5555

5656
pub const Request = struct {
5757
server: *Server,
58-
/// Pointers in this struct are invalidated with the next call to
59-
/// `receiveHead`.
58+
/// Pointers in this struct are invalidated when the request body stream is
59+
/// initialized.
6060
head: Head,
6161
head_buffer: []const u8,
6262
respond_err: ?RespondError = null,
@@ -224,6 +224,14 @@ pub const Request = struct {
224224
inline fn int64(array: *const [8]u8) u64 {
225225
return @bitCast(array.*);
226226
}
227+
228+
/// Help the programmer avoid bugs by calling this when the string
229+
/// memory of `Head` becomes invalidated.
230+
fn invalidateStrings(h: *Head) void {
231+
h.target = undefined;
232+
if (h.expect) |*s| s.* = undefined;
233+
if (h.content_type) |*s| s.* = undefined;
234+
}
227235
};
228236

229237
pub fn iterateHeaders(r: *const Request) http.HeaderIterator {
@@ -578,9 +586,12 @@ pub const Request = struct {
578586
/// this function.
579587
///
580588
/// Asserts that this function is only called once.
589+
///
590+
/// Invalidates the string memory inside `Head`.
581591
pub fn readerExpectNone(request: *Request, buffer: []u8) *Reader {
582592
assert(request.server.reader.state == .received_head);
583593
assert(request.head.expect == null);
594+
request.head.invalidateStrings();
584595
if (!request.head.method.requestHasBody()) return .ending;
585596
return request.server.reader.bodyReader(buffer, request.head.transfer_encoding, request.head.content_length);
586597
}

‎lib/std/http/test.zig

Lines changed: 42 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -65,23 +65,22 @@ test "trailers" {
6565
try req.sendBodiless();
6666
var response = try req.receiveHead(&.{});
6767

68-
const body = try response.reader(&.{}).allocRemaining(gpa, .unlimited);
69-
defer gpa.free(body);
70-
71-
try expectEqualStrings("Hello, World!\n", body);
72-
7368
{
7469
var it = response.head.iterateHeaders();
7570
const header = it.next().?;
76-
try expect(!it.is_trailer);
7771
try expectEqualStrings("transfer-encoding", header.name);
7872
try expectEqualStrings("chunked", header.value);
7973
try expectEqual(null, it.next());
8074
}
75+
76+
const body = try response.reader(&.{}).allocRemaining(gpa, .unlimited);
77+
defer gpa.free(body);
78+
79+
try expectEqualStrings("Hello, World!\n", body);
80+
8181
{
8282
var it = response.iterateTrailers();
8383
const header = it.next().?;
84-
try expect(it.is_trailer);
8584
try expectEqualStrings("X-Checksum", header.name);
8685
try expectEqualStrings("aaaa", header.value);
8786
try expectEqual(null, it.next());
@@ -208,12 +207,14 @@ test "echo content server" {
208207
// request.head.target,
209208
//});
210209

210+
try expect(mem.startsWith(u8, request.head.target, "/echo-content"));
211+
try expectEqualStrings("text/plain", request.head.content_type.?);
212+
213+
// head strings expire here
211214
const body = try (try request.readerExpectContinue(&.{})).allocRemaining(std.testing.allocator, .unlimited);
212215
defer std.testing.allocator.free(body);
213216

214-
try expect(mem.startsWith(u8, request.head.target, "/echo-content"));
215217
try expectEqualStrings("Hello, World!\n", body);
216-
try expectEqualStrings("text/plain", request.head.content_type.?);
217218

218219
var response = try request.respondStreaming(&.{}, .{
219220
.content_length = switch (request.head.transfer_encoding) {
@@ -410,17 +411,19 @@ test "general client/server API coverage" {
410411

411412
fn handleRequest(request: *http.Server.Request, listen_port: u16) !void {
412413
const log = std.log.scoped(.server);
414+
const gpa = std.testing.allocator;
413415

414416
log.info("{f} {t} {s}", .{ request.head.method, request.head.version, request.head.target });
417+
const target = try gpa.dupe(u8, request.head.target);
418+
defer gpa.free(target);
415419

416-
const gpa = std.testing.allocator;
417420
const reader = (try request.readerExpectContinue(&.{}));
418421
const body = try reader.allocRemaining(gpa, .unlimited);
419422
defer gpa.free(body);
420423

421-
if (mem.startsWith(u8, request.head.target, "/get")) {
424+
if (mem.startsWith(u8, target, "/get")) {
422425
var response = try request.respondStreaming(&.{}, .{
423-
.content_length = if (mem.indexOf(u8, request.head.target, "?chunked") == null)
426+
.content_length = if (mem.indexOf(u8, target, "?chunked") == null)
424427
14
425428
else
426429
null,
@@ -435,7 +438,7 @@ test "general client/server API coverage" {
435438
try w.writeAll("World!\n");
436439
try response.end();
437440
// Writing again would cause an assertion failure.
438-
} else if (mem.startsWith(u8, request.head.target, "/large")) {
441+
} else if (mem.startsWith(u8, target, "/large")) {
439442
var response = try request.respondStreaming(&.{}, .{
440443
.content_length = 14 * 1024 + 14 * 10,
441444
});
@@ -458,7 +461,7 @@ test "general client/server API coverage" {
458461
}
459462

460463
try response.end();
461-
} else if (mem.eql(u8, request.head.target, "/redirect/1")) {
464+
} else if (mem.eql(u8, target, "/redirect/1")) {
462465
var response = try request.respondStreaming(&.{}, .{
463466
.respond_options = .{
464467
.status = .found,
@@ -472,14 +475,14 @@ test "general client/server API coverage" {
472475
try w.writeAll("Hello, ");
473476
try w.writeAll("Redirected!\n");
474477
try response.end();
475-
} else if (mem.eql(u8, request.head.target, "/redirect/2")) {
478+
} else if (mem.eql(u8, target, "/redirect/2")) {
476479
try request.respond("Hello, Redirected!\n", .{
477480
.status = .found,
478481
.extra_headers = &.{
479482
.{ .name = "location", .value = "/redirect/1" },
480483
},
481484
});
482-
} else if (mem.eql(u8, request.head.target, "/redirect/3")) {
485+
} else if (mem.eql(u8, target, "/redirect/3")) {
483486
const location = try std.fmt.allocPrint(gpa, "http://127.0.0.1:{d}/redirect/2", .{
484487
listen_port,
485488
});
@@ -491,23 +494,23 @@ test "general client/server API coverage" {
491494
.{ .name = "location", .value = location },
492495
},
493496
});
494-
} else if (mem.eql(u8, request.head.target, "/redirect/4")) {
497+
} else if (mem.eql(u8, target, "/redirect/4")) {
495498
try request.respond("Hello, Redirected!\n", .{
496499
.status = .found,
497500
.extra_headers = &.{
498501
.{ .name = "location", .value = "/redirect/3" },
499502
},
500503
});
501-
} else if (mem.eql(u8, request.head.target, "/redirect/5")) {
504+
} else if (mem.eql(u8, target, "/redirect/5")) {
502505
try request.respond("Hello, Redirected!\n", .{
503506
.status = .found,
504507
.extra_headers = &.{
505508
.{ .name = "location", .value = "/%2525" },
506509
},
507510
});
508-
} else if (mem.eql(u8, request.head.target, "/%2525")) {
511+
} else if (mem.eql(u8, target, "/%2525")) {
509512
try request.respond("Encoded redirect successful!\n", .{});
510-
} else if (mem.eql(u8, request.head.target, "/redirect/invalid")) {
513+
} else if (mem.eql(u8, target, "/redirect/invalid")) {
511514
const invalid_port = try getUnusedTcpPort();
512515
const location = try std.fmt.allocPrint(gpa, "http://127.0.0.1:{d}", .{invalid_port});
513516
defer gpa.free(location);
@@ -518,7 +521,7 @@ test "general client/server API coverage" {
518521
.{ .name = "location", .value = location },
519522
},
520523
});
521-
} else if (mem.eql(u8, request.head.target, "/empty")) {
524+
} else if (mem.eql(u8, target, "/empty")) {
522525
try request.respond("", .{
523526
.extra_headers = &.{
524527
.{ .name = "empty", .value = "" },
@@ -559,11 +562,12 @@ test "general client/server API coverage" {
559562
try req.sendBodiless();
560563
var response = try req.receiveHead(&redirect_buffer);
561564

565+
try expectEqualStrings("text/plain", response.head.content_type.?);
566+
562567
const body = try response.reader(&.{}).allocRemaining(gpa, .unlimited);
563568
defer gpa.free(body);
564569

565570
try expectEqualStrings("Hello, World!\n", body);
566-
try expectEqualStrings("text/plain", response.head.content_type.?);
567571
}
568572

569573
// connection has been kept alive
@@ -604,12 +608,13 @@ test "general client/server API coverage" {
604608
try req.sendBodiless();
605609
var response = try req.receiveHead(&redirect_buffer);
606610

611+
try expectEqualStrings("text/plain", response.head.content_type.?);
612+
try expectEqual(14, response.head.content_length.?);
613+
607614
const body = try response.reader(&.{}).allocRemaining(gpa, .unlimited);
608615
defer gpa.free(body);
609616

610617
try expectEqualStrings("", body);
611-
try expectEqualStrings("text/plain", response.head.content_type.?);
612-
try expectEqual(14, response.head.content_length.?);
613618
}
614619

615620
// connection has been kept alive
@@ -628,11 +633,12 @@ test "general client/server API coverage" {
628633
try req.sendBodiless();
629634
var response = try req.receiveHead(&redirect_buffer);
630635

636+
try expectEqualStrings("text/plain", response.head.content_type.?);
637+
631638
const body = try response.reader(&.{}).allocRemaining(gpa, .unlimited);
632639
defer gpa.free(body);
633640

634641
try expectEqualStrings("Hello, World!\n", body);
635-
try expectEqualStrings("text/plain", response.head.content_type.?);
636642
}
637643

638644
// connection has been kept alive
@@ -651,12 +657,13 @@ test "general client/server API coverage" {
651657
try req.sendBodiless();
652658
var response = try req.receiveHead(&redirect_buffer);
653659

660+
try expectEqualStrings("text/plain", response.head.content_type.?);
661+
try expect(response.head.transfer_encoding == .chunked);
662+
654663
const body = try response.reader(&.{}).allocRemaining(gpa, .unlimited);
655664
defer gpa.free(body);
656665

657666
try expectEqualStrings("", body);
658-
try expectEqualStrings("text/plain", response.head.content_type.?);
659-
try expect(response.head.transfer_encoding == .chunked);
660667
}
661668

662669
// connection has been kept alive
@@ -677,11 +684,12 @@ test "general client/server API coverage" {
677684
try req.sendBodiless();
678685
var response = try req.receiveHead(&redirect_buffer);
679686

687+
try expectEqualStrings("text/plain", response.head.content_type.?);
688+
680689
const body = try response.reader(&.{}).allocRemaining(gpa, .unlimited);
681690
defer gpa.free(body);
682691

683692
try expectEqualStrings("Hello, World!\n", body);
684-
try expectEqualStrings("text/plain", response.head.content_type.?);
685693
}
686694

687695
// connection has been closed
@@ -706,18 +714,19 @@ test "general client/server API coverage" {
706714

707715
try std.testing.expectEqual(.ok, response.head.status);
708716

709-
const body = try response.reader(&.{}).allocRemaining(gpa, .unlimited);
710-
defer gpa.free(body);
711-
712-
try expectEqualStrings("", body);
713-
714717
var it = response.head.iterateHeaders();
715718
{
716719
const header = it.next().?;
717720
try expect(!it.is_trailer);
718721
try expectEqualStrings("content-length", header.name);
719722
try expectEqualStrings("0", header.value);
720723
}
724+
725+
const body = try response.reader(&.{}).allocRemaining(gpa, .unlimited);
726+
defer gpa.free(body);
727+
728+
try expectEqualStrings("", body);
729+
721730
{
722731
const header = it.next().?;
723732
try expect(!it.is_trailer);

0 commit comments

Comments
 (0)