Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add string array as an option for request context params for endpoint resolution #276

Merged
merged 10 commits into from
Aug 5, 2024

Conversation

DmitriyMusatkin
Copy link
Contributor

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.

Source/AwsCommonRuntimeKit/crt/Utilities.swift Outdated Show resolved Hide resolved

/// Note: this function and associated scan function above are copied verbatim from
/// swift standard lib where its private.
public func withArrayOfCStrings<R>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions should be internal to our module and not public. Also, if performance is not a concern, we should just implement the simpler version from https://oleb.net/blog/2016/10/swift-array-of-c-strings/.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh, i didnt really understand how swift works. updated to a much simpler version.


func withByteCursorArrayFromStringArray<R>(
_ arg: [String], _ body: (UnsafePointer<aws_byte_cursor>, Int) -> R) -> R {
let cursors = arg.map { aws_byte_cursor_from_c_str($0) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not memory-safe. $0 is a Swift string and is only valid for the scope of aws_byte_cursor_from_c_str. The behavior is undefined after this line because the cursor refers to a memory area not guaranteed by the Swift compiler. We will need to make a copy, and then release it using our allocator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, you are right. i added the copy back.
is it possible to run asan or some sort of mem checker in swift?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have our tracing allocator hooked up, but I never looked into ASAN. It seems easy enough to enable: https://www.swift.org/documentation/server/guides/llvm-sanitizers.html.

Source/AwsCommonRuntimeKit/crt/Utilities.swift Outdated Show resolved Hide resolved
Source/AwsCommonRuntimeKit/crt/Utilities.swift Outdated Show resolved Hide resolved
DmitriyMusatkin and others added 4 commits August 5, 2024 10:59
Co-authored-by: Waqar Ahmed Khan <waahm7@gmail.com>
Co-authored-by: Waqar Ahmed Khan <waahm7@gmail.com>
@DmitriyMusatkin DmitriyMusatkin merged commit 26be737 into main Aug 5, 2024
25 checks passed
@DmitriyMusatkin DmitriyMusatkin deleted the string_array branch August 5, 2024 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants