Fix EventStream to properly handle utf-8 #216
Conversation
Source/AwsCommonRuntimeKit/event-stream/EventStreamMessage.swift
Outdated
Show resolved
Hide resolved
| switch header.value { | ||
| case .bool(let value): | ||
| return aws_event_stream_add_bool_header( | ||
| rawHeaders, | ||
| headerName, | ||
| UInt8(header.name.count), | ||
| headerNameLength, |
There was a problem hiding this comment.
This is spooky. Please search the codebase for all other uses of withCString or .count to see if we're making this mistake anywhere else
There was a problem hiding this comment.
Thanks, I have removed withCString since it is not needed and Swift will automatically do it for you under the hood. I have also verified the usage of .count.
| @@ -54,36 +54,37 @@ extension EventStreamMessage { | |||
| } | |||
| let addCHeader: () throws -> Int32 = { | |||
| return try header.name.withCString { headerName in | |||
| let headerNameLength = UInt8(header.name.utf8.count) | |||
There was a problem hiding this comment.
I wonder if .utf8.count is really just O(N) under the hood...
If it is, I'd be tempted to just call strlen(cstring) because it seems harder to mess up, compared to checking that we always do swiftstring.utf8.count and never accidentally do swiftstring.count
really wish withCString() passed both pointer and length to the closure
There was a problem hiding this comment.
Updated and removed withCString. Yeah, I agree it's really easy to mess up, given that we need to pass .utf8.count to C APIs instead of .count.
| value) | ||
| case .int32(let value): | ||
| return aws_event_stream_add_int32_header( | ||
| let headerNameLength = UInt8(header.name.utf8.count) |
There was a problem hiding this comment.
trivial: move this up, so we can do if headerNameLength > EventStreamHeader.maxNameLength
instead of needing to type the easy-to-mess-up .utf8.count multiple times
| header.name, | ||
| headerNameLength, | ||
| bytes, | ||
| UInt16($0.count), |
There was a problem hiding this comment.
trivial: similar, we get length in 2 mildly different ways here: value.count up at the length check, then $0.count down here. Can we use a temp variable, or do value.count in both places, or move the length-check down so we can do$0.count in both places?
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.