Skip to content

Commit

Permalink
Fix Swift dispatcher bug dispatching collocated requets - Fix #3286
Browse files Browse the repository at this point in the history
The Swift dispatcher handling of collocated requests was bogus, it can end-up reading from memory that has been released.
  • Loading branch information
pepone authored Jan 28, 2025
1 parent 2ea7528 commit e2a8f14
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 12 deletions.
5 changes: 2 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,8 @@ jobs:
- os: macos-15
config: "ios"
build_flags: "PLATFORMS='macosx iphonesimulator' CONFIGS='static shared' LANGUAGES='cpp swift'"
# Remove rfilter swift/Ice/operations once #3286 is fixed
# swift/Ice/udp doesn't work on CI
test_flags: "--languages='cpp,swift' --platform=iphonesimulator --controller-app --rfilter=swift/Ice/operations --rfilter=swift/Ice/udp"
# TODO swift/Ice/udp doesn't work on CI
test_flags: "--languages='cpp,swift' --platform=iphonesimulator --controller-app --rfilter=swift/Ice/udp"
build_cpp_and_python: true

# Static builds
Expand Down
23 changes: 14 additions & 9 deletions swift/src/IceImpl/DispatchAdapter.mm
Original file line number Diff line number Diff line change
Expand Up @@ -16,31 +16,36 @@
const std::byte* inEncaps;
std::function<void()> cleanup;

// An InputSteam can contain one or more requests when we're processing a batch request. In the case when there
// is more than one request in the the batch we need to copy the encapsulation from the InputStream to a
// heap allocated vector as the remainder of the memory is needed for subsequent requests.
if (request.requestCount() > 1)
// The Swift side can asynchronously unmarshal the request, so we need to either move or copy the encapsulation
// data to ensure it remains alive until Swift completes unmarshaling.
//
// For a batch request with multiple requests remaining in the input stream, we need to copy the encapsulation data
// because moving it isn't an option—subsequent requests still depend on the input stream.
//
// For collocated requests, we also need to copy the encapsulation data because the input stream doesn't own
// the memory. The memory is owned by the output stream used for the invocation, which might be freed before the
// input stream is unmarshaled. Additionally, we can't take ownership of the output stream because it is still
// needed in case of a retry.
//
// If neither of these conditions applies, the input stream can be safely moved to the heap for processing.
if (request.requestCount() > 1 || request.current().con == nullptr)
{
Ice::InputStream& inputStream = request.inputStream();
inputStream.readEncapsulation(inEncaps, sz);

// Copy the contents to a heap allocated vector.
auto encapsulation = new std::vector<std::byte>(inEncaps, inEncaps + sz);
inEncaps = encapsulation->data();

cleanup = [encapsulation] { delete encapsulation; };
}
else
{
// In the case of a twoway request we can just take the memory as its no longer needed after this request.
// Move the request's InputStream into a new heap allocated InputStream.
// When dispatch completes, the new InputStream will be deleted.
auto dispatchInputStream = new Ice::InputStream(std::move(request.inputStream()));

cleanup = [dispatchInputStream] { delete dispatchInputStream; };

dispatchInputStream->readEncapsulation(inEncaps, sz);
};
}

ICEOutgoingResponse outgoingResponse =
^(uint8_t replyStatus, NSString* exceptionId, NSString* exceptionDetails, const void* message, long count) {
Expand Down

0 comments on commit e2a8f14

Please sign in to comment.